Bug 225829 – WebFrameLoaderClient::dispatchWillSendRequest sometimes resets app-bound value

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

webPage->injectedBundleResourceLoadClient().willSendRequestForFrame can sometimes return a completely new request. We should make sure the app-bound value is kept when this happens.

[Radar WebKit Bug Importer]

<rdar://problem/78034595>

[Kate Cheney]

Created attachment 428669 [details]
Patch

[Darin Adler]

Comment on attachment 428669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428669&action=review

> Source/WebKit/ChangeLog:10
> +        No new tests, was not able to reproduce this in our testing
> +        infrastructure, only in real use cases. I tested this manually.

This is risky. Seems likely we’ll make the similar mistake again if we can’t test it. We should consider how to enhance our testing machinery so we can test it. Probably means adding a case of an injected bundle client that implements willSendRequest and an API test.

[Darin Adler]

Comment on attachment 428669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428669&action=review

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:251
>      auto requester = request.requester();
> +    auto appBoundValue = request.isAppBound();
>      webPage->injectedBundleResourceLoadClient().willSendRequestForFrame(*webPage, m_frame, identifier, request, redirectResponse);
> -    if (!request.isNull())
> +    if (!request.isNull()) {
>          request.setRequester(requester);
> +        request.setIsAppBound(appBoundValue);
> +    }

I’m thinking that this a clue about the granularity of our objects being wrong. If we can’t trust some of the fields on the request that comes back from the resource load client, then instead of carrying context from one request to the next, we probably need to refactor so we only get the parts of the request that we *can* trust from the client and combine it with other things we already have that the client cannot have any influence on. Not a trivial refactoring, but I think it would likely be worth it.

[Kate Cheney]

(In reply to Darin Adler from comment #3)
> Comment on attachment 428669 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428669&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        No new tests, was not able to reproduce this in our testing
> > +        infrastructure, only in real use cases. I tested this manually.
> 
> This is risky. Seems likely we’ll make the similar mistake again if we can’t
> test it. We should consider how to enhance our testing machinery so we can
> test it. Probably means adding a case of an injected bundle client that
> implements willSendRequest and an API test.

Alex told me how to write a test. I will cq- and upload a new patch with a test.

[Kate Cheney]

(In reply to Darin Adler from comment #4)
> Comment on attachment 428669 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428669&action=review
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:251
> >      auto requester = request.requester();
> > +    auto appBoundValue = request.isAppBound();
> >      webPage->injectedBundleResourceLoadClient().willSendRequestForFrame(*webPage, m_frame, identifier, request, redirectResponse);
> > -    if (!request.isNull())
> > +    if (!request.isNull()) {
> >          request.setRequester(requester);
> > +        request.setIsAppBound(appBoundValue);
> > +    }
> 
> I’m thinking that this a clue about the granularity of our objects being
> wrong. If we can’t trust some of the fields on the request that comes back
> from the resource load client, then instead of carrying context from one
> request to the next, we probably need to refactor so we only get the parts
> of the request that we *can* trust from the client and combine it with other
> things we already have that the client cannot have any influence on. Not a
> trivial refactoring, but I think it would likely be worth it.

That's a good point. I will look into it and see if I can figure it out for this patch.

[Kate Cheney]

Created attachment 428846 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #6)
> (In reply to Darin Adler from comment #4)
> > Comment on attachment 428669 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=428669&action=review
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:251
> > >      auto requester = request.requester();
> > > +    auto appBoundValue = request.isAppBound();
> > >      webPage->injectedBundleResourceLoadClient().willSendRequestForFrame(*webPage, m_frame, identifier, request, redirectResponse);
> > > -    if (!request.isNull())
> > > +    if (!request.isNull()) {
> > >          request.setRequester(requester);
> > > +        request.setIsAppBound(appBoundValue);
> > > +    }
> > 
> > I’m thinking that this a clue about the granularity of our objects being
> > wrong. If we can’t trust some of the fields on the request that comes back
> > from the resource load client, then instead of carrying context from one
> > request to the next, we probably need to refactor so we only get the parts
> > of the request that we *can* trust from the client and combine it with other
> > things we already have that the client cannot have any influence on. Not a
> > trivial refactoring, but I think it would likely be worth it.
> 
> That's a good point. I will look into it and see if I can figure it out for
> this patch.

Talked offline with Alex, I think this would be better in another patch. Latest patch does add a test.

[Alex Christensen]

Comment on attachment 428846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428846&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacyPlugIn.mm:33
> +#import <WebKit/WKWebProcessPlugInRangeHandle.h>

Unused includes.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacyPlugIn.mm:47
> +- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController didCreateBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController

All this is probably unnecessary.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacyPlugIn.mm:59
> +    return [[NSURLRequest alloc] initWithURL:request.URL];

This needs an autorelease.

[Kate Cheney]

Comment on attachment 428846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428846&action=review

Thanks Alex, I'll fix before landing.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacyPlugIn.mm:33
>> +#import <WebKit/WKWebProcessPlugInRangeHandle.h>
> 
> Unused includes.

copy-paste fail :(

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacyPlugIn.mm:47
>> +- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController didCreateBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController
> 
> All this is probably unnecessary.

copy-paste fail pt 2.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacyPlugIn.mm:59
>> +    return [[NSURLRequest alloc] initWithURL:request.URL];
> 
> This needs an autorelease.

Will fix.

[Chris Dumez]

Comment on attachment 428846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428846&action=review

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacyPlugIn.mm:59
>>> +    return [[NSURLRequest alloc] initWithURL:request.URL];
>> 
>> This needs an autorelease.
> 
> Will fix.

adoptNS([[NSURLRequest alloc] initWithURL:request.URL]).autorelease()

[Kate Cheney]

Created attachment 428901 [details]
Patch for landing

[EWS]

ChangeLog entry in LayoutTests/ChangeLog is not at the top of the file.

[Kate Cheney]

Created attachment 428902 [details]
Patch for landing

[EWS]

Committed r277628 (237839@main): <https://commits.webkit.org/237839@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428902 [details].