REGRESSION (r167856): adobe.com no longer able to launch Create Cloud app using a URL with a custom scheme

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

REGRESSION (r167856): adobe.com no longer able to launch Create Cloud app using a URL with a custom scheme

[Andy Estes]

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.

[Andy Estes]

Created attachment 236699 [details]
Patch

[Andy Estes]

Created attachment 236706 [details]
Patch

[Andy Estes]

Created attachment 236707 [details]
Patch

[Darin Adler]

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.

[Andy Estes]

<rdar://problem/17850158>

[Andy Estes]

Committed r172697: <http://trac.webkit.org/changeset/172697>

[Andy Estes]

(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.

[Andy Estes]

(In reply to comment #8)
> 
> scheduleLocationChange is handled as a redirect in some cases, but not in others.

As a quick redirect, that is.

[Darin Adler]

For that matter, scheduleRedirect is not always handled as a quick redirect either. Sometimes it’s a non-quick redirect.

[Andy Estes]

Reverted r172697 in <http://trac.webkit.org/changeset/172699> since the change also made some layout tests flaky.

[Andy Estes]

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.

[Andy Estes]

Created attachment 236761 [details]
Patch

[Darin Adler]

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.

[Andy Estes]

Committed r172709: <http://trac.webkit.org/changeset/172709>

[Andy Estes]

(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.

[Darin Adler]

(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.