Bug 104721 - CachedResources should hang on to stripped fragment identifiers
Summary: CachedResources should hang on to stripped fragment identifiers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-11 14:46 PST by Nate Chapin
Modified: 2012-12-13 09:38 PST (History)
7 users (show)

See Also:


Attachments
patch (4.32 KB, patch)
2012-12-12 13:02 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
clear fragment after use, add FIXME (4.36 KB, patch)
2012-12-12 13:30 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (4.81 KB, patch)
2012-12-13 08:56 PST, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2012-12-11 14:46:13 PST
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.
Comment 1 Alexey Proskuryakov 2012-12-12 10:57:39 PST
Is this what blocks re-landing of the fix for bug 49246?
Comment 2 Nate Chapin 2012-12-12 11:12:42 PST
(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().
Comment 3 Alexey Proskuryakov 2012-12-12 12:36:27 PST
Is there a bug tracking that assertion?
Comment 4 Nate Chapin 2012-12-12 12:40:55 PST
(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?
Comment 5 Alexey Proskuryakov 2012-12-12 12:44:30 PST
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())
Comment 6 Nate Chapin 2012-12-12 13:02:07 PST
Created attachment 179111 [details]
patch
Comment 7 Nate Chapin 2012-12-12 13:05:03 PST
(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.
Comment 8 Maciej Stachowiak 2012-12-12 13:07:24 PST
(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.
Comment 9 Maciej Stachowiak 2012-12-12 13:21:17 PST
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.
Comment 10 Maciej Stachowiak 2012-12-12 13:23:48 PST
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.
Comment 11 Nate Chapin 2012-12-12 13:30:29 PST
Created attachment 179116 [details]
clear fragment after use, add FIXME
Comment 12 Maciej Stachowiak 2012-12-12 13:35:25 PST
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.
Comment 13 Nate Chapin 2012-12-13 08:56:47 PST
Created attachment 179280 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-12-13 09:38:35 PST
Comment on attachment 179280 [details]
Patch for landing

Clearing flags on attachment: 179280

Committed r137604: <http://trac.webkit.org/changeset/137604>
Comment 15 WebKit Review Bot 2012-12-13 09:38:39 PST
All reviewed patches have been landed.  Closing bug.