Created attachment 382823 [details] Sample project reproducing the issue WKWebView has a hard time navigating away from about:blank. We noticed two issues, the first one of them is new to iOS 13.2: 1. In iOS 13.2, WKWebView fails provisional navigation when navigating from about:blank to HTML loaded through `-[WKWebView loadFileURL:]`. This used to work in iOS 13.1 (and earlier). 2. In iOS 12.0-13.2, WKWebView fails provisional navigation when navigating from about:blank to HTML loaded through `-[WKWebView loadRequest:]`. I am only able to reproduce this on a device (tested on iPhone X and iPod touch 7th gen), not on the iOS simulator in Xcode 11. I attached a small sample project that reproduces the issue. When run, there is a WKWebView and 4 buttons in the toolbar at the bottom. With the sample project, the first issue (the iOS 13.2 regression) can be reproduced by tapping on: 1. "fileFromURL" 2. "blank" 3. "fileFromURL" Here I expect the fileFromURL HTML to show up on step 3, but nothing shows up and the WKNavigationalDelegate method `webView:didFailProvisionalNavigation:withError:` is called. For the second issue, it can be reproduced in the sample project by tapping: 1. "request" 2. "blank" 3. "request" Here I expect the HTML loaded by "request" to be shown on step 3, but nothing appears and the WKNavigationalDelegate method `webView:didFailProvisionalNavigation:withError:` is called. Thank you, Felix Lapalme
Still same broken behavior on 13.3 (17C5032d).
<rdar://problem/56973112>
Created attachment 383475 [details] Patch
Comment on attachment 383475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383475&action=review I think this looks good (as far as the mechanics of the change) but I have some questions to understand the reason for the change. > Source/WebKit/ChangeLog:15 > + assumed access. I’m a little confused by this explanation. Do we always revoke access to local files as soon as the loads are finished? (This does seem like a good thing). If that is the case, why do we need the flag? Shouldn’t we always need to create the extension? Are there some cases where we do not revoke the extension after a file loads? If there are, I don’t understand why — shouldn’t we give up privileges as soon as we are done using them? > Tools/TestWebKitAPI/Tests/WebKitCocoa/LoadFileURL.mm:98 > +TEST(WKWebView, RepeatLoadFileURL) It seems like we should also have a test of the case that we have revoked our permission, and should not have it. Can we test that the extension was properly revoked and the load fails in that case (for example a successful load where we revoke access, but a simulated attacker tries to load the file again)
(In reply to Brent Fulgham from comment #4) > Comment on attachment 383475 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383475&action=review > > I think this looks good (as far as the mechanics of the change) but I have > some questions to understand the reason for the change. > > > Source/WebKit/ChangeLog:15 > > + assumed access. > > I’m a little confused by this explanation. Do we always revoke access to > local files as soon as the loads are finished? (This does seem like a good > thing). > Yes, I believe so, but we wait to revoke until the next load starts. > If that is the case, why do we need the flag? Shouldn’t we always need to > create the extension? > That is a good point. The reason for this approach was to have a minimal change which restored the previous behavior in order to avoid further regressions. I was a little worried about always creating the extensions in WebPageProxy::maybeInitializeSandboxExtension, since it has multiple callers, and creating too many unneeded extensions would not be optimal for memory-use and performance reasons. > Are there some cases where we do not revoke the extension after a file > loads? If there are, I don’t understand why — shouldn’t we give up > privileges as soon as we are done using them? > In my limited testing, I have not seen any such cases. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/LoadFileURL.mm:98 > > +TEST(WKWebView, RepeatLoadFileURL) > > It seems like we should also have a test of the case that we have revoked > our permission, and should not have it. Can we test that the extension was > properly revoked and the load fails in that case (for example a successful > load where we revoke access, but a simulated attacker tries to load the file > again) Having a test that makes sure the extension is revoked after a load is an excellent idea. Would you be OK with doing this in a follow-up patch? Thanks for reviewing :)
Comment on attachment 383475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383475&action=review >>> Tools/TestWebKitAPI/Tests/WebKitCocoa/LoadFileURL.mm:98 >>> +TEST(WKWebView, RepeatLoadFileURL) >> >> It seems like we should also have a test of the case that we have revoked our permission, and should not have it. Can we test that the extension was properly revoked and the load fails in that case (for example a successful load where we revoke access, but a simulated attacker tries to load the file again) > > Having a test that makes sure the extension is revoked after a load is an excellent idea. Would you be OK with doing this in a follow-up patch? > > Thanks for reviewing :) Sure -- a second patch to add this testing (and address any issues it discovers) seems like a good idea, since this patch just restores original behavior.
(In reply to Brent Fulgham from comment #6) > Comment on attachment 383475 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383475&action=review > > >>> Tools/TestWebKitAPI/Tests/WebKitCocoa/LoadFileURL.mm:98 > >>> +TEST(WKWebView, RepeatLoadFileURL) > >> > >> It seems like we should also have a test of the case that we have revoked our permission, and should not have it. Can we test that the extension was properly revoked and the load fails in that case (for example a successful load where we revoke access, but a simulated attacker tries to load the file again) > > > > Having a test that makes sure the extension is revoked after a load is an excellent idea. Would you be OK with doing this in a follow-up patch? > > > > Thanks for reviewing :) > > Sure -- a second patch to add this testing (and address any issues it > discovers) seems like a good idea, since this patch just restores original > behavior. Created https://bugs.webkit.org/show_bug.cgi?id=204181.
Comment on attachment 383475 [details] Patch Clearing flags on attachment: 383475 Committed r252442: <https://trac.webkit.org/changeset/252442>
All reviewed patches have been landed. Closing bug.