The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla8

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: smaug, Assigned: hsivonen)

Tracking

Trunk
mozilla8
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
See bug 562013 comment 29 and bug 562013 comment 32.
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.
Henri, see comment 1?
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?
> 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?)
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Depends on: 596182
(Assignee)

Comment 6

6 years ago
Created attachment 522995 [details] [diff] [review]
Don't change the scripting state in the HTML case (XML case runs old code still)
(Assignee)

Comment 7

6 years ago
Created attachment 523265 [details] [diff] [review]
Part 1: HTML case
Attachment #522995 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Created attachment 523266 [details] [diff] [review]
Part 2: XML case (changes behavior to spec compliance)
(Assignee)

Comment 9

6 years ago
Created attachment 523267 [details] [diff] [review]
Test for the XML case
(Assignee)

Updated

6 years ago
Attachment #523265 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #523266 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #523267 - Flags: review?(Olli.Pettay)
(Reporter)

Updated

6 years ago
Attachment #523265 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Updated

6 years ago
Attachment #523267 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Comment 10

6 years ago
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.
Attachment #523266 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

6 years ago
Blocks: 613662
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 526707 [details] [diff] [review]
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.
Attachment #526707 - Flags: review?(Olli.Pettay)
(Reporter)

Comment 13

6 years ago
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 ||
Attachment #526707 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 14

6 years ago
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
Flags: in-testsuite?
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 677658
You need to log in before you can comment on or make changes to this bug.