Bug 91685 - [chromium] Some SubstituteData loads broken after r121912
Summary: [chromium] Some SubstituteData loads broken after r121912
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-07-18 15:49 PDT by Nate Chapin
Modified: 2012-09-05 21:48 PDT (History)
5 users (show)

See Also:


Attachments
patch (1.92 KB, patch)
2012-07-18 16:03 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (555.65 KB, application/zip)
2012-07-18 17:16 PDT, WebKit Review Bot
no flags Details
patch + chromium unit test (5.69 KB, patch)
2012-08-07 11:13 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (5.78 KB, patch)
2012-09-05 11:24 PDT, 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-07-18 15:49:31 PDT
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.
Comment 1 Nate Chapin 2012-07-18 16:03:41 PDT
Created attachment 153124 [details]
patch
Comment 2 Brady Eidson 2012-07-18 16:13:38 PDT
Comment on attachment 153124 [details]
patch

Can we add a layout test for this so we don't re-break it?
Comment 3 Nate Chapin 2012-07-18 16:22:28 PDT
(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.
Comment 4 WebKit Review Bot 2012-07-18 17:16:32 PDT
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
Comment 5 WebKit Review Bot 2012-07-18 17:16:35 PDT
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
Comment 6 Nate Chapin 2012-07-23 14:22:44 PDT
(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.
Comment 7 Nate Chapin 2012-08-07 11:13:01 PDT
Created attachment 156960 [details]
patch + chromium unit test

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

Sorry for how long this took.
Comment 8 Adam Barth 2012-09-05 11:03:56 PDT
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!
Comment 9 Nate Chapin 2012-09-05 11:24:57 PDT
Created attachment 162295 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-09-05 21:47:57 PDT
Comment on attachment 162295 [details]
Patch for landing

Clearing flags on attachment: 162295

Committed r127690: <http://trac.webkit.org/changeset/127690>
Comment 11 WebKit Review Bot 2012-09-05 21:48:01 PDT
All reviewed patches have been landed.  Closing bug.