Bug 104721 – CachedResources should hang on to stripped fragment identifiers

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

[Nate Chapin]

ResourceRequests for main resource loads typically exit WebCore with the fragment identifier on the request url. Some platforms (at least chromium, probably others) depends on this. With main resources going through the cache (and therefore having fragment identifiers removed), we should figure out a way to plumb the fragment identifier through to the network layer without needing to reach into the DocumentLoader from ResourceLoader.

[Alexey Proskuryakov]

Is this what blocks re-landing of the fix for bug 49246?

[Nate Chapin]

(In reply to comment #1)
> Is this what blocks re-landing of the fix for bug 49246?

It's part of it. I also am trying to work through a report of an assertion being hit in MainResourceLoader::load().

[Alexey Proskuryakov]

Is there a bug tracking that assertion?

[Nate Chapin]

(In reply to comment #3)
> Is there a bug tracking that assertion?

The assertion was added as part of bug 49246 and was removed with the rollout, so I was assuming a bug wasn't necessary. Is that incorrect?

[Alexey Proskuryakov]

https://codereview.chromium.org/11548010/ mentions an assertion failure in MainResourceLoader::load(), but that sounds like the same issue with fragments:

ASSERTION FAILED: equalIgnoringFragmentIdentifier(initialRequest.url(), request.url())

[Nate Chapin]

Created attachment 179111 [details]
patch

[Nate Chapin]

(In reply to comment #5)
> https://codereview.chromium.org/11548010/ mentions an assertion failure in MainResourceLoader::load(), but that sounds like the same issue with fragments:
> 
> ASSERTION FAILED: equalIgnoringFragmentIdentifier(initialRequest.url(), request.url())

The more I think about it, the more I realize that assertion was wrong on multiple levels.

1. it's asserting that FrameLoaderClient::willSendRequest() won't change the url unless it blanks it.  I don't think this is a hard guarantee.

2. It's allowing for the possibility of sending the main resource request without the fragment identifier, which is a change in behavior that ports may depend on.

[Maciej Stachowiak]

(In reply to comment #7)
> (In reply to comment #5)
> > https://codereview.chromium.org/11548010/ mentions an assertion failure in MainResourceLoader::load(), but that sounds like the same issue with fragments:
> > 
> > ASSERTION FAILED: equalIgnoringFragmentIdentifier(initialRequest.url(), request.url())
> 
> The more I think about it, the more I realize that assertion was wrong on multiple levels.
> 
> 1. it's asserting that FrameLoaderClient::willSendRequest() won't change the url unless it blanks it.  I don't think this is a hard guarantee.
> 
> 2. It's allowing for the possibility of sending the main resource request without the fragment identifier, which is a change in behavior that ports may depend on.

Now that you describe it, that does seem like a bad assert. willSendRequest callbacks are allowed to replace the request with anything at all.

[Maciej Stachowiak]

Comment on attachment 179111 [details]
patch

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

> Source/WebCore/loader/cache/CachedResource.cpp:307
> +        request.setURL(url);

I suggest clearing m_fragmentIdentifierForRequest here instead of in CachedResource::stop(), to make clear that it's only meant to be used once per given CachedResource (and thus won't be accidentally picked up by future or simultaneous loads of the same resource); and also because it is not actually used between here and the time stop() is called.

> Source/WebCore/loader/cache/CachedResource.cpp:448
> +    m_fragmentIdentifierForRequest = String();

See note above.

[Maciej Stachowiak]

I suggest also adding a FIXME, if it is plausible to someday wean the network layer off of knowing about fragment IDs. That seems like poor design, because the fragment ID is in theory something that should be processed purely by the layer interpreting the resource (WebCore), not the layer loading it (the network layer) as it should not affect the actual response.

[Nate Chapin]

Created attachment 179116 [details]
clear fragment after use, add FIXME

[Maciej Stachowiak]

Comment on attachment 179116 [details]
clear fragment after use, add FIXME

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

r=me with one optional suggested improvement for the ChangeLog.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:-445
> -    if (request.resourceRequest().url() != url)
> -        request.mutableResourceRequest().setURL(url);

It might be worthwhile to add a note to ChangeLog to explain the deletion of these two lines. With explanation, I can see that what they accomplish is done elsewhere in the patch but it's hard to tell otherwise.

[Nate Chapin]

Created attachment 179280 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 179280 [details]
Patch for landing

Clearing flags on attachment: 179280

Committed r137604: <http://trac.webkit.org/changeset/137604>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.