REGRESSION (r167856): adobe.com no longer able to launch Create Cloud app using a URL with a custom scheme
http://trac.webkit.org/changeset/167856 caused WebCore to stop scheduling navigations to invalid URLs in some cases (window.location changes, meta-refresh, and some calls to window.open). As a result, the client's policy delegate is no longer called for such navigations. Adobe.com is relying on Safari's navigation policy delegate to launch Create Cloud on navigations to its aam: scheme (but otherwise ignore the navigation), and unfortunately they use URLs that aren't valid to do so.
Created attachment 236699 [details] Patch
Created attachment 236706 [details] Patch
Created attachment 236707 [details] Patch
Comment on attachment 236707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236707&action=review > Source/WebCore/loader/FrameLoader.cpp:2901 > + bool willRedirectToInvalidURL = m_quickRedirectComing && !request.url().isValid(); What about non-redirect requests that are for an invalid URL? The code in NavigationScheduler applies for scheduleLocationChange, not just scheduleRedirect.
<rdar://problem/17850158>
Committed r172697: <http://trac.webkit.org/changeset/172697>
(In reply to comment #5) > (From update of attachment 236707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236707&action=review > > > Source/WebCore/loader/FrameLoader.cpp:2901 > > + bool willRedirectToInvalidURL = m_quickRedirectComing && !request.url().isValid(); > > What about non-redirect requests that are for an invalid URL? The code in NavigationScheduler applies for scheduleLocationChange, not just scheduleRedirect. Oh, thanks for pointing this out. scheduleLocationChange is handled as a redirect in some cases, but not in others. The two cases I didn't cover are fragment navigations and non-quick redirects. I'll post a second patch that addresses these cases.
(In reply to comment #8) > > scheduleLocationChange is handled as a redirect in some cases, but not in others. As a quick redirect, that is.
For that matter, scheduleRedirect is not always handled as a quick redirect either. Sometimes it’s a non-quick redirect.
Reverted r172697 in <http://trac.webkit.org/changeset/172699> since the change also made some layout tests flaky.
There were at least two problems with the patch that was rolled out: 1. As discussed, it did not disallow non-quick redirects to invalid URLs like r167856 did. 2. A navigation other than the one started by NavigationScheduler could be incorrectly disallowed, for instance if a load is started from the API while the NavigationScheduler's timer is active (because m_quickRedirectComing would be true even for the API load). Instead of relying on m_quickRedirectComing I added an AllowNavigationToInvalidURL enum and piped it down to continueLoadAfterNavigationPolicy. Only the scheduled navigation types checked by r167856 (ScheduledRedirect and ScheduledLocationChange) use the AllowNavigationToInvalidURL::No value.
Created attachment 236761 [details] Patch
Comment on attachment 236761 [details] Patch I’ll have to take your word for the fact that this is right and covers all the cases. I can’t follow all the different paths from reading the code. I suppose test coverage also may make it sufficiently clear. But the logic here is quite hard to follow. I am hoping that later we can disallow invalid URLs in even more cases, while allowing them in the few cases that we need to; I hope that eventually the default behavior can be No instead of Yes. It also seems like a serious problem that the frame loader has so many different load calls.
Committed r172709: <http://trac.webkit.org/changeset/172709>
(In reply to comment #14) > (From update of attachment 236761 [details]) > I’ll have to take your word for the fact that this is right and covers all the cases. I can’t follow all the different paths from reading the code. I suppose test coverage also may make it sufficiently clear. But the logic here is quite hard to follow. I am hoping that later we can disallow invalid URLs in even more cases, while allowing them in the few cases that we need to; I hope that eventually the default behavior can be No instead of Yes. It also seems like a serious problem that the frame loader has so many different load calls. I agree that the many different load calls is a serious problem. I was disappointed with the amount of plumbing necessary to get a value from NavigationScheduler to the right FrameLoader function. I intentionally avoided using a default value for the AllowNavigationToInvalidURL arguments so that finding the additional places where we can disallow invalid URLs just requires looking for AllowNavigationToInvalidURL::Yes. I hope this makes it easier to migrate more loading code paths over to disallowing invalid URLs.
(In reply to comment #16) > I intentionally avoided using a default value for the AllowNavigationToInvalidURL arguments so that finding the additional places where we can disallow invalid URLs just requires looking for AllowNavigationToInvalidURL::Yes. Except for changeLocation. Also note that sometimes the function’s job is so abstract that it’s really hard to understand if Yes or No is correct.