Bug 109995 – LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adam Barth]

These tests apparently also timeout in debug builds, which is making it difficult to debug what's going on.  :)

[Adam Barth]

The issue appears to be that TestRunner's m_topLoadingFrame isn't cleared, which results in the call to testRunner.notifyDone to be ignored.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

sorry, irrc, the iframe was added and removed during onload, which caused the trouble.  again in our email somewhere.

[Adam Barth]

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.

[Adam Barth]

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

[Adam Barth]

Good news: the reduction works in Debug as well, which means we actually use a real debugger.  :)

[Adam Barth]

The call to didFailLoad is coming from the call to FrameLoader::checkLoadComplete inside the FrameLoader::frameDetached call.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

bug 110401 may fix this.

[Eric Seidel (no email)]

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.

[Adam Barth]

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

[Eric Seidel (no email)]

Created attachment 189623 [details]
Patch

[Adam Barth]

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?

[Eric Seidel (no email)]

Created attachment 189630 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189630 [details]
Patch for landing

Clearing flags on attachment: 189630

Committed r143664: <http://trac.webkit.org/changeset/143664>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Looks like this broke the Mac build on all bots.

[Eric Seidel (no email)]

Shucks.  Too bad the mac ews took over 24 hours to process the patch or I might have noticed. :p

Investigating.

[Eric Seidel (no email)]

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

[Benjamin Poulain]

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

[Adam Barth]

Looks like a missing null check.  Perhaps we should roll out?

[Adam Barth]

We have the check for document but not for frame.

[Eric Seidel (no email)]

We can easily roll out, or I'll try a speculative fix for wk1.

[Eric Seidel (no email)]

Speculative fix landed as:
Committed r143681: <http://trac.webkit.org/changeset/143681>

[Ryosuke Niwa]

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