Bug 76083 - Fail to run non-async scripts in IETC 11_RemovingAsyncScript
Summary: Fail to run non-async scripts in IETC 11_RemovingAsyncScript
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
: 84884 (view as bug list)
Depends on: 76350
Blocks: 76198
  Show dependency treegraph
 
Reported: 2012-01-11 11:39 PST by Eric Seidel (no email)
Modified: 2012-04-25 15:15 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.96 KB, patch)
2012-01-12 16:09 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (8.19 KB, patch)
2012-01-13 00:22 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (10.52 KB, patch)
2012-01-13 01:50 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (10.57 KB, patch)
2012-01-13 03:02 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-01-11 11:39:18 PST
Fail to run non-async scripts in IETC 11_RemovingAsyncScript

http://samples.msdn.microsoft.com/ietestcenter/HTML5/show_async_test.htm?11_RemovingAsyncScript

We only show "4".  Unclear why we don't run 1 2 or 3.
Comment 1 Eric Seidel (no email) 2012-01-11 23:36:57 PST
I suspect this will be simple to track down in a debugger.  I didn't spend very long looking at the reduction.
Comment 2 James Simonsen 2012-01-12 14:34:16 PST
Actually, Jochen has already started on this. So I'll pass this bug to him.
Comment 3 jochen 2012-01-12 16:09:16 PST
Created attachment 122331 [details]
Patch
Comment 4 Eric Seidel (no email) 2012-01-12 16:45:40 PST
Comment on attachment 122331 [details]
Patch

ick.  Really?  We need PHP for this?

Looks fine though.
Comment 5 James Simonsen 2012-01-12 16:48:53 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=122331&action=review

> Source/WebCore/dom/ScriptElement.cpp:85
> +    if (!m_willExecuteInOrder)

Hmm. I can't find anything in the spec for this. It seems like once we reach the end of prepareScript(), we need to execute it, unless it's parser-inserted and no longer in the document. That means we were broken in the async=true case too. The correct fix is probably to remove this function entirely.

http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1.html#execute-the-script-block

Can you try running the tests without this? If none break, we should remove it. If one does, we should add a FIXME that this is non-standard.

> LayoutTests/http/tests/misc/async-script-expected.txt:1
> +Test that a synchronous script tag is executed in the right order, even if it's removed from the document after before it was loaded.

"after before"?
Comment 6 jochen 2012-01-13 00:22:36 PST
Created attachment 122386 [details]
Patch
Comment 7 Adam Barth 2012-01-13 00:50:32 PST
Comment on attachment 122386 [details]
Patch

Did you want to add a test for the async=true case as well?
Comment 8 jochen 2012-01-13 01:50:16 PST
Created attachment 122393 [details]
Patch
Comment 9 Adam Barth 2012-01-13 01:54:05 PST
Comment on attachment 122393 [details]
Patch

Thanks!
Comment 10 WebKit Review Bot 2012-01-13 02:19:53 PST
Comment on attachment 122393 [details]
Patch

Attachment 122393 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11142945

New failing tests:
http/tests/misc/async-script.html
Comment 11 jochen 2012-01-13 03:02:39 PST
Created attachment 122405 [details]
Patch
Comment 12 Eric Seidel (no email) 2012-01-13 03:07:19 PST
Comment on attachment 122405 [details]
Patch

LGTM.
Comment 13 Tony Gentilcore 2012-01-13 03:31:16 PST
LGTM, nice
Comment 14 WebKit Review Bot 2012-01-13 06:20:35 PST
Comment on attachment 122405 [details]
Patch

Clearing flags on attachment: 122405

Committed r104927: <http://trac.webkit.org/changeset/104927>
Comment 15 WebKit Review Bot 2012-01-13 06:20:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Csaba Osztrogonác 2012-01-15 00:48:12 PST
Reopen, because it fails on SL and Qt:
--- /Volumes/Data/slave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/misc/async-script-expected.txt 
+++ /Volumes/Data/slave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/misc/async-script-actual.txt 
@@ -1,5 +1,6 @@
+FAIL: Timed out waiting for notifyDone to be called
 Test that an asynchronous script tag is executed, even if it's removed from the document before it was loaded.
 
 This test passes if the word "PASS" is displayed below.
 
-PASS
+


Could you check what the problem is?
Comment 17 jochen 2012-01-15 00:55:23 PST
(In reply to comment #16)
> Reopen, because it fails on SL and Qt:
> --- /Volumes/Data/slave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/misc/async-script-expected.txt 
> +++ /Volumes/Data/slave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/misc/async-script-actual.txt 
> @@ -1,5 +1,6 @@
> +FAIL: Timed out waiting for notifyDone to be called
>  Test that an asynchronous script tag is executed, even if it's removed from the document before it was loaded.
> 
>  This test passes if the word "PASS" is displayed below.
> 
> -PASS
> +
> 
> 
> Could you check what the problem is?

looking..
Comment 18 jochen 2012-01-15 14:45:23 PST
posted a fix in bug 76350
Comment 19 Alexey Proskuryakov 2012-04-25 15:15:27 PDT
*** Bug 84884 has been marked as a duplicate of this bug. ***