Closed Bug 563322 Opened 10 years ago Closed 9 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: smaug, Assigned: hsivonen)

References

Details

(Whiteboard: [inbound])

Attachments

(4 files, 1 obsolete file)

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.
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?)
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: nobody → hsivonen
Status: NEW → ASSIGNED
Depends on: 596182
Attachment #522995 - Attachment is obsolete: true
Attachment #523265 - Flags: review?(Olli.Pettay)
Attachment #523266 - Flags: review?(Olli.Pettay)
Attachment #523267 - Flags: review?(Olli.Pettay)
Attachment #523265 - Flags: review?(Olli.Pettay) → review+
Attachment #523267 - Flags: review?(Olli.Pettay) → review+
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+
Blocks: 613662
(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.
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)
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+
Depends on: 677658
You need to log in before you can comment on or make changes to this bug.