669012 - Disentangle nsIContent::DoneAddingChildren and script handling

Status: RESOLVED FIXED

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

Comment 1

[Mounir Lamouri (:mounir)]

Could you explain the issues? The summary isn't really self-explanatory.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: ignore for and event attributes for SVG elements

I was trying to explain why my upcoming patch is correct, but it turned out not to be, because we look at the for and event attributes of SVG elements. (Yes, really.)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 543822 [details] [diff] [review]
Part a: ignore for and event attributes for SVG elements

Make sure other browsers behave the same way.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Introduce nsIScriptElement::AttemptToExecute

tl;dr: I'm right.

First, why I think we should do this: at first sight, it may seem like a good idea (or at least, not a bad one) to use the same signature for the script elements and the other implementations of this method (nsHTMLObjectElement, nsHTMLTextAreaElement, nsHTMLSharedObjectElement, nsHTMLTitleElement, nsHTMLSelectElement, nsSVGTitleElement, and nsXTFElementWrapper). After all, they need to be called around the same time for roughly the same reason.

It turns out that's not really true. Looking at the patch, only XSLT actually calls it at the same time for both sets of elements. Also, they only superficially use the same signature: script elements ignore the aHaveNotified parameter, and the other implementation always return NS_OK (and indeed, all callers ignore the return value). The script implementations don't always return NS_OK, but their only other return value NS_ERROR_HTMLPARSER_BLOCK (see below), and as such it would be clearer to return a boolean (and that would prevent bugs such as introduced in bug 383383, where a returned NS_ERROR_DOM_NOT_SUPPORTED_ERR shut down the XML parser).

nsScriptLoader::ProcessScriptElement either returns a failure nsresult, NS_CONTENT_SCRIPT_IS_EVENTHANDLER (only for HTML scripts since part a), or NS_OK. nsScriptElement::MaybeProcessScript then filters out all failure nsresults except for NS_ERROR_HTMLPARSER_BLOCK, and nsHTMLScriptElement::MaybeProcessScript turns NS_CONTENT_SCRIPT_IS_EVENTHANDLER into NS_OK. That makes the return value of MaybeProcessScript effectively two-state.

To make the code more understandable, I've written patches to split nsresult nsIContent::DoneAddingChildren(PRBool) into two methods more suited for their respective uses: bool nsIScriptElement::AttemptToExecute() (part b) and void nsIContent::DoneAddingChildren(PRBool) (part c). Because nsIScriptElement::AttemptToExecute is its only caller, I'll then make nsIScriptElement::MaybeProcessScript a boolean as well, and remove nsHTMLScriptElement::MaybeProcessScript as it's no longer necessary (part d), and do the same to nsScriptLoader::ProcessScriptElement (part e). (After that, I've got a couple of cleanups I noticed coming up as well.)

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Remove the return value from nsIContent::DoneAddingChildren

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part d: Make nsIScriptElement::MaybeProcessScript return a boolean

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part e: Make nsScriptLoader::ProcessScriptElement return a boolean

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part f: Less QIs in nsScriptLoader::ProcessScriptElement

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part g: Remove NS_CONTENT_SCRIPT_IS_EVENTHANDLER

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'd like Henri to have a look at the patches as well, and he's away until the 29th, so no hurry here.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Splitting the patch makes reviewing a bit harder this case...

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 551336 [details] [diff] [review]
Part b: Introduce nsIScriptElement::AttemptToExecute


>+  /**
>+   * This method is called when the parser finishes creating the script
>+   * element's children, if any are present.
>+   *
>+   * @return whether the script will be loaded asynchronously
Is that what NS_ERROR_HTMLPARSER_BLOCK is really about?
It is easy to misunderstood "asynchronously" since there is also <script async>.
So, could you rephrase that sentence.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 551339 [details] [diff] [review]
Part d: Make nsIScriptElement::MaybeProcessScript return a boolean

>   /**
>    * Processes the script if it's in the document-tree and links to or
>    * contains a script. Once it has been evaluated there is no way to make it
>    * reevaluate the script, you'll have to create a new element. This also means
>    * that when adding a src attribute to an element that already contains an
>    * inline script, the script referenced by the src attribute will not be
>    * loaded.
>    *
>    * In order to be able to use multiple childNodes, or to use the
>    * fallback mechanism of using both inline script and linked script you have
>    * to add all attributes and childNodes before adding the element to the
>    * document-tree.
>    */
>-  virtual nsresult MaybeProcessScript() = 0;
>+  virtual bool MaybeProcessScript() = 0;

Please add a comment what the return value means.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 551340 [details] [diff] [review]
Part e: Make nsScriptLoader::ProcessScriptElement return a boolean

In general I'm not strongly against or for this change.

Though, it is nice to have DoneAddingChildren just a simple
notification which doesn't need to return anything.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Henri, could you have a look at the patches here?

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to Ms2ger from comment #15)
> Henri, could you have a look at the patches here?

They look good.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Comment on attachment 551336 [details] [diff] [review] [diff] [details] [review]
> Part b: Introduce nsIScriptElement::AttemptToExecute
> 
> 
> >+  /**
> >+   * This method is called when the parser finishes creating the script
> >+   * element's children, if any are present.
> >+   *
> >+   * @return whether the script will be loaded asynchronously
> Is that what NS_ERROR_HTMLPARSER_BLOCK is really about?
> It is easy to misunderstood "asynchronously" since there is also <script
> async>.
> So, could you rephrase that sentence.

Something along the lines of "whether the parser will be blocked while this script is being loaded", perhaps?

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/acf6f9a14854
https://hg.mozilla.org/mozilla-central/rev/dcf7c1b0a756
https://hg.mozilla.org/mozilla-central/rev/a47f8a6f6705
https://hg.mozilla.org/mozilla-central/rev/ec02dc79904a
https://hg.mozilla.org/mozilla-central/rev/6b58c2e56c26
https://hg.mozilla.org/mozilla-central/rev/ce3ad0e12408
https://hg.mozilla.org/mozilla-central/rev/52951fda6786