Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"prepare a script" and scripts moved to a different document #3486

Closed
annevk opened this issue Feb 20, 2018 · 9 comments
Closed

"prepare a script" and scripts moved to a different document #3486

annevk opened this issue Feb 20, 2018 · 9 comments
Labels
interop Implementations are not interoperable with each other topic: script

Comments

@annevk
Copy link
Member

annevk commented Feb 20, 2018 •
edited
Loading

This relates to #2137, but since it's not strictly related to the parser I figured I'd file it separately for now. We can merge later on.

If the element is flagged as "parser-inserted", but the element's node document is not the Document of the parser that created the element, then return.

It seems that this step does not necessarily depend solely on "parser-inserted".

Aside: does this mean we need to store the "creator document" on the script element? The current prose is rather hand-wavy.

<script>
 var doc = document.implementation.createHTMLDocument();
 var s1 = document.createElement("script"); s1.textContent = "console.log('s1 executed'); doc.body.appendChild(s2);"
 var s2 = document.createElement("script"); s2.textContent = "console.log('s2 executed');";
 var df = document.createDocumentFragment(); df.appendChild(s1); df.appendChild(s2);
 document.head.appendChild(df);
</script>

This only logs "s1 executed" in Chrome, Edge, Firefox, and Safari.

<iframe></iframe>
<script>
 var s1 = document.createElement("script"); s1.textContent = "console.log('s1 executed'); frames[0].document.body.appendChild(s2);"
 var s2 = document.createElement("script"); s2.textContent = "console.log('s2 executed');";
 var df = document.createDocumentFragment(); df.appendChild(s1); df.appendChild(s2);
 document.head.appendChild(df);
</script>

This only logs "s1 executed" in Firefox, but logs "s1 executed", "s2 executed" in Chrome, Edge, and Safari. Does this mean non-Firefox basically wants browsing-context connected as a sole check?

I also tested:

If the element is not connected, then return. The script is not executed.

<script>
 var s1 = document.createElement("script"); s1.textContent = "console.log('s1 executed'); df.appendChild(s2);"
 var s2 = document.createElement("script"); s2.textContent = "console.log('s2 executed');";
 var df = document.createDocumentFragment(); df.appendChild(s1); df.appendChild(s2);
 document.head.appendChild(df);
</script>

Apart from Firefox everyone implements it. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1439572 on this.
@annevk annevk added topic: script interop Implementations are not interoperable with each other labels Feb 20, 2018
@annevk
Copy link
Member Author

annevk commented Feb 20, 2018

(Note that when scripts execute relative to insertion via DocumentFragment is tracked by whatwg/dom#575 and only Safari is a little different there. Safari executes after the insert of the element, whereas the other browsers execute after the insert operation (which can insert multiple elements as shown above) completes.)

@domenic
Copy link
Member

domenic commented Feb 20, 2018

I think #2673 is the most ambitious attempt to fix this, but I believe @hiroshige-g hasn't had time to try this in Chrome yet.

@annevk annevk mentioned this issue Feb 21, 2018
Merged
3 tasks
@bzbarsky
Copy link
Contributor

So for the second testcase in #3486 (comment) here's what Gecko is doing:

  1. We insert all the nodes in the DOM. This queues up the script executions for those nodes, and marks them "already started". In particular, we insert s2 into the document before s1 runs. This does not seem to be universally true. If I console.log(s2.parentNode) while s1 is running, I get null in WebKit but the <head> in Gecko/Blink/Edge. https://jsfiddle.net/czs0cv24/1/ has a live testcase. Anyway, each of these queued-up script executions knows what document it's for.
  2. Before returning from the appendChild(df) call we start processing those queued up script executions.
  3. We run the first script. When it does the appendChild(s2) call the call is ignored, because s2 is marked "already started".
  4. We go to run the second script (from the already-queued-up thing). We compare the node document of the script node to the document we're queued up for, discover that they do not match, and do not run the script. This is separate from the checks we do for parser-inserted scripts: those act on the level of the "queue up executions" bit in step 1. This check is at https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp#2182-2185

@bzbarsky
Copy link
Contributor

So in terms of how this compares to the spec... As far as I can tell, per https://dom.spec.whatwg.org/#concept-node-insert the "insertion steps" for s1 should run when s2 is in fact not inserted yet, and in fact when its parentNode is null per step 4 having happened for it but step 7 not having reached it yet.

Those insertion steps land us at https://html.spec.whatwg.org/multipage/infrastructure.html#becomes-connected and https://html.spec.whatwg.org/multipage/scripting.html#the-script-element says that when the script element becomes connected it must immediately prepare the script. That lands in https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script and synchronously lands in the "Otherwise" case of step 25, which runs the script.

In particular, per spec as currently written, when s1 runs s2 is not yet marked as "already started" and the append should execute it. But then we should unwind to the insert algorithm and insert s2 in the <head> as well, so it's now inserted in two places. That seems broken. @annevk am I missing something here?

@annevk
Copy link
Member Author

annevk commented Feb 26, 2018

I consider it a bug that DOM runs the insertion steps so early: whatwg/dom#576 has a fix; I haven't yet written tests or filed a bug against Safari though. Was waiting for more thorough review since it's a rather substantial change.

I don't understand what you mean in 3 by the appendChild() call being ignored. If I log doc.body.innerHTML afterwards it seems like that call very much succeeded.

@bzbarsky
Copy link
Contributor

I consider it a bug that DOM runs the insertion steps so early

Ah, good.

I don't understand what you mean in 3 by the appendChild() call being ignored.

Sorry, I should have been more clear. It's ignored for purposes of running the s2 script. The call succeeds, the insertion steps are run, https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script step 1 returns immediately and nothing interesting happens in terms of the script executing.

@bzbarsky
Copy link
Contributor

I consider it a bug that DOM runs the insertion steps so early

Actually... There are complications about running the insertion steps later, in general. I guess this is only observable with document fragments, so maybe there won't be too much breakage, but it would be a pretty significant behavior change to make all insertion steps run later.

@annevk
Copy link
Member Author

annevk commented Feb 26, 2018

The change only affects insertion of a DocumentFragment object as only then can you insert multiple nodes at once. Concerns with that are best added directly to whatwg/dom#576.

@domenic
Copy link
Member

domenic commented Jul 7, 2020

I feel like this is closed now that #2673 and related refactorings are done, but there's also a lot of discussion of DocumentFragment cases. I'll close this, but please feel free to reopen (or re-file a more focused issue) if there's a case I've missed that we still want to track.

@domenic domenic closed this as completed Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interop Implementations are not interoperable with each other topic: script
Milestone
No milestone
Development

No branches or pull requests
3 participants
@domenic @bzbarsky @annevk