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.
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
Sounds like bug 43328.
Confirmed by building from source that the bug was introduced in svn r65868.
OK. We need a reduction to make progress with this bug. :)
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.
<rdar://problem/8629435>
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 -->
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
This is blocking Chrome 8, I'll look into it.
Created attachment 74159 [details] Patch
This isn't quite complete. I have a question in the patch about an ASSERT I'm hitting. FYI, all tests are passing.
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.
Created attachment 74187 [details] Move abort() to DocumentLoader
(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.
> 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]; ... }
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. :)
> 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.
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.
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.)
(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.
I landed the cleanup part of this patch for Tony in http://trac.webkit.org/changeset/72578
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.
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 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
I should say that they're mostly timeouts.
I take it back. Something's wrong with my machine.
Created attachment 76279 [details] Patch
Comment on attachment 76279 [details] Patch This is better than being broken. Thanks.
Committed r73817: <http://trac.webkit.org/changeset/73817>
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.)
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?
I should clarify. The regression to acid3 results is only on mac leopard (debug).
Sheriffbot failed me! I don't have a Leopard machine to test with....
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.
(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). :)
*** Bug 43328 has been marked as a duplicate of this bug. ***