Bug 225923 - FrameLoader::loadURL, FrameLoader::loadPostRequest not attributing requests as app bound
Summary: FrameLoader::loadURL, FrameLoader::loadPostRequest not attributing requests a...
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-18 10:11 PDT by Kate Cheney
Modified: 2021-05-21 09:41 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.72 KB, patch)
2021-05-18 10:18 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (16.50 KB, patch)
2021-05-19 17:40 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (16.51 KB, patch)
2021-05-19 17:50 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (24.49 KB, patch)
2021-05-20 17:06 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (23.81 KB, patch)
2021-05-21 09:03 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-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].