Last Comment Bug 682058 - nsGenericElement::SetInnerHTML should suppress DOMNodeRemoved when dealing with a non-HTML5 document
: nsGenericElement::SetInnerHTML should suppress DOMNodeRemoved when dealing wi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 8 Branch
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: xssfilter
  Show dependency treegraph
 
Reported: 2011-08-25 12:47 PDT by Riccardo Pelizzi
Modified: 2011-09-09 02:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (326 bytes, application/xhtml+xml)
2011-08-25 16:26 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details
patch (1.96 KB, patch)
2011-08-26 02:49 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
jonas: review+
Details | Diff | Review

Description Riccardo Pelizzi 2011-08-25 12:47:20 PDT
User Agent: Mozilla/5.0 (X11; Linux i686; rv:8.0a2) Gecko/20110819 Firefox/8.0a2
Build ID: 20110819142441

Steps to reproduce:

To implement a patch for bug #528661, I need to escape HTML entities on a custom string. hsivonen suggested that I create a title element with NS_NewHTMLTitleElement, stuff the unencoded text in it with SetInnerHTML and retrieve the decoded text with GetTextContent.


Actual results:

The reftest layout/reftests/svg/as-image/svg-stylesheet-datauri-1.html fails because it triggers the assertion:

NS_ASSERTION(aChild->IsNodeOfType(nsINode::eCONTENT) &&
                static_cast<nsIContent*>(aChild)->
                  IsInNativeAnonymousSubtree() ||
                IsSafeToRunScript() ||
                sDOMNodeRemovedSuppressCount,
                "Want to fire DOMNodeRemoved event, but it's not safe");

sicking thinks that the caller (nsGenericHTMLElement::SetInnerHTML) should block DomNodeRemoved scripts from running using a nsAutoScriptBlockerSuppressNodeRemoved in the else-path that is taken when the document being modified is not an HTML5 document.


Expected results:

There shouldn't be any assertion. I don't know the code as well as Jonas, but aren't we checking whether it's safe to run a script too eagerly? In nsContentUtils::MaybeFireNodeRemoved, we are asserting this before we even check that any script would need to be run in this specific context. For example, in my code there would be no script fired for any mutation event on the title element. If we asserted that *after* we probed for event listeners, there would no assertion as the execution path would not even test the assertion.
Comment 1 Riccardo Pelizzi 2011-08-25 12:54:56 PDT
sorry, the last paragraph should say "No assertion should fail", either because it does not get asserted when my code calls into setInnerHTML (because we check if there is any event handler before asserting whether it's safe to execute such handler) or because sDOMNodeRemovedSuppressionCount is > 0 (or something like that, mxr is taking a break right now...)
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-25 16:26:50 PDT
Created attachment 555886 [details]
testcase
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-26 02:49:36 PDT
Created attachment 555986 [details] [diff] [review]
patch
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-08 15:31:47 PDT
Comment on attachment 555986 [details] [diff] [review]
patch

Review of attachment 555986 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-09-09 02:54:17 PDT
http://hg.mozilla.org/mozilla-central/rev/164976bffd31

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