add_task() in script in <head><script src></head> will prevent add_task() in <body><script></script></body> to register

RESOLVED FIXED in Firefox 64

Status

defect
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

Version 3
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments)

Posted patch patchSplinter Review
STR:

1. apply my patch
2. ./mach test testing/mochitest/tests/Harness_sanity/test_add_task_in_head.html

Expected:

1. Test pass with one assertion

Actual:

[SimpleTest.finish()] No checks actually run. (You need to call ok(), is(), or similar functions at least once. Make sure you use SimpleTest.waitForExplicitFinish() if you need it.)

Note:

* You can see the <script> block does get executed given that the info() prints out.
* SimpleTest.executeSoon() can work around this.

I am not sure this is a bug in AddTask.js or HTML parsing/script execution, because I can see "Leaving test noop" shows up before info(), yet these async should not block inline JS execution.
* these async function should not
Perhaps they didn't "block" the next <script> block, but rather because of each <script> block is a macrotask of its own.

The following HTML produces console.log in the correct order (1-2-3) since Promise is scheduling an microtask.

data:text/html,<script>console.log(1); Promise.resolve().then(() => console.log(2))</script><script>console.log(3)</script>

and the following HTML produces correct order too (1-2-3) because we are using setTimeout() to schedule another macrotask.

data:text/html,<script>console.log(1); setTimeout(() => console.log(3))</script><script>console.log(2)</script>

I checked AddTask.js and it does seem like what AddTask.js is doing. Do our setTimeout() in mochitest behaves differently than in the wild?
The workaround does not really fix the problem :'(.

I suspect the place where setTimeout() schedule the macrotask could actually depend on when the parser adds the inline <script> block to the task queue.
Yeah, the fix in AddTask.js worked.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=673a21f58a1fdd8df76e4f363fe507571836b298

A few tests need adjustment because they are waiting for onload or DOMContentLoaded event in the test function. With the patch those can be removed.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #8)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0068860080c161c4aefdcc24a93155dc934b50a3

Looks like there are still a few failures.

Comment 10

8 months ago
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48f36c285594
add_task() should wait for document finishes loading before starting the test r=florian
(In reply to Florian Quèze [:florian] from comment #9)
> Looks like there are still a few failures.

I fixed layout/style/test/chrome/test_display_mode_reflow.html before landing. Other failure looks unrelated.

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/48f36c285594
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.