Bug 43328 – REGRESSION(61234): mail.139.com login fails

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[Xianzhu Wang]

Reproducing steps:
1. Open mail.139.com in a webkit browser (after SVN revision 61351, platform: Windows, Linux, Mac);
2. Enter user name and password (if you already have one) and login;

Actual results: It shows an error page

Expected results: Should login normally

Other browsers:
Chrome 6.0.472.x  FAIL
Chrome 5.x OK
Safari 4.x OK
Firefox: OK
IE: OK

I used binary search to find out which revision introduced this problem. Nightly build 61173 is OK, next nightly build 61351 fails.

I'm still investigating this issue.

[Alexey Proskuryakov]

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.

[Xianzhu Wang]

(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.

[Xianzhu Wang]

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.

[Xianzhu Wang]

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.

[Adam Barth]

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.

[Xianzhu Wang]

> 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>.

[Xianzhu Wang]

Created attachment 63299 [details]
test case.

[Adam Barth]

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

[Xianzhu Wang]

(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)

[Adam Barth]

> 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.

[Eric Seidel (no email)]

I'm looking at this now.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Alexey Proskuryakov]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

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...

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

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.

[Andy Estes]

<rdar://problem/8415151>

[Eric Seidel (no email)]

Created attachment 67447 [details]
Patch

[Darin Adler]

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?

[Eric Seidel (no email)]

(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.

[Darin Adler]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 67452 [details]
Patch

[Adam Barth]

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 "."  ;)

[Eric Seidel (no email)]

Comment on attachment 67452 [details]
Patch

New patch coming.

[Eric Seidel (no email)]

Created attachment 67465 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 67465 [details]
Patch for landing

This is not failing for me locally on SL.  trying again.

[Eric Seidel (no email)]

Comment on attachment 67465 [details]
Patch for landing

nm

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Steve Block]

> 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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Darin Adler]

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

[Adam Barth]

(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.

[Adam Barth]

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