Bug 45627 - REGRESSION: Infinite redirect on developer.apple.com
Summary: REGRESSION: Infinite redirect on developer.apple.com
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P1 Normal
Assignee: Adam Barth
URL: http://developer.apple.com/library/ma...
Keywords: InRadar, Regression
: 43328 (view as bug list)
Depends on:
Blocks: 41115 43328
  Show dependency treegraph
 
Reported: 2010-09-12 18:56 PDT by opendarwin
Modified: 2011-01-17 16:02 PST (History)
7 users (show)

See Also:


Attachments
Testcase (132 bytes, text/html)
2010-11-10 13:28 PST, Tony Gentilcore
no flags Details
Patch (12.05 KB, patch)
2010-11-17 14:47 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Move abort() to DocumentLoader (12.38 KB, patch)
2010-11-17 18:01 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch (7.00 KB, patch)
2010-12-10 16:07 PST, Adam Barth
eric: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description opendarwin 2010-09-12 18:56:44 PDT
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.
Comment 1 opendarwin 2010-09-12 20:23:42 PDT
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
Comment 2 Eric Seidel (no email) 2010-09-12 20:25:57 PDT
Sounds like bug 43328.
Comment 3 opendarwin 2010-09-12 21:46:04 PDT
Confirmed by building from source that the bug was introduced in svn r65868.
Comment 4 Eric Seidel (no email) 2010-09-12 22:19:10 PDT
OK.  We need a reduction to make progress with this bug. :)
Comment 5 opendarwin 2010-09-13 05:08:05 PDT
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.
Comment 6 Alexey Proskuryakov 2010-11-03 19:55:42 PDT
<rdar://problem/8629435>
Comment 7 Tony Gentilcore 2010-11-10 13:28:29 PST
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 -->
Comment 8 Tony Gentilcore 2010-11-10 13:42:44 PST
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
Comment 9 Tony Gentilcore 2010-11-15 09:45:12 PST
This is blocking Chrome 8, I'll look into it.
Comment 10 Tony Gentilcore 2010-11-17 14:47:01 PST
Created attachment 74159 [details]
Patch
Comment 11 Tony Gentilcore 2010-11-17 14:50:53 PST
This isn't quite complete. I have a question in the patch about an ASSERT I'm hitting. FYI, all tests are passing.
Comment 12 Darin Adler 2010-11-17 14:56:06 PST
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.
Comment 13 Tony Gentilcore 2010-11-17 18:01:51 PST
Created attachment 74187 [details]
Move abort() to DocumentLoader
Comment 14 Tony Gentilcore 2010-11-17 18:03:04 PST
(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.
Comment 15 Tony Gentilcore 2010-11-18 13:24:50 PST
> 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];
...
}
Comment 16 Eric Seidel (no email) 2010-11-18 13:27:22 PST
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. :)
Comment 17 Tony Gentilcore 2010-11-18 17:21:22 PST
> 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.
Comment 18 Eric Seidel (no email) 2010-11-18 17:25:43 PST
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.
Comment 19 Eric Seidel (no email) 2010-11-18 17:26:24 PST
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.)
Comment 20 Tony Gentilcore 2010-11-18 17:38:31 PST
(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.
Comment 21 Adam Barth 2010-11-22 17:51:39 PST
I landed the cleanup part of this patch for Tony in http://trac.webkit.org/changeset/72578
Comment 22 Adam Barth 2010-11-22 18:00:03 PST
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.
Comment 23 Adam Barth 2010-11-24 12:11:30 PST
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.
Comment 24 Adam Barth 2010-11-24 13:17:06 PST
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
Comment 25 Adam Barth 2010-11-24 13:30:33 PST
I should say that they're mostly timeouts.
Comment 26 Adam Barth 2010-11-24 15:37:30 PST
I take it back.  Something's wrong with my machine.
Comment 27 Adam Barth 2010-12-10 16:07:40 PST
Created attachment 76279 [details]
Patch
Comment 28 Eric Seidel (no email) 2010-12-10 16:10:14 PST
Comment on attachment 76279 [details]
Patch

This is better than being broken.  Thanks.
Comment 29 Adam Barth 2010-12-10 16:13:00 PST
Committed r73817: <http://trac.webkit.org/changeset/73817>
Comment 30 opendarwin 2010-12-10 19:43:20 PST
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.)
Comment 31 Eric Seidel (no email) 2010-12-13 01:57:32 PST
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?
Comment 32 Eric Seidel (no email) 2010-12-13 01:58:00 PST
I should clarify.  The regression to acid3 results is only on mac leopard (debug).
Comment 33 Adam Barth 2010-12-13 17:45:52 PST
Sheriffbot failed me!  I don't have a Leopard machine to test with....
Comment 34 Tony Gentilcore 2010-12-21 14:37:37 PST
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.
Comment 35 Adam Barth 2010-12-21 14:39:36 PST
(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).  :)
Comment 36 Adam Barth 2011-01-17 16:02:28 PST
*** Bug 43328 has been marked as a duplicate of this bug. ***