| Summary: | LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||
| Component: | WebCore Misc. | Assignee: | Eric Seidel (no email) <eric> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, eric, esprehn+autocc, japhet, lforschler, ojan.autocc, rniwa, tonyg, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 110401 | ||||||||
| Bug Blocks: | 106127 | ||||||||
| Attachments: |
|
||||||||
|
Description
Adam Barth
2013-02-15 17:51:49 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. 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. sorry, irrc, the iframe was added and removed during onload, which caused the trouble. again in our email somewhere. 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. == 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>
Good news: the reduction works in Debug as well, which means we actually use a real debugger. :) The call to didFailLoad is coming from the call to FrameLoader::checkLoadComplete inside the FrameLoader::frameDetached call. 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. bug 110401 may fix this. 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. 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. :) Created attachment 189623 [details]
Patch
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? Created attachment 189630 [details]
Patch for landing
Comment on attachment 189630 [details] Patch for landing Clearing flags on attachment: 189630 Committed r143664: <http://trac.webkit.org/changeset/143664> All reviewed patches have been landed. Closing bug. Looks like this broke the Mac build on all bots. Shucks. Too bad the mac ews took over 24 hours to process the patch or I might have noticed. :p Investigating. (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. 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 Looks like a missing null check. Perhaps we should roll out? We have the check for document but not for frame. We can easily roll out, or I'll try a speculative fix for wk1. Speculative fix landed as: Committed r143681: <http://trac.webkit.org/changeset/143681> 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 |