Bug 45627 – REGRESSION: Infinite redirect on developer.apple.com

• Status: VERIFIED
• Resolution: FIXED
• Component: DOM

[Jeff Johnson]

WebKit Debug build from source at svn r67348
Safari 5.0.2 (6533.18.5)
Mac OS X 10.6.4 Build 10F2521

When I load the page http://developer.apple.com/library/mac/navigation/#section=Frameworks&topic=Application%20Kit in Safari, it goes into an infinite redirect. It eventually redirects to http://developer.apple.com/library/mac/index.html#home/ and keeps redirecting to there.

This bug does not occur using stock Mac OS X WebKit.

[Jeff Johnson]

Using the WebKit nightly builds, I found that this bug was introduced between svn r65825 and r66052.

My wild guess is that it may have been http://trac.webkit.org/changeset/65868/trunk

[Eric Seidel (no email)]

Sounds like bug 43328.

[Jeff Johnson]

Confirmed by building from source that the bug was introduced in svn r65868.

[Eric Seidel (no email)]

OK.  We need a reduction to make progress with this bug. :)

[Jeff Johnson]

I'm afraid that may be beyond my ability, but perhaps someone at Apple could enlist the help of Ron Hayden and the documentation team, who would know how that page works better than anyone.

[Alexey Proskuryakov]

<rdar://problem/8629435>

[Tony Gentilcore]

Created attachment 73532 [details]
Testcase

I've reduced the redirect loop down to this:
<script>
window.location.replace("location_replace.html");
</script>
<meta http-equiv="refresh" content="0; URL=meta_refresh.html">

In minefield, the location.replace happens first and the meta refresh is ignored, thus the user lands on location_replace.html. In WebKit, the meta refresh overrides the location replace and the user lands on meta_refresh.html. I'm not sure what the specced behavior is here.

The developer.apple page falls victim to this differences with this line that I don't think the author expected to be reached:
<meta id="refresh" http-equiv="refresh" CONTENT="0; URL=_index.html"><!-- Needed for iPad redirects -->

[Tony Gentilcore]

Eric pointed out this and bug 43328 will be fixed by implementing this update:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=10625

[Tony Gentilcore]

This is blocking Chrome 8, I'll look into it.

[Tony Gentilcore]

Created attachment 74159 [details]
Patch

[Tony Gentilcore]

This isn't quite complete. I have a question in the patch about an ASSERT I'm hitting. FYI, all tests are passing.

[Darin Adler]

Comment on attachment 74159 [details]
Patch

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

> WebCore/dom/Document.cpp:1967
> +// This method is designed to match "Aborting a document load":
> +// http://www.whatwg.org/specs/web-apps/current-work/#abort-a-document
> +void Document::abort()

A function to abort a document load seems like it should be on the document loader, not the document.

[Tony Gentilcore]

Created attachment 74187 [details]
Move abort() to DocumentLoader

[Tony Gentilcore]

(In reply to comment #12)
> A function to abort a document load seems like it should be on the document loader, not the document.
That makes sense. I moved the method. Still having trouble figuring out how to avoid the ASSERT in commitData(), any hints would be appreciated.

[Tony Gentilcore]

> Still having trouble figuring out how to avoid the ASSERT in commitData(), any hints would be appreciated.

The ASSERT only triggers for fast/loader/submit-form-while-parsing-1.xhtml, so I'm trying to figure out what is different about the XMLDocumentParser from the HTMLDocumentParser in this case. The commitData doesn't have any real data, it originates from:

WebHTMLRepresentation.mm:finishedLoadingWithDataSource() {
...
// Telling the frame we received some data and passing nil as the data is our
// way to get work done that is normally done when the first bit of data is
// received, even for the case of a document with no data (like about:blank).
[webFrame _commitData:nil];
...
}

[Eric Seidel (no email)]

That may be hold-over from the old html parser where write("") was commonly used as a way to "spin" the parser.  These days the parser's loop is separate from the write/append calls.

I don't really know what commitData does, so I could be wrong here.  I would try removing it and see what breaks. :)

[Tony Gentilcore]

> I don't really know what commitData does, so I could be wrong here.  I would try removing it and see what breaks. :)

That didn't turn out to be fruitful. Too many other tests fail to call notifyDone without it.

Like I mentioned in http://code.google.com/p/chromium/issues/detail?id=61958, I'll be on vacation for a while so I'm releasing this in case someone is able to get to it in the mean time.

Here's a quick brain dump:
When we schedule a navigation that will change the location, we should hit the current loader with the hardest hammer (cancel all loads, tasks, subframes, etc). window.stop() uses FrameLoader.stopForUserCancel(), so it is probably safe and appropriate. Then we should stop the parser if the hammer didn't already do so. Finally, we need to make sure the unload event fires and the new page loads.

The current patch does all of that, but hits an assert that indicates it isn't canceling the loads hard enough (I believe it only cancels subresources and not the main resource). The flows I've found that cancel the main resource all have unintended side effects that either cancel the redirect, fire errors, or don't fire the unload event.

[Eric Seidel (no email)]

I would argue firing the unload event is secondary and does not need to be solved in this fix.  We can just fix the regression and deal with the unload event (which IE doesn't always fire) in a separate bug.

[Eric Seidel (no email)]

Do you have a suggestion for a path which fixes this all-but the load event firing?  (I'm also on vacation for the next two weeks, getting on a plane shortly.)

[Tony Gentilcore]

(In reply to comment #19)
> Do you have a suggestion for a path which fixes this all-but the load event firing?  

If we don't fire unload, we'd regress on fast/loader/unload-hyperlink.html (and possibly others).

> (I'm also on vacation for the next two weeks, getting on a plane shortly.)

Cool! Maybe Adam or Nate can take a look.

[Adam Barth]

I landed the cleanup part of this patch for Tony in http://trac.webkit.org/changeset/72578

[Adam Barth]

I agree with Eric that we should aim for a small patch that just fixes the regression for the short term.  In the longer term (e.g., when folks are back from vacation) we can do the all-sing, all-dance version.

[Adam Barth]

Apparently I wrote http://trac.webkit.org/browser/trunk/LayoutTests/fast/loader/unload-hyperlink.html.  I certainly didn't intend it to cover the behavior we're tripping over here.  I'll cross-test other browsers.

[Adam Barth]

Comment on attachment 74187 [details]
Move abort() to DocumentLoader

This appears to cause a bunch of test failures:



compositing/video/video-background-color.html	expected	actual	diff	pretty diff
fast/history/history-subframe-with-name.html	expected	actual	diff	pretty diff
fast/parser/no-crash-innerHTML-isindex.html	expected	actual	diff	pretty diff
media/audio-data-url.html	expected	actual	diff	pretty diff
media/audio-mpeg-supported.html	expected	actual	diff	pretty diff
media/video-does-not-loop.html	expected	actual	diff	pretty diff
media/video-source-moved.html	expected	actual	diff	pretty diff
http/tests/media/reload-after-dialog.html	expected	actual	diff	pretty diff
http/tests/media/remove-while-loading.html	expected	actual	diff	pretty diff
http/tests/media/video-cancel-load.html	expected	actual	diff	pretty diff
http/tests/media/video-served-as-text.html	expected	actual	diff	pretty diff

[Adam Barth]

I should say that they're mostly timeouts.

[Adam Barth]

I take it back.  Something's wrong with my machine.

[Adam Barth]

Created attachment 76279 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 76279 [details]
Patch

This is better than being broken.  Thanks.

[Adam Barth]

Committed r73817: <http://trac.webkit.org/changeset/73817>

[Jeff Johnson]

Verified as fixed with svn r73831 built from trunk and Tony's test case.

(Apparently the original developer.apple.com URLs have been rewritten and no longer contain the relevant code.)

[Eric Seidel (no email)]

webkit-patch failure-reason is blaming:

  http://trac.webkit.org/changeset/73815
  http://trac.webkit.org/changeset/73816
  http://trac.webkit.org/changeset/73817

For a regression to http/tests/misc/acid3.html

I feel like this is the only change which has any chance of causing that?

[Eric Seidel (no email)]

I should clarify.  The regression to acid3 results is only on mac leopard (debug).

[Adam Barth]

Sheriffbot failed me!  I don't have a Leopard machine to test with....

[Tony Gentilcore]

I happened to see http://trac.webkit.org/changeset/74426 float by. That fixed the extra didCommit which triggered the ASSERT I was hitting in my original patch. With any luck that might make the more optimal fix easier now.

[Adam Barth]

(In reply to comment #34)
> I happened to see http://trac.webkit.org/changeset/74426 float by. That fixed the extra didCommit which triggered the ASSERT I was hitting in my original patch. With any luck that might make the more optimal fix easier now.

Great.  Your approach is much better (hence the big FIXME in my patch).  :)

[Adam Barth]

*** Bug 43328 has been marked as a duplicate of this bug. ***