Bug 48476 – ResourceRequest targetType set to Subresource for main frame and subframe loads

• Status: RESOLVED
• Resolution: INVALID
• Component: Page Loading

[Joe Mason]

ResourceRequestBase::targetType was added for the Chrome port and moved into cross-platform WebCore in svn 51859.

But, it is only initialized to a resource type in PingLoader::loadImage, PingLoader::sendPing, WorkerScriptLoader::createResourceRequest, and cachedResourceTypeToTargetType in loader.cpp - all of which only deal with subresources.  For ResourceRequest's created to do top-level loads (into a main frame or subframe), it's left as the default of TargetIsSubresource.  This isn't just underspecified, it's clearly wrong - target type should be TargetIsMainFrame or TargetIsSubframe.

As far as I know nobody actually checks targetType for non-subresources at the moment, so this is not critical.  But this information would be useful or clients to have, for instance in dispatchDecidePolicyForNavigationAction to make different decisions when loading a main frame vs loading a subframe.

The attached patch sets the targetType whenever a ResourceRequest is created.  No tests yet since I'd like to get a second opinion on whether this is the right approach before spending more time getting it ready for submission.

[Joe Mason]

Created attachment 72095 [details]
initial patch: looking for comments

[Adam Barth]

Comment on attachment 72095 [details]
initial patch: looking for comments

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

> WebCore/loader/FrameLoader.cpp:178
> +    Page* page = frame->page();
> +    if (page && frame == page->mainFrame())

I'm not sure this is the right way to test this condition.  Maybe try frame->tree()->parent() is null?  You'll have to test what happens with a detached frame.

[Alexey Proskuryakov]

I don't think we should be accepting any changes to ResourceRequest::m_targetType other than the change that will remove it for good.

[Adam Barth]

> I don't think we should be accepting any changes to ResourceRequest::m_targetType other than the change that will remove it for good.

What's involved in removing it?  (My box is busy running a test suite.)

[Joe Mason]

(In reply to comment #4)
> > I don't think we should be accepting any changes to ResourceRequest::m_targetType other than the change that will remove it for good.
> 
> What's involved in removing it?  (My box is busy running a test suite.)

Basically revert svn commit 51859, which moved it out of chromium into cross-platform code, and fix any build failures that result due to later patches making use of it.

The Blackberry port which we're starting to upstream makes use of it, though, so if you get rid of it we'll need to duplicate it in the platform layer.

[Alexey Proskuryakov]

I've filed bug 48483 to have a place of its own for that discussion.

[Joe Mason]

If this patch is too intrusive, until we resolve bug 48483 I suggest adding a TargetIsUnknown type and defaulting to that instead of TargetIsSubresource.  That way at least for toplevel resources it will be unset instead of wrong.

[Adam Barth]

It's not the intrusiveness we're worried about.  We need to sort out the overall direction for this part of the code before we can figure out what to do in this particular case.

[Joe Mason]

I think this is obsoleted by the removal of target type in bug 48483.

(In reply to comment #9)
> I think this is obsoleted by the removal of target type in bug 48483.

I intend to move the targetType to the right layer. While it's temporarily only available on the chromium port, I hope to resurface it once we've figured out how to do this without a layering violation.