WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
109995
LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser
https://bugs.webkit.org/show_bug.cgi?id=109995
Summary
LayoutTests/fast/encoding/parser-tests-*.html timeout with threaded HTML parser
Adam Barth
Reported
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. :)
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Adam Barth
Comment 1
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.
Eric Seidel (no email)
Comment 2
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.
Eric Seidel (no email)
Comment 3
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.
Adam Barth
Comment 4
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.
Adam Barth
Comment 5
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>
Adam Barth
Comment 6
2013-02-15 18:08:45 PST
Good news: the reduction works in Debug as well, which means we actually use a real debugger. :)
Adam Barth
Comment 7
2013-02-15 18:15:19 PST
The call to didFailLoad is coming from the call to FrameLoader::checkLoadComplete inside the FrameLoader::frameDetached call.
Eric Seidel (no email)
Comment 8
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.
Eric Seidel (no email)
Comment 9
2013-02-20 16:37:43 PST
bug 110401
may fix this.
Eric Seidel (no email)
Comment 10
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.
Adam Barth
Comment 11
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. :)
Eric Seidel (no email)
Comment 12
2013-02-21 15:52:43 PST
Created
attachment 189623
[details]
Patch
Adam Barth
Comment 13
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?
Eric Seidel (no email)
Comment 14
2013-02-21 16:19:37 PST
Created
attachment 189630
[details]
Patch for landing
WebKit Review Bot
Comment 15
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
>
WebKit Review Bot
Comment 16
2013-02-21 16:41:25 PST
All reviewed patches have been landed. Closing bug.
Benjamin Poulain
Comment 17
2013-02-21 17:38:06 PST
Looks like this broke the Mac build on all bots.
Eric Seidel (no email)
Comment 18
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.
Eric Seidel (no email)
Comment 19
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.
Benjamin Poulain
Comment 20
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
Adam Barth
Comment 21
2013-02-21 18:38:56 PST
Looks like a missing null check. Perhaps we should roll out?
Adam Barth
Comment 22
2013-02-21 18:40:28 PST
We have the check for document but not for frame.
Eric Seidel (no email)
Comment 23
2013-02-21 18:49:10 PST
We can easily roll out, or I'll try a speculative fix for wk1.
Eric Seidel (no email)
Comment 24
2013-02-21 18:56:21 PST
Speculative fix landed as: Committed
r143681
: <
http://trac.webkit.org/changeset/143681
>
Ryosuke Niwa
Comment 25
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug