Closed Bug 563322 Opened 15 years ago Closed 13 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: smaug, Assigned: hsivonen)

References

Details

(Whiteboard: [inbound])

Attachments

(4 files, 1 obsolete file)

Part 1: HTML case
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
Part 2: XML case (changes behavior to spec compliance)
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
Test for the XML case
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
Part 3: Make XML script execution prevention work for real
(deleted), patch
smaug
: review+
Details | Diff | Splinter Review
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?)
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
Attached patch Part 1: HTML case (deleted) — — Splinter Review
Attachment #522995 - Attachment is obsolete: true
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+
(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.

Attachment

General

Creator:
Created:
Updated:
Size: