Bug 225829 - WebFrameLoaderClient::dispatchWillSendRequest sometimes resets app-bound value
Summary: WebFrameLoaderClient::dispatchWillSendRequest sometimes resets app-bound value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-14 15:05 PDT by Kate Cheney
Modified: 2021-05-17 18:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.28 KB, patch)
2021-05-14 15:09 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (12.40 KB, patch)
2021-05-17 11:04 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (12.89 KB, patch)
2021-05-17 17:56 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (11.49 KB, patch)
2021-05-17 18:00 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-05-14 15:05:25 PDT
webPage->injectedBundleResourceLoadClient().willSendRequestForFrame can sometimes return a completely new request. We should make sure the app-bound value is kept when this happens.
Comment 1 Radar WebKit Bug Importer 2021-05-14 15:05:44 PDT
<rdar://problem/78034595>
Comment 2 Kate Cheney 2021-05-14 15:09:35 PDT
Created attachment 428669 [details]
Patch
Comment 3 Darin Adler 2021-05-15 15:48:15 PDT
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.
Comment 4 Darin Adler 2021-05-15 15:51:56 PDT
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.
Comment 5 Kate Cheney 2021-05-15 21:09:50 PDT
(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.
Comment 6 Kate Cheney 2021-05-15 21:11:45 PDT
(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.
Comment 7 Kate Cheney 2021-05-17 11:04:21 PDT
Created attachment 428846 [details]
Patch
Comment 8 Kate Cheney 2021-05-17 11:04:57 PDT
(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.
Comment 9 Alex Christensen 2021-05-17 12:24:38 PDT
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.
Comment 10 Kate Cheney 2021-05-17 12:33:52 PDT
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.
Comment 11 Chris Dumez 2021-05-17 12:35:52 PDT
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()
Comment 12 Kate Cheney 2021-05-17 17:56:25 PDT
Created attachment 428901 [details]
Patch for landing
Comment 13 EWS 2021-05-17 17:57:10 PDT
ChangeLog entry in LayoutTests/ChangeLog is not at the top of the file.
Comment 14 Kate Cheney 2021-05-17 18:00:18 PDT
Created attachment 428902 [details]
Patch for landing
Comment 15 EWS 2021-05-17 18:52:24 PDT
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].