Last Comment Bug 563322 - Does nsGenericHTMLElement::SetInnerHTML need to call scriptloader->SetEnabled(...)
: Does nsGenericHTMLElement::SetInnerHTML need to call scriptloader->SetEnabled...
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
:
Mentors:
Depends on: 596182 677658
Blocks: 562013 613662
  Show dependency treegraph
 
Reported: 2010-05-03 05:41 PDT by Olli Pettay [:smaug]
Modified: 2011-08-09 12:57 PDT (History)
10 users (show)
hsivonen: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't change the scripting state in the HTML case (XML case runs old code still) (2.85 KB, patch)
2011-03-30 07:00 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
no flags Details | Diff | Splinter Review
Part 1: HTML case (2.85 KB, patch)
2011-03-31 02:43 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
bugs: review+
Details | Diff | Splinter Review
Part 2: XML case (changes behavior to spec compliance) (16.81 KB, patch)
2011-03-31 02:44 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
bugs: review+
Details | Diff | Splinter Review
Test for the XML case (2.27 KB, patch)
2011-03-31 02:44 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
bugs: review+
Details | Diff | Splinter Review
Part 3: Make XML script execution prevention work for real (1.18 KB, patch)
2011-04-18 06:08 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
bugs: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2010-05-03 05:41:18 PDT
See bug 562013 comment 29 and bug 562013 comment 32.
Comment 1 Boris Zbarsky [:bz] 2010-05-03 05:53:38 PDT
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 [:bz] 2010-05-03 05:56:39 PDT
Henri, see comment 1?
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2010-05-03 06:31:38 PDT
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 [:bz] 2010-05-03 06:47:48 PDT
> 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) (Not doing reviews or reading bugmail until 2016-08-01) 2010-05-04 07:07:01 PDT
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) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-30 07:00:22 PDT
Created attachment 522995 [details] [diff] [review]
Don't change the scripting state in the HTML case (XML case runs old code still)
Comment 7 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-31 02:43:42 PDT
Created attachment 523265 [details] [diff] [review]
Part 1: HTML case
Comment 8 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-31 02:44:33 PDT
Created attachment 523266 [details] [diff] [review]
Part 2: XML case (changes behavior to spec compliance)
Comment 9 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-31 02:44:52 PDT
Created attachment 523267 [details] [diff] [review]
Test for the XML case
Comment 10 Olli Pettay [:smaug] 2011-03-31 04:45:15 PDT
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) (Not doing reviews or reading bugmail until 2016-08-01) 2011-03-31 05:27:21 PDT
(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) (Not doing reviews or reading bugmail until 2016-08-01) 2011-04-18 06:08:55 PDT
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.
Comment 13 Olli Pettay [:smaug] 2011-04-18 08:50:14 PDT
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 ||

Note You need to log in before you can comment on or make changes to this bug.