Bug 238159 – [Cocoa] Ensure correct isAppInitiated state when creating URLRequests from URL

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This patch follows on the work from Bug 236167. It corrects a few more load paths are getting incorrect state for 'isAppInitiated'.

[Brent Fulgham]

<rdar://problem/88490742>

[Brent Fulgham]

Created attachment 455296 [details]
Patch

[Kate Cheney]

Comment on attachment 455296 [details]
Patch

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

Have you seen these code paths be hit with incorrect app initiated values? For all of the NavigationScheduler functions we eventually call frame.changeLocation() which will set the app initiated value on the request in FrameLoader::updateRequestAndAddExtraFields based on the main frame document loader's value. Ditto for FrameLoader::loadFrameRequest() in the contextMenuItem case.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:831
> +    preconnectTo(WTFMove(resourceRequest), *webPage, webFrame, storedCredentialsPolicy, shouldPreconnectAsFirstParty, WTFMove(completionHandler));

I think we already do this, see the preconnectTo function below.

[Brent Fulgham]

Comment on attachment 455296 [details]
Patch

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

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:831
>> +    preconnectTo(WTFMove(resourceRequest), *webPage, webFrame, storedCredentialsPolicy, shouldPreconnectAsFirstParty, WTFMove(completionHandler));
> 
> I think we already do this, see the preconnectTo function below.

You are right -- this isn't needed.

[Brent Fulgham]

(In reply to Kate Cheney from comment #3)
> Comment on attachment 455296 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455296&action=review
> 
> Have you seen these code paths be hit with incorrect app initiated values?
> For all of the NavigationScheduler functions we eventually call
> frame.changeLocation() which will set the app initiated value on the request
> in FrameLoader::updateRequestAndAddExtraFields based on the main frame
> document loader's value. Ditto for FrameLoader::loadFrameRequest() in the
> contextMenuItem case.

Ah, okay. I can remove those.

> 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:831
> > +    preconnectTo(WTFMove(resourceRequest), *webPage, webFrame, storedCredentialsPolicy, shouldPreconnectAsFirstParty, WTFMove(completionHandler));
> 
> I think we already do this, see the preconnectTo function below.

I'm suspicious of this code path because the hits we've been getting seem to be related to code like this:

<link rel="preconnect" href="https://something.example.com">

[Brent Fulgham]

(In reply to Kate Cheney from comment #3)
> Comment on attachment 455296 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455296&action=review
> 
> Have you seen these code paths be hit with incorrect app initiated values?
> For all of the NavigationScheduler functions we eventually call
> frame.changeLocation() which will set the app initiated value on the request
> in FrameLoader::updateRequestAndAddExtraFields based on the main frame
> document loader's value. Ditto for FrameLoader::loadFrameRequest() in the
> contextMenuItem case.
> 

I believe that NavigationScheduler does have one case that uses:

    frame.loader().load(WTFMove(frameLoadRequest));

It looks like FrameLoader::load(FrameLoadRequest&& request) can bypass 'updateRequestAndAddExtraFields' in the case where request.shouldCheckNewWindowPolicy() is true, which I think can get through the load without setting AppInitiated state.

[Brent Fulgham]

Created attachment 455653 [details]
Patch

[Kate Cheney]

(In reply to Brent Fulgham from comment #6)
> (In reply to Kate Cheney from comment #3)
> > Comment on attachment 455296 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=455296&action=review
> > 
> > Have you seen these code paths be hit with incorrect app initiated values?
> > For all of the NavigationScheduler functions we eventually call
> > frame.changeLocation() which will set the app initiated value on the request
> > in FrameLoader::updateRequestAndAddExtraFields based on the main frame
> > document loader's value. Ditto for FrameLoader::loadFrameRequest() in the
> > contextMenuItem case.
> > 
> 
> I believe that NavigationScheduler does have one case that uses:
> 
>     frame.loader().load(WTFMove(frameLoadRequest));
> 
> It looks like FrameLoader::load(FrameLoadRequest&& request) can bypass
> 'updateRequestAndAddExtraFields' in the case where
> request.shouldCheckNewWindowPolicy() is true, which I think can get through
> the load without setting AppInitiated state.

Oh, good catch.

Weird about the <link> preconnect case causing issues. Does it repro in a reduced test case?

[Kate Cheney]

Comment on attachment 455653 [details]
Patch

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

> Source/WebCore/loader/ThreadableLoader.cpp:117
> +    if (auto* documentLoader = document ? document->loader() : nullptr)

Was this a place you were catching errors? A test case would be nice. Or if it is speculative maybe we should add this to WorkerThreadableLoader as well?

[Brent Fulgham]

(In reply to Kate Cheney from comment #8)
> (In reply to Brent Fulgham from comment #6)
> > (In reply to Kate Cheney from comment #3)
> > > Comment on attachment 455296 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=455296&action=review
> > > 
> > > Have you seen these code paths be hit with incorrect app initiated values?
> > > For all of the NavigationScheduler functions we eventually call
> > > frame.changeLocation() which will set the app initiated value on the request
> > > in FrameLoader::updateRequestAndAddExtraFields based on the main frame
> > > document loader's value. Ditto for FrameLoader::loadFrameRequest() in the
> > > contextMenuItem case.
> > > 
> > 
> > I believe that NavigationScheduler does have one case that uses:
> > 
> >     frame.loader().load(WTFMove(frameLoadRequest));
> > 
> > It looks like FrameLoader::load(FrameLoadRequest&& request) can bypass
> > 'updateRequestAndAddExtraFields' in the case where
> > request.shouldCheckNewWindowPolicy() is true, which I think can get through
> > the load without setting AppInitiated state.
> 
> Oh, good catch.
> 
> Weird about the <link> preconnect case causing issues. Does it repro in a
> reduced test case?

I need to try to build one. I've been testing with an iOS Simulator build, and I can't get those two cases to trigger a mismatch, so perhaps something else is going on.

Another possibility is that the Cache loader doesn't handle AppInitiated state, and so the first load that goes to Cache gets flagged as App-Initiated?

[Brent Fulgham]

Comment on attachment 455653 [details]
Patch

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

>> Source/WebCore/loader/ThreadableLoader.cpp:117
>> +    if (auto* documentLoader = document ? document->loader() : nullptr)
> 
> Was this a place you were catching errors? A test case would be nice. Or if it is speculative maybe we should add this to WorkerThreadableLoader as well?

Yes, this was a case that I encountered mismatches, but I'm not sure if lower-level loading code might correct it. At a minimum, I should probably move this above the WorkerThreadableLoader::create so that if the context has a document we can properly reflect it's AppInitiated state.

[Brent Fulgham]

Created attachment 456058 [details]
Patch

[Brent Fulgham]

Created attachment 456144 [details]
Patch

[Kate Cheney]

Comment on attachment 456144 [details]
Patch

r=me

[Darin Adler]

Comment on attachment 456144 [details]
Patch

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

> Source/WebCore/loader/NavigationScheduler.cpp:385
> +        if (auto* documentLoader = m_originDocument.loader())
> +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Is it really OK to leave this set to the default if there is no document loader associated? Does that mean it gets set to true or false? Which is the correct value for that case?

> Source/WebCore/loader/ThreadableLoader.cpp:115
> +    if (auto* documentLoader = document ? document->loader() : nullptr)
> +        request.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());

Is it really OK to leave this set to the default if there is no document loader associated? Does that mean it gets set to true or false? Which is the correct value for that case?

> Source/WebCore/page/DragController.cpp:298
> +    resourceRequest.setIsAppInitiated(false);

Why is this needed? Isn’t false the default?

[EWS]

Committed r292139 (249046@main): <https://commits.webkit.org/249046@main>

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

[Kate Cheney]

(In reply to Darin Adler from comment #15)
> Comment on attachment 456144 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456144&action=review
> 
> > Source/WebCore/loader/NavigationScheduler.cpp:385
> > +        if (auto* documentLoader = m_originDocument.loader())
> > +            resourceRequest.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());
> 
> Is it really OK to leave this set to the default if there is no document
> loader associated? Does that mean it gets set to true or false? Which is the
> correct value for that case?
> 
> > Source/WebCore/loader/ThreadableLoader.cpp:115
> > +    if (auto* documentLoader = document ? document->loader() : nullptr)
> > +        request.setIsAppInitiated(documentLoader->lastNavigationWasAppInitiated());
> 
> Is it really OK to leave this set to the default if there is no document
> loader associated? Does that mean it gets set to true or false? Which is the
> correct value for that case?
> 
> > Source/WebCore/page/DragController.cpp:298
> > +    resourceRequest.setIsAppInitiated(false);
> 
> Why is this needed? Isn’t false the default?

The default for a request's attribution value is to attribute to the app/developer (https://developer.apple.com/documentation/foundation/nsurlrequest/attribution) which equates to app-initiated being "true".

I am not sure of the full extent of cases where we create a request without a document loader, which is where we store the attribution value from the initial request. Maybe an investigation into that would be useful. For now, in the case where we are unsure of the attribution value because the document loader is null, I think it makes sense to keep the default value.

[Brent Fulgham]

Comment on attachment 456144 [details]
Patch

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

>> Source/WebCore/page/DragController.cpp:298
>> +    resourceRequest.setIsAppInitiated(false);
> 
> Why is this needed? Isn’t false the default?

The default value is app initiated; we have to set to non-app-initiated if we have reason to believe a user triggered the load. In this case, since the request is happening because of a user drag operation, we can set to "non-app-initiated".