Summary: | [Chromium] Renderer hang when using www.expedia.com | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Fisher (:fishd, Google) <fishd> | ||||||
Component: | Page Loading | Assignee: | Darin Fisher (:fishd, Google) <fishd> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Major | CC: | abarth, ap, beidson, eric | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Darin Fisher (:fishd, Google)
2009-11-24 00:09:13 PST
Created attachment 43754 [details]
v1 patch
Comment on attachment 43754 [details]
v1 patch
This looks sane to me, but this area of code is tricky enough that we should get someone more familiar with the loader to look at this before I would feel fully comfortable r+ing this.
Comment on attachment 43754 [details]
v1 patch
Talked with fishd via chat. This change conflicts with a comment about how originalRequest is supposed to be used. Similar code in SubResourceLoader::create user originalrequestcopy, so he's going to change to use that instead.
Otherwise this seems fine to me, but this code is so complex I don't have very high confidence.
Created attachment 43788 [details]
v2 patch: with FrameLoader::originalRequest [aka DocumentLoader::originalRequestCopy]
Looks mostly sane to me. I don't understand the change commented as "Avoid repeating a form submission when navigating back or forward" - is this something to be avoided? E.g. Safari asks the user if the form can be re-submitted. CC'ing Brady, who knows the most about this code. Comment on attachment 43788 [details]
v2 patch: with FrameLoader::originalRequest [aka DocumentLoader::originalRequestCopy]
Indeed Safari asks before reposting.
Sadly, I see that the layout test that covers this is currently disabled...
LayoutTests/http/tests/navigation/post-goback-repost-policy.html-disabled
...so I don't know if this change is relevant. But I doubt the sole WebCore change here breaks it.
Since the code/comment about re-posting is in Chromium's own WebKit layer, I suppose it's up to their port to decide what their policy is. Personally I think their policy of throwing out form data on back/forward is probably a bad one, but it's also better than posting-without-prompting.
> Sadly, I see that the layout test that covers this is currently disabled... > LayoutTests/http/tests/navigation/post-goback-repost-policy.html-disabled > ...so I don't know if this change is relevant. But I doubt the sole WebCore > change here breaks it. Interesting. I didn't know about that test. Please note, that I am only changing the WebCore code path for synchronous XHR. I am changing it to more closely match what we do for asynchronous XHR and other subresource loads. > Since the code/comment about re-posting is in Chromium's own WebKit layer, I > suppose it's up to their port to decide what their policy is. Personally I > think their policy of throwing out form data on back/forward is probably a bad > one, but it's also better than posting-without-prompting. I think you misunderstand. We do not throw out form data on back/forward if we can help it. We keep the response from a form submission in our cache, and we try to reuse that when navigating back to a page that resulted from a form submission. In some cases, the cache will not have the data (e.g., if the response included 'cache-control: no-store'). Comment on attachment 43788 [details]
v2 patch: with FrameLoader::originalRequest [aka DocumentLoader::originalRequestCopy]
Thanks for updating the patch. Sounds like we should do this to more closely match the other types of resource loads.
Landed as http://trac.webkit.org/changeset/51350 |