563322 - Does nsGenericHTMLElement::SetInnerHTML need to call scriptloader->SetEnabled(...)

Status: RESOLVED FIXED

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

Description

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

See bug 562013 comment 29 and bug 562013 comment 32.

Comment 1

[Boris Zbarsky [:bzbarsky]]

From bug 562013 comment 29:

> Is this script disabling and re-enabling for <script> elements or also for
> something else?

As far as I know, it's just for <script> elements.  The change to scriptloader was made in bug 299231.  The change to do something for scripts at all was bug 116834.

So if the HTML5 parser guarantees that the <script>s from inside innerHTML won't run, this code can go away.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Henri, see comment 1?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We do want to make sure that those <script>s get their mIsEvaluated flag set though. Otherwise these scripts could run unexpectedly if their parent is moved around in the DOM.

Not sure how that affects things?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> We do want to make sure that those <script>s get their mIsEvaluated flag set

Sure.  It is.  Right here: http://hg.mozilla.org/mozilla-central/file/1acce93e0198/parser/html/nsHtml5TreeOpExecutor.cpp#l693

(On a somewhat unrelated note we have a similar call in the frameset case in the old parser that doesn't happen in the new one... should it?)

Comment 5

[Henri Sivonen (:hsivonen)]

In the fragment mode, the HTML5 parser takes care of preventing execution per spec. Inside framesets, the HTML5 parser discards scripts without putting them into the DOM.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Don't change the scripting state in the HTML case (XML case runs old code still)

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Part 1: HTML case

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Part 2: XML case (changes behavior to spec compliance)

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Test for the XML case

Comment 10

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

Comment on attachment 523266 [details] [diff] [review]
Part 2: XML case (changes behavior to spec compliance)

>+  // True to call prevent script execution in the fragment mode.
>+  PRUint8 mPreventScriptExecution : 1;
>+
I think contentsink has zeroing new, but couldn't find it now.
But if we have, this is ok.

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to comment #10)
> Comment on attachment 523266 [details] [diff] [review]
> Part 2: XML case (changes behavior to spec compliance)
> 
> >+  // True to call prevent script execution in the fragment mode.
> >+  PRUint8 mPreventScriptExecution : 1;
> >+
> I think contentsink has zeroing new, but couldn't find it now.
> But if we have, this is ok.

We have NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW in each subclass as the comment in nsContentSink says we should.

Thanks.

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Part 3: Make XML script execution prevention work for real

Bug 650501 was accidentally making the mochitest here pass, which is why I didn't notice that the fix in part 2 was incomplete. Here's an addendum that makes the fix actually work.

Comment 13

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

Comment on attachment 526707 [details] [diff] [review]
Part 3: Make XML script execution prevention work for real

> nsresult
> nsXMLFragmentContentSink::CloseElement(nsIContent* aContent)
> {
>   // don't do fancy stuff in nsXMLContentSink
>+  if (mPreventScriptExecution && aContent->Tag() == nsGkAtoms::script
>+      && (aContent->GetNameSpaceID() == kNameSpaceID_XHTML
&& should be in the previous line

>+          || aContent->GetNameSpaceID() == kNameSpaceID_SVG)) {
And so should ||

Comment 14

[Henri Sivonen (:hsivonen)]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ff515cbd864e
http://hg.mozilla.org/integration/mozilla-inbound/rev/1e3cf346898c
http://hg.mozilla.org/integration/mozilla-inbound/rev/a228ff77a9f8
http://hg.mozilla.org/integration/mozilla-inbound/rev/79548c572c09

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ff515cbd864e
http://hg.mozilla.org/mozilla-central/rev/1e3cf346898c
http://hg.mozilla.org/mozilla-central/rev/a228ff77a9f8
http://hg.mozilla.org/mozilla-central/rev/79548c572c09