Bug 43328

Would it be possible for you to create a test account on this site? Not knowing the language, I couldn't find out how to register.

(In reply to comment #1) > Would it be possible for you to create a test account on this site? Not knowing the language, I couldn't find out how to register. Please use this account: (In the first input box, the account name): webkit (In the second input box, the password): web123kit Under the input boxes, the button on the left is the login button. After clicking it, if you see "http://mail.139.com/error/systemTip2.html?aspxerrorpath=/Login.ashx" in the address bar, this is the bug. Otherwise the URL is like "http://wmsvr1.mail.10086.cn/c/XTS/default.jsp?funcid=main&sid=MAZKFpkkvWkRzWZrwvkkcLhiETGqCHaT". (Note: mail.139.com is a mail service provided by China Mobile for its users. The above account is bound to my personal phone. However, I have never actually used the mail box, so feel free to do any test with it.) I just finished the binary search for the revision which caused the issue: r61285 (on Mac) (http://trac.webkit.org/changeset/61285: Enable HTML5 Parser in Safari on Mac). Because HTML5 parser was enabled at different revisions for different platforms, the statement in the original bug report "(after SVN revision 61351, platform: Windows, Linux, Mac) might be inaccurate. I'm still investigating why HTML5 parser causes this issue.

Created attachment 63298 [details] HTTP request/response log Some updates: I modified inspector to let it show HTTP request/response log. The screen shot of the log (after clicking the login button) is attached. From the log we can see that the form submission URL was requested twice. The server responded error for the second request. I used curl to simulate the form submission, the response was: curl --data "UserName=webkit&Password=web123kit&VerifyCode=" "https://mail.10086.cn/Login/Login.ashx" <script>window.location.href='http://wmsvr1.mail.10086.cn/c/XTS/default.jsp?funcid=main&sid=BAsIPoOOVFnNZnCgSzOONObGeOGLBpVl';</script><META HTTP-EQUIV=REFRESH CONTENT='0';URL='http://wmsvr1.mail.10086.cn/c/XTS/default.jsp?funcid=main&sid=BAsIPoOOVFnNZnCgSzOONObGeOGLBpVl'> The response is a redirection, and the target URL is correct. With HTML5 parser disabled, the HTTP request/response log is much shorter. The browser just followed the redirected URL and show the correct page.

The root cause is in the form submission action page: <META HTTP-EQUIV=REFRESH CONTENT='0';URL='http://wmsvr1.mail.10086.cn/c/XTS/default.jsp?funcid=main&sid=BAsIPoOOVFnNZnCgSzOONObGeOGLBpVl'> There are extra quotation marks, so the URL part of the refresh content is ignored by our HTML5 parser. This should cause redirection to itself, and the server returns an error page when it finds the self-redirection. Because this works in all other browsers, and in webkit browser before HTML5 parser enabled, I think we should fix this to improve compatibility (though the importance should be low.) BTW, I'd suggest to add redirection tracing features into the developer tool, which will save much time debugging such problems. I'll ask the developer tool team for this.

I'm not sure our parsing of that meta tag changed when we enabled the HTML5 tokenizer. Is it possible something else is going on? For example, I just tried the following document in Safari 5: <META HTTP-EQUIV=REFRESH CONTENT='0';URL='http://www.google.com/'> Instead of being redirected to Google, I was redirected to the same page.

> I'm not sure our parsing of that meta tag changed when we enabled the HTML5 tokenizer. Is it possible something else is going on? > > For example, I just tried the following document in Safari 5: > > <META HTTP-EQUIV=REFRESH CONTENT='0';URL='http://www.google.com/'> > > Instead of being redirected to Google, I was redirected to the same page. Sorry, I made incorrect conclusion. You're right. The root cause should be the priority of <script> and <meta refresh>. Before HTML5 and in other browsers, <script> is executed before handling <meta refresh>.

Created attachment 63299 [details] test case.

Oh, that's very interesting. Thanks for the report. Either Eric or I will investigate in the next couple of days.

(In reply to comment #6) > Oh, that's very interesting. Thanks for the report. Either Eric or I will investigate in the next couple of days. Sorry again. The execution sequence is just a guess and not confirmed. However, Johnny told me that the window.location change is asynchronous which might be the real root cause.(In reply to comment #8)

> Sorry again. The execution sequence is just a guess and not confirmed. They're both asynchronous, but they're going to the same task queue, so they should come out in the same order. The race might be if the first navigation gets far enough along not to be cancelled by the second navigation. In any case, I tried your test case in Safari 5, Firefox 3.6, and the Chrome Dev channel. In Safari 5 and Firefox 3.6, the script won. In the Dev channel, the meta-refresh won. We should at least understand why our behavior changed and hopefully make this case work like it did before.

I'm looking at this now.

I've confirmed using nightlies that the regression range is r61173-r61351. I'm going to git-bisect now.

I had to write a layout test in order to run git bisect. In case I somehow forget to post a patch, here is the test: % cat test.html <script> if (window.layoutTestController) layoutTestController.waitUntilDone(); window.location.href = 'success.html'; </script> <META HTTP-EQUIV=REFRESH CONTENT='0';URL='success.html'> % cat success.html <script> document.write('PASS'); if (window.layoutTestController) { layoutTestController.dumpAsText(); layoutTestController.notifyDone(); } </script> It times out if it fails. I could make it more self-documenting.

Confirmed this regressed when the HTML5 lexer was turned on. r61234. The DOM looks the same before and after the regression, so I think it must be related to script execution changing. Investigating.

Isn't it just the malformed META? It should have CONTENT='0;URL=success.html'.

The test should just be changed to: <script>window.location.href="success.html"</script> <meta http-equiv="refresh" content="0"> I'm just not sure how the old parser escaped infinite looping there before.

(In reply to comment #15) > Isn't it just the malformed META? It should have CONTENT='0;URL=success.html'. The meta is definitely malformed, yes. But how we avoided infinite looping with meta content=0 before, I don't know. Firefox doesn't infinite loop on the test, so it seems we shouldn't either.

Um. Looks like the old parser explicitly yielded in the case where a location change was pending: while (!m_src.isEmpty() && (!frame || !frame->redirectScheduler()->locationChangePending())) { http://trac.webkit.org/browser/trunk/WebCore/html/HTMLDocumentParser.cpp?rev=61233#L1779 Sad there was never any testing of this code. :( Also, I'm not sure how the parser was sure to resume after such a yield? It's possible the network had already given us all bytes and no one is ever going to call us back to finish pumping those...

Turns out this is old code. It was added to the old parser back in: http://trac.webkit.org/changeset/7263 and modified to only apply to <meta> in http://trac.webkit.org/changeset/7800 (I'm not sure that's actually still how it works. Neither of those changes had automated tests, sadly.

The old parser appears to simply abort parsing when a redirect is scheduled. I'm not sure that's correct. If the redirect triggers a download for instance (and thus doesn't redirect), I assume we're supposed to continue parsing. The old parser would have eventually continued parsing if reading from the network, but I don't think it would have done the right thing in the document.write() case or in the "final pump during end()" case. I need to figure out what the right behavior in both of those cases is before I can really fix this in the new parser. Opinions welcome. :)

Turns out at least FF makes the parser stop. Test case: <script> if (window.layoutTestController) { layoutTestController.waitUntilDone(); layoutTestController.dumpAsText(); } // This redirect, although it should have priority, will turn into // a download and "fail", thus parsing should continue, right? Wrong. window.location.href = 'resources/binary_data.exe'; </script> FAIL, parsing didn't stop after the redirect.

<rdar://problem/8415151>

Created attachment 67447 [details] Patch

Comment on attachment 67447 [details] Patch > + // Some web pages assume that scheduled location changes abort parsing. > + // The old HTML parser emulated this by polling the redirect scheduler > + // every loop to see if it had scheduled a location change. Instead we're > + // stopping parsing if a location change is ever scheduled. > + // FIXME: It's unclear if this is the right layer for this hack. It is > + // possible that not all scheduled location changes should abort parsing. > + if (DocumentParser* parser = m_frame->document()->parser()) > + parser->stopParsing(); What makes this a hack? Is this something that should be documented in the HTML specification?

(In reply to comment #24) > (From update of attachment 67447 [details]) > > + // Some web pages assume that scheduled location changes abort parsing. > > + // The old HTML parser emulated this by polling the redirect scheduler > > + // every loop to see if it had scheduled a location change. Instead we're > > + // stopping parsing if a location change is ever scheduled. > > + // FIXME: It's unclear if this is the right layer for this hack. It is > > + // possible that not all scheduled location changes should abort parsing. > > + if (DocumentParser* parser = m_frame->document()->parser()) > > + parser->stopParsing(); > > What makes this a hack? The hack part is due entirely to lack of specification. There are at least 5 different places I could have conceivably put this code, each of which could have different behaviors exposed to the web. For example: 1. in JSLocationCustom::setHref -- would work here, but wouldn't cover other possible location changes. 2. Before or after security checks in JSLocationCustom? 3. Before or after null checks in JSLocationCustom? 4. Inside the constructor to ScheduledLocation change? 5. Inside the HTMLDocumentParser loop (this is how the old parser did it, but it didn't also check in end(), so it would have been possible in the old parser to restart parsing if the redirect failed and later bytes came off the network. This seemed unintentional.) > Is this something that should be documented in the HTML specification? Yes. I'm just not sure what the behavior should be. This patch attempted to be simple, but making all scheduled location changes (from any source) abort parsing.

I don’t mean to quibble semantics, but I think we are overusing the word hack here. We need to add this behavior for compatibility and it’s not yet covered in the HTML specification=. But that doesn’t make it a “hack” in my opinion.

I'm happy to remove the word "hack". Will do so after lunch. http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#the-location-interface Does not seem to cover this behavior. Nor does http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigate. HTML5 navigation stops parsing when unloading the document, however setting location.href does not need to unload the document to stop parsing it seems. I wrote a test before (which I'm happy to add back) which sets the location.href to a download (which shouldn't cause the document to unload), yet that still stops parsing.

Filed a bug with HTML5: http://www.w3.org/Bugs/Public/show_bug.cgi?id=10625

Created attachment 67452 [details] Patch

Comment on attachment 67452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67452&action=prettypatch > WebCore/html/parser/HTMLDocumentParser.cpp:248 > + // We should have aborted above if a location change was ever scheduled. > + // If we hit this ASSERT, we're likely parsing when we shouldn't be. See: > + // https://bugs.webkit.org/show_bug.cgi?id=43328 > + ASSERT(frame()->redirectScheduler()->locationChangePending()); Presumably we need a "not" here, right? > WebCore/loader/RedirectScheduler.cpp:304 > + // every loop to see if it had scheduled a location change. Instead we're One space after "." ;)

Comment on attachment 67452 [details] Patch New patch coming.

Created attachment 67465 [details] Patch for landing

Comment on attachment 67465 [details] Patch for landing Rejecting patch 67465 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20987 test cases. fast/dom/Geolocation/clear-watch-invalid-id-crash.html -> failed Exiting early after 1 failures. 6639 tests run. 168.67s total testing time 6638 test cases (99%) succeeded 1 test case (<1%) had incorrect layout Full output: http://queues.webkit.org/results/4037008

Comment on attachment 67465 [details] Patch for landing Rejecting patch 67465 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20987 test cases. fast/dom/Geolocation/clear-watch-invalid-id-crash.html -> failed Exiting early after 1 failures. 6639 tests run. 127.90s total testing time 6638 test cases (99%) succeeded 1 test case (<1%) had incorrect layout Full output: http://queues.webkit.org/results/4034013

Looks like http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Geolocation/clear-watch-invalid-id-crash.html was added while this regression was in the tree and depends on wrong behavior.

Comment on attachment 67465 [details] Patch for landing This is not failing for me locally on SL. trying again.

Comment on attachment 67465 [details] Patch for landing nm

My patch appears to be wrong and is having strange failures based on which set of layout tests are run. :(

I think my patch doesn't quite work after TonyG's ParserState patch. Then again, maybe my patch never worked...

> Looks like http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/Geolocation/clear-watch-invalid-id-crash.html > was added while this regression was in the tree and depends on wrong behavior. This test is pretty simple - it just makes a Geolocation request when onload fires, then navigates the page. I don't think it should depend on any of the behaviour discussed in this patch. However, the first version of the test was flaky, but was fixed in http://trac.webkit.org/changeset/67546 on 2010-09-15 - just after the commit bot had problems with it. So it's possible that your change was just unfortunate to tickle the flakiness. Have you tried your change with the new version of the test? Sorry for the trouble.

http://crbug.com/56104 shows a similar issue at: http://developer.apple.com/library/safari/documentation/UserExperience/Reference/SafariExtensionsReference/_index.html Which probably is this same root cause. The current status of this bug is that Ian Hickson is actively investigating what to spec here for HTML5. When he's done, then we can fix WebKit to actually match all browsers.

Ian has specced this now. We can look at fixing our behavior to match the HTML5 spec.

Eric, Adam, is one of you going to fix this or should we look for someone else?

(In reply to comment #43) > Eric, Adam, is one of you going to fix this or should we look for someone else? I believe this is already fixed. I can dig up the revision if you'd like to merge it to a branch. If you can still produce the issue, please re-open this bug.

*** This bug has been marked as a duplicate of bug 45627 ***