Bug 31822 – [Chromium] Renderer hang when using www.expedia.com

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

[Chromium] Renderer hang when using www.expedia.com

Originally reported here:
http://code.google.com/p/chromium/issues/detail?id=26446

1. Go to http://www.expedia.com/.
2. Enter 'to' and 'from' and dates details and click on 'search for flights'.
3. Wait till flights list appears.
4. Select any flight and continue.
5. Press back button and scroll flights list.

Observe the process running WebKit is hung.  Upon further inspection, it is stuck in a loop making synchronous XMLHttpRequests.  The sync XHRs always fail because the XHR is loaded with a cache policy of ReturnCacheDataDontLoad and the cache no longer has the associated data.

Safari does not have this bug.

More details here:
http://code.google.com/p/chromium/issues/detail?id=26446#c17

[Darin Fisher (:fishd, Google)]

Created attachment 43754 [details]
v1 patch

[Eric Seidel (no email)]

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.

[Adam Barth]

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.

[Darin Fisher (:fishd, Google)]

Created attachment 43788 [details]
v2 patch: with FrameLoader::originalRequest [aka DocumentLoader::originalRequestCopy]

[Alexey Proskuryakov]

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.

[Brady Eidson]

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.

[Darin Fisher (:fishd, Google)]

> 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').

[Adam Barth]

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.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/51350