Bug 109995 - LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser
Summary: LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on: 110401
Blocks: 106127
  Show dependency treegraph
 
Reported: 2013-02-15 17:51 PST by Adam Barth
Modified: 2013-02-21 21:52 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.82 KB, patch)
2013-02-21 15:52 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (7.08 KB, patch)
2013-02-21 16:19 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2013-02-15 17:51:49 PST
These tests apparently also timeout in debug builds, which is making it difficult to debug what's going on.  :)
Comment 1 Adam Barth 2013-02-15 17:52:43 PST
The issue appears to be that TestRunner's m_topLoadingFrame isn't cleared, which results in the call to testRunner.notifyDone to be ignored.
Comment 2 Eric Seidel (no email) 2013-02-15 17:52:58 PST
DumpRenderTree --no-timeout?

I remebmer looking at these, it was the removal of a not-yet-fully loaded iframe which was causing the load event to never fire.  At least that was before your load event fixes.  WE have an email thread on the subject.
Comment 3 Eric Seidel (no email) 2013-02-15 17:53:37 PST
sorry, irrc, the iframe was added and removed during onload, which caused the trouble.  again in our email somewhere.
Comment 4 Adam Barth 2013-02-15 17:58:35 PST
Interesting.  In the Release case, there's a call to didFailLoad which clears the top loading frame.

It seems like there should be a call to didFinishLoad instead indicating that the main frame actually finished loading.
Comment 5 Adam Barth 2013-02-15 18:06:22 PST
== Reduction ==

<iframe id="test"></iframe>
<script>
testRunner.dumpAsText();
testRunner.waitUntilDone();

var frame = document.getElementById('test');

window.onload = function () {
    frame.src = 'resources/010.html';
}

window.foo = function () {
    frame.remove();
    testRunner.notifyDone();
};
</script>

... where 010.html is:

<script>parent.foo()</script>
Comment 6 Adam Barth 2013-02-15 18:08:45 PST
Good news: the reduction works in Debug as well, which means we actually use a real debugger.  :)
Comment 7 Adam Barth 2013-02-15 18:15:19 PST
The call to didFailLoad is coming from the call to FrameLoader::checkLoadComplete inside the FrameLoader::frameDetached call.
Comment 8 Eric Seidel (no email) 2013-02-20 15:10:07 PST
We now understand what's going on here.

Previously DocumentLoader::stopLoading() would think it was still loading (because it was on the stack and thus still had a main resource loader), and thus send an error callback to the embedder when removing the iframe.

In the threaded case, the DocumentLoader believes the load is complete by the time the removeChild call is made on the iframe, and stopLoading() has code to guard against being called twice, if DocumentLoader::loading() is already false.

Our current theory is that we might make DocumentLoader::loading() notice that the parser is still active and return true until it's done.  This will require the parser to call checkCompleted() when it's done, but it already calls FrameLoader::checkCompleted() and could be made to call DocumentLoader::checkCompleted() as well if necessary.
Comment 9 Eric Seidel (no email) 2013-02-20 16:37:43 PST
bug 110401 may fix this.
Comment 10 Eric Seidel (no email) 2013-02-20 17:01:46 PST
I was just talking with Tony, and I worry that this bug could be confusing our DOMContentLoaded number.

I'm not exactly sure where our DCL number is taken from, but I worry that it could not be including all parse time based on our debugging here.
Comment 11 Adam Barth 2013-02-20 17:56:37 PST
It's good to be skeptical, but DOMContentLoaded should be triggered by the parser after processing EOF.  In any case, we need to fix the correctness issues.  :)
Comment 12 Eric Seidel (no email) 2013-02-21 15:52:43 PST
Created attachment 189623 [details]
Patch
Comment 13 Adam Barth 2013-02-21 15:54:07 PST
Comment on attachment 189623 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:283
> +    if (m_frame->document() && m_frame->document()->hasActiveParser())

You don't need to call checkComplete on the DocumentLoader when the last parser finishes?
Comment 14 Eric Seidel (no email) 2013-02-21 16:19:37 PST
Created attachment 189630 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-02-21 16:41:20 PST
Comment on attachment 189630 [details]
Patch for landing

Clearing flags on attachment: 189630

Committed r143664: <http://trac.webkit.org/changeset/143664>
Comment 16 WebKit Review Bot 2013-02-21 16:41:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Benjamin Poulain 2013-02-21 17:38:06 PST
Looks like this broke the Mac build on all bots.
Comment 18 Eric Seidel (no email) 2013-02-21 17:39:17 PST
Shucks.  Too bad the mac ews took over 24 hours to process the patch or I might have noticed. :p

Investigating.
Comment 19 Eric Seidel (no email) 2013-02-21 17:42:24 PST
(In reply to comment #18)
> Shucks.  Too bad the mac ews took over 24 hours to process the patch or I might have noticed. :p
> 
> Investigating.

I should be clear, my snarky remark was about the patch when it was attached to bug 110401. :)

Anyway, Benjiman says he has a fix ready to land.  Thank you!  And thanks for letting me know.

Hopefully this won't cause any test failures for Mac.  I would not expect it to, but anything is possible.
Comment 20 Benjamin Poulain 2013-02-21 18:34:00 PST
Most tests crash on WebKit1.

Here is a backtrace (all of the one I have looked have the same backtrace): https://gist.github.com/BenjaminPoulain/5010267
Comment 21 Adam Barth 2013-02-21 18:38:56 PST
Looks like a missing null check.  Perhaps we should roll out?
Comment 22 Adam Barth 2013-02-21 18:40:28 PST
We have the check for document but not for frame.
Comment 23 Eric Seidel (no email) 2013-02-21 18:49:10 PST
We can easily roll out, or I'll try a speculative fix for wk1.
Comment 24 Eric Seidel (no email) 2013-02-21 18:56:21 PST
Speculative fix landed as:
Committed r143681: <http://trac.webkit.org/changeset/143681>
Comment 25 Ryosuke Niwa 2013-02-21 21:52:02 PST
This patch caused http/tests/security/feed-urls-from-remote.html to fail on Mac WK1:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fsecurity%2Ffeed-urls-from-remote.html