Bug 225923

Summary: FrameLoader::loadURL, FrameLoader::loadPostRequest not attributing requests as app bound
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, ews-watchlist, japhet, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Kate Cheney 2021-05-18 10:11:27 PDT
We should mark loadURL requests as app-bound.
Comment 1 Radar WebKit Bug Importer 2021-05-18 10:11:43 PDT
<rdar://problem/78160933>
Comment 2 Kate Cheney 2021-05-18 10:18:43 PDT
Created attachment 428952 [details]
Patch
Comment 3 youenn fablet 2021-05-19 07:39:05 PDT
Comment on attachment 428952 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:1341
> +        request.setIsAppBound(m_documentLoader->lastNavigationWasAppBound());

What about the other call sites like loadPostRequest?
Comment 4 Kate Cheney 2021-05-19 17:40:29 PDT
Created attachment 429121 [details]
Patch
Comment 5 Kate Cheney 2021-05-19 17:41:16 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 428952 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428952&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:1341
> > +        request.setIsAppBound(m_documentLoader->lastNavigationWasAppBound());
> 
> What about the other call sites like loadPostRequest?

You're right. I moved this to FrameLoader::addExtraFieldsToRequest so it will be caught in other cases. And added more tests.
Comment 6 Kate Cheney 2021-05-19 17:50:25 PDT
Created attachment 429123 [details]
Patch
Comment 7 Alex Christensen 2021-05-20 15:28:01 PDT
Comment on attachment 429123 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        loadURL not attributing requests as app bound

FrameLoader:: would help understand what is going on here.

> Source/WebCore/loader/FrameLoader.cpp:1495
> +    addExtraFieldsToRequest(r, IsMainResource::Yes, m_loadType, ShouldUpdateAppBoundValue::No);

We should probably rename addExtraFieldsToRequest because it's no longer just about fields.

> LayoutTests/http/tests/in-app-browser-privacy/app-bound-attribution-ping-load.html:15
> +

extra space
Comment 8 Kate Cheney 2021-05-20 17:06:37 PDT
Created attachment 429245 [details]
Patch
Comment 9 Alex Christensen 2021-05-20 20:14:14 PDT
Comment on attachment 429245 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.h:89
> +enum class ShouldUpdateAppBoundValue { No, Yes };

: bool
Comment 10 Kate Cheney 2021-05-21 09:03:53 PDT
Created attachment 429296 [details]
Patch for landing
Comment 11 EWS 2021-05-21 09:41:39 PDT
Committed r277865 (238006@main): <https://commits.webkit.org/238006@main>

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