search

mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.36k stars 1.33k forks source link

Fix error receiving local file URL response #16428

Closed 1ec5 closed 4 years ago

1ec5 commented 4 years ago

Happily accept the data in an NSURLResponse that isn’t an NSHTTPURLResponse as long as the file is a local file URL. I left the existing else statement in place just in case, but we could probably remove that error to fix other non-HTTP protocols like smb:.

Fixes #16427.

/cc @mapbox/maps-ios

1ec5 commented 4 years ago

Ordinarily, I’d say this change is pretty risk given the area of code where it takes place. However, it’s guaranteed that the same request would’ve previously resulted in an error, making it unlikely that anyone would’ve relied on redirecting to a local file URL in production code up to now. The risk is that there’s some kind of local file – a directory, perhaps? – that somehow makes it this far and has a valid response but is unsuitable for use in mbgl. But again, I don’t think anyone would be relying on that behavior, so it would be a bug perhaps but not a regression.

1ec5 commented 4 years ago

I’ve verified that requesting a directory results in an error that is caught early on in the completion handler:

2020-04-24 09:59:14.742756-0700 xctest[47455:7552181] Task <558A006B-2EF6-4E9C-A88F-0206DAE82C77>.<1> finished with error [-1101] Error Domain=NSURLErrorDomain Code=-1101 "file is directory" UserInfo={NSUnderlyingError=0x100eb9930 {Error Domain=kCFErrorDomainCFNetwork Code=-1101 "(null)"}, NSErrorFailingURLStringKey=file:///path/to/mapbox-gl-native-ios/build/macos/Debug/test.xctest/Contents/Resources/, NSErrorFailingURLKey=file:///path/to/mapbox-gl-native-ios/build/macos/Debug/test.xctest/Contents/Resources/, NSLocalizedDescription=file is directory}

It would be possible to write a unit test that verifies this case in the iOS/macOS map SDK’s unit test suite and even in http_file_source.test.cpp in this repository. But I’m not sure if any other platform supports this code path, where a file URL ends up being treated mostly the same as any HTTP URL. There may be significant differences in how file URLs are handled on Android, for example.