Summary: | WebFrameLoaderClient::dispatchWillSendRequest sometimes resets app-bound value | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cdumez, darin, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Kate Cheney
2021-05-14 15:05:25 PDT
Created attachment 428669 [details]
Patch
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 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. (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. (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. Created attachment 428846 [details]
Patch
(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 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 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 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() Created attachment 428901 [details]
Patch for landing
ChangeLog entry in LayoutTests/ChangeLog is not at the top of the file. Created attachment 428902 [details]
Patch for landing
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]. |