Closed Bug 527896 Opened 12 years ago Closed 12 years ago

Freeze script deferredness and asyncness upon insertion to tree

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

(Keywords: html5)

Attachments

(1 file, 1 obsolete file)

HTML5 says:
"Changing the src, type, charset, async, and defer attributes dynamically has no direct effect; these attribute are only used at specific times described below (namely, when the element is inserted into the document)."

Currently, the deferredness and asyncness at script execution time depends on the state of the attributes at execution time.

This a problem, because the HTML5 parser (which doesn't read back from the DOM due to off-the-main-thread parsing) can't know if a script still has the defer and async attributes by the time the end tag is parsed and the script executed.

It's probably not worthwhile to support timeouts going in and changing these attribute between the start and end tags if the parser happens to spin the event loop. However, to prevent timeouts like that putting the DOM and the HTML5 parser into an inconsistent state, it would be useful to freeze the asyncness and the deferredness bits in non-attribute fields of the script element node so that the HTML5 parser can be sure which code path the script loader will take (and, thus, can assume that a speculation isn't needed, etc.).
I was unable to make the part of the test that tests whether this bug is fixed run with the old parser. (The DOM state isn't consistent between document.writes, it seems.) However, the part of the test that tests that the fix didn't regress script-inserted cases runs using the default HTML parser.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #412176 - Flags: review?(jonas)
(In the test case, I'm trusting that all of async, defer and src get frozen as a group, so I just test the frozenness of the one that's the easiest to test: src.)
Can't enable the test case for this bug on m-c until bug 529544 has been sorted out properly. :-(
Depends on: 529544
Comment on attachment 412176 [details] [diff] [review]
Patch that freezes src, async and defer upon insertion if parser-inserted or when committed to executing at the latest

>diff --git a/content/base/public/nsIScriptElement.h b/content/base/public/nsIScriptElement.h

>   nsIScriptElement()
>     : mLineNumber(0),
>       mIsEvaluated(PR_FALSE),
>       mMalformed(PR_FALSE),
>       mDoneAddingChildren(PR_TRUE),
>+      mFrozen(PR_FALSE),
>+      mDefer(PR_FALSE),
>+      mAsync(PR_FALSE),
>+      mUri(nsnull),

No need to explicitly initialize an nsCOMPtr to nsnull, it does that by default.

>-  virtual already_AddRefed<nsIURI> GetScriptURI() = 0;
>+  already_AddRefed<nsIURI> GetScriptURI()
>+  {
>+    NS_PRECONDITION(mFrozen, "Not ready for this call yet!");
>+    NS_IF_ADDREF(mUri);
>+    return mUri.get();
>+  }

Make this return a raw pointer instead, i.e. one that isn't addrefed.

> protected:
>-  PRUint32 mLineNumber;
>-  PRPackedBool mIsEvaluated;
>-  PRPackedBool mMalformed;
>-  PRPackedBool mDoneAddingChildren;
>-  nsWeakPtr    mCreatorParser;
>+  PRUint32         mLineNumber;
>+  PRPackedBool     mIsEvaluated;
>+  PRPackedBool     mMalformed;
>+  PRPackedBool     mDoneAddingChildren;
>+  PRPackedBool     mFrozen;
>+  PRPackedBool     mDefer;
>+  PRPackedBool     mAsync;
>+  nsCOMPtr<nsIURI> mUri;
>+  nsWeakPtr        mCreatorParser;

Please don't try to line up things like this. It's just a pain to maintain when members are added and removed and makes patches hard to read. Just do the simple thing. If you do want to increase readability group members that are related together and put an empty line between groups.

>diff --git a/content/html/content/src/nsHTMLScriptElement.cpp >@@ -402,16 +400,21 @@ nsHTMLScriptElement::BindToTree(nsIDocum
>                                 nsIContent* aBindingParent,
>                                 PRBool aCompileEventHandlers)
> {
>   nsresult rv = nsGenericHTMLElement::BindToTree(aDocument, aParent,
>                                                  aBindingParent,
>                                                  aCompileEventHandlers);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (!mDoneAddingChildren) {
>+    // parser-inserted script
>+    FreezeUriAsyncDefer();
>+  }

It would make much more sense to me to make the call to FreezeUriAsyncDefer from the sink/treebuilder. Such as right after the call to SetScriptLineNumber.

r- based on the last comment, but looks great otherwise.

I take it this allows you to rely on that the async/defer attributes means that you don't have to stop parsing and wait for the script to execute? (well, not stop, but switch to speculative mode).
Attachment #412176 - Flags: review?(jonas) → review-
(In reply to comment #4)
+400,21 @@ nsHTMLScriptElement::BindToTree(nsIDocum
> >                                 nsIContent* aBindingParent,
> >                                 PRBool aCompileEventHandlers)
> > {
> >   nsresult rv = nsGenericHTMLElement::BindToTree(aDocument, aParent,
> >                                                  aBindingParent,
> >                                                  aCompileEventHandlers);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+  if (!mDoneAddingChildren) {
> >+    // parser-inserted script
> >+    FreezeUriAsyncDefer();
> >+  }
> 
> It would make much more sense to me to make the call to FreezeUriAsyncDefer
> from the sink/treebuilder. Such as right after the call to SetScriptLineNumber.

BindToTree is already sensitive to mDoneAddingChildren (whether it runs the script). Wouldn't it be inconsistent to leave it is the sink to call FreezeUriAsyncDefer() when there's already another mDoneAddingChildren-sensitive behavior in BindToTree?

In general, I was hoping to make elements know more about their special behavior in the parser-inserted case to avoid per-element branching in the parser. See bug 531260 and bug 531259.

> I take it this allows you to rely on that the async/defer attributes means that
> you don't have to stop parsing and wait for the script to execute? (well, not
> stop, but switch to speculative mode).

Yes, the idea is that the parser thread can know if a script will be treated as async or defer.
(In reply to comment #5)
> (In reply to comment #4)
> +400,21 @@ nsHTMLScriptElement::BindToTree(nsIDocum
> > >                                 nsIContent* aBindingParent,
> > >                                 PRBool aCompileEventHandlers)
> > > {
> > >   nsresult rv = nsGenericHTMLElement::BindToTree(aDocument, aParent,
> > >                                                  aBindingParent,
> > >                                                  aCompileEventHandlers);
> > >   NS_ENSURE_SUCCESS(rv, rv);
> > > 
> > >+  if (!mDoneAddingChildren) {
> > >+    // parser-inserted script
> > >+    FreezeUriAsyncDefer();
> > >+  }
> > 
> > It would make much more sense to me to make the call to FreezeUriAsyncDefer
> > from the sink/treebuilder. Such as right after the call to SetScriptLineNumber.
> 
> BindToTree is already sensitive to mDoneAddingChildren (whether it runs the
> script). Wouldn't it be inconsistent to leave it is the sink to call
> FreezeUriAsyncDefer() when there's already another
> mDoneAddingChildren-sensitive behavior in BindToTree?

The reason we're freezing is *because* we're being created by the sink, so I think it's more clear to have the call there. Having freezing as a consequence of being inserted into the tree is less intuitive IMO.

> In general, I was hoping to make elements know more about their special
> behavior in the parser-inserted case to avoid per-element branching in the
> parser. See bug 531260 and bug 531259.

I agree with this in general, and it's something i've worked on for a very long time (things used to be a lot worse than they are now). However script elements are always going to be special enough that trying to have them handled automatically is just going to make the code harder to follow.
Note to self: Inspect content/html/content/test/test_bug300691-2.html
Thanks for the r and sr. Pushed:
http://hg.mozilla.org/mozilla-central/rev/3d89d101b53e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.