Bug 91685 – [chromium] Some SubstituteData loads broken after r121912

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

[Nate Chapin]

Original report: http://crbug.com/136310

To reproduce with chromium canary: Go to an error page (Like http://0.0.0.0). Then use the arrows to navigate back to another page, and then back to the error page.  The error page is not re-displayed.

This is because WebFrameImpl::loadData() sometimes calls FrameLoader::setReplacing() after starting a SubstituteData load. r121912 mistakenly assumes that setReplacing() is only called for multipart loads (the call in loadData() is in fact the only other time it is called). Checking MainResourceLoader::isLoadingMultipartContent() before checking FrameLoader::isReplacing() in DocumentLoader::commitData() should fix the problem.

[Nate Chapin]

Created attachment 153124 [details]
patch

[Brady Eidson]

Comment on attachment 153124 [details]
patch

Can we add a layout test for this so we don't re-break it?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 153124 [details])
> Can we add a layout test for this so we don't re-break it?

I'll see what I can do.

[WebKit Review Bot]

Comment on attachment 153124 [details]
patch

Attachment 153124 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13272681

New failing tests:
fast/loader/document-with-fragment-url-1.html

[WebKit Review Bot]

Created attachment 153142 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Nate Chapin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 153124 [details] [details])
> > Can we add a layout test for this so we don't re-break it?
> 
> I'll see what I can do.

OK, I don't think I can write a layout test for this particular case, because it depends on a chromium quirk (the idea of calling FrameLoader::setReplacing() for a SubstituteData load), and that quirk is not reachable from a code path that DRT can exercise.

I'm looking into writing a chromium webkit unit test, but there are gaps in the infrastructure that need to be fixed first, so it may take a while to get everything in order.

[Nate Chapin]

Created attachment 156960 [details]
patch + chromium unit test

Chromium infrastructure landed downstream, WebKit/chromium/DEPS now includes it.

Sorry for how long this took.

[Adam Barth]

Comment on attachment 156960 [details]
patch + chromium unit test

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

I'm not 100% confident in this review, but the explanation in the ChangeLog makes sense.

> Source/WebCore/ChangeLog:8
> +        No new tests, regressed cases covered by chromium.org's browser_tests.

No new tests <--- There is a test now!

[Nate Chapin]

Created attachment 162295 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162295 [details]
Patch for landing

Clearing flags on attachment: 162295

Committed r127690: <http://trac.webkit.org/changeset/127690>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.