Bug 234015 – Propagate app-initiated state to new ResourceRequests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

While reviewing logging we have found that some ResourceRequest objects are missing the correct state for app-initiated loads. Typically, these are cases where a new ResourceRequest is created from a URL string, losing the context that came with the original load (or the context of the page triggering the load).

This patch corrects a number of such cases.

[Brent Fulgham]

<rdar://problem/86128638>

[Brent Fulgham]

Created attachment 446382 [details]
WIP Patch

[Kate Cheney]

Comment on attachment 446382 [details]
WIP Patch

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

Thanks for covering these cases. r=me with gtk/wpe/wincairo/win fixes.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:97
> +    if (auto* documentLoader = m_document->loader())

Seems like you may need to #include DocumentLoader.h here for GTK and WPE.

> Source/WebCore/loader/FrameLoader.cpp:3058
> +    workingResourceRequest.setIsAppInitiated(inRequest.isAppInitiated());

We set this in updateRequestAndAddExtraFields which is called below, so no need to do it here.

[Brent Fulgham]

Comment on attachment 446382 [details]
WIP Patch

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

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:97
>> +    if (auto* documentLoader = m_document->loader())
> 
> Seems like you may need to #include DocumentLoader.h here for GTK and WPE.

Will do!

>> Source/WebCore/loader/FrameLoader.cpp:3058
>> +    workingResourceRequest.setIsAppInitiated(inRequest.isAppInitiated());
> 
> We set this in updateRequestAndAddExtraFields which is called below, so no need to do it here.

Ah! Good catch. Will remove.

[Brent Fulgham]

Created attachment 446391 [details]
Patch

[Brent Fulgham]

Created attachment 447671 [details]
WIP Patch

[Brent Fulgham]

Comment on attachment 447671 [details]
WIP Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:239
> +            WTFReportBacktraceWithPrefix("WebLoaderStrategy");

This debugging code should not be landed.

[Brent Fulgham]

Created attachment 447731 [details]
Patch

[Kate Cheney]

Comment on attachment 447731 [details]
Patch

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

One thing to note is only the main frame's document loader holds the correct value, so there may be some places where we need to check that value instead of the current document's loader.

> Source/WebCore/loader/FrameLoader.cpp:978
> +        frameLoadRequest.resourceRequest().setIsAppInitiated(*appInitiated);

I think this one is unnecessary because loadURL() called below will call updateRequestAndAddExtraFields which will set the value.

> Source/WebKit/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp:50
> +    encoder << m_isAppInitiated;

Good catch.

[Kate Cheney]

Comment on attachment 447731 [details]
Patch

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

> Source/WebCore/loader/NavigationScheduler.cpp:198
> +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

I think this will also always end up in a frame loader function that calls updateRequestAndAddExtraFields, so it is not needed.

> Source/WebCore/loader/NavigationScheduler.cpp:232
> +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Ditto

> Source/WebCore/loader/NavigationScheduler.cpp:261
> +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Ditto.

> Source/WebCore/loader/NavigationScheduler.cpp:378
> +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Ditto.

> Source/WebCore/loader/NavigationScheduler.cpp:482
> +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Ditto.

> Source/WebCore/page/DOMWindow.cpp:2545
> +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Ditto here. changeLocation calls loadFrameRequest which calls loadURL or loadPostRequest which will both eventually call updateRequestAndAddExtraFields.

> Source/WebCore/page/DragController.cpp:300
> +        resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Here too.

[Brent Fulgham]

Created attachment 447742 [details]
Patch

[Brent Fulgham]

Created attachment 447763 [details]
Patch

[EWS]

Committed r287348 (245490@main): <https://commits.webkit.org/245490@main>

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