1017896 - Crash setting innerHTML to "<template>" in stale document

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Element creation created null pointer.: 'newContent', file parser/html/nsHtml5TreeOperation.cpp, line 362

###!!! ASSERTION: Null child: 'aChild', file parser/html/nsHtml5TreeBuilderCppSupplement.h, line 302

###!!! ASSERTION: null ptr: 'aKid', file content/base/src/FragmentOrElement.cpp, line 1069

Crash [@ nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) ]

Comment 1

[Jesse Ruderman]

Attachment: stack for the first assertion

Comment 2

[Jesse Ruderman]

Attachment: stack for the crash

Comment 3

[Boris Zbarsky [:bzbarsky]]

So we land in NS_NewHTMLTemplateElement, call mozilla::dom::HTMLTemplateElement::Init, get a null contentsOwner because  NS_ENSURE_TRUE(scriptObject || !hasHadScriptObject) fails in GetTemplateContentsOwner(), and return null from NS_NewHTMLTemplateElement.

This causes NS_NewHTMLElement and hence NS_NewElement to return NS_ERROR_OUT_OF_MEMORY.  

But nsHtml5TreeOperation::CreateElement assumes NS_NewElement never fails, which was true afaict until we added this template stuff.

Template element creation should be succeeding no matter what state the document is in, imo.  If we can't get a contentsOwner, we should just use a null one and make whatever relies on having one fail, instead of failing element creation altogether.

Comment 4

[William Chen [:wchen]]

It's problematic if template element creation succeeds but fails to create a template contents owner. The parser itself is dependent on having a contents owner because it is used to create a document fragment that the parser uses when appending children.

Without a document fragment, the parser doesn't have anywhere to put children elements. The next best thing we could do is probably append the children into the template element or use a document fragment owned by the main document. Both of these options violate the spec, but the latter behaves closer to the desired behavior.

Comment 5

[William Chen [:wchen]]

Attachment: Handle null template contents owner when creating template element

Change template element creation to not fail.
Uses the owner document if we fail at creating a template contents owner.

Comment 6

[Boris Zbarsky [:bzbarsky]]

If we need the template content owner, why can't we just make nsDocument::GetTemplateContentsOwner always succeed?  Seems like in the !scriptObject && hasHadScriptObject case we'd just need to make sure the template content owner ends up with hasHadScriptObject set to true is all.

(And also, should we be clearing the script handling object on mTemplateContentsOwner when it's cleared on the main document?  Seems to me like we should...)

Comment 7

[William Chen [:wchen]]

Comment on attachment 8431953 [details] [diff] [review]
Handle null template contents owner when creating template element

I'll look into doing that.

Comment 8

[William Chen [:wchen]]

Attachment: Handle null template contents owner when creating template element.

Updated patch with comments addressed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8432035 [details] [diff] [review]
Handle null template contents owner when creating template element.

>+      mTemplateContentsOwner->SetScriptHandlingObject(scriptObject);

Didn't NS_NewDOMDocument do that already?

I think we do want the SetScopeObject() call you have, though, if !scriptObject.

>+  if (!contentsOwner) {

How about we just MOZ_CRASH if it happens, if we want to handle it sanely?  Seems to me like it can't happen, though.

r=me witht those fixed.

Comment 10

[William Chen [:wchen]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Didn't NS_NewDOMDocument do that already?
Yeah, it does. I've removed the redundant call.
> How about we just MOZ_CRASH if it happens, if we want to handle it sanely? 
> Seems to me like it can't happen, though.
Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c15e59678a60

Comment 11

[Carsten Book [:Tomcat]]

sorry had to backout this csets in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=24743a3a0f91 since i guess that one of this 2 changes caused https://tbpl.mozilla.org/php/getParsedLog.php?id=41007049&tree=Mozilla-Inbound

Comment 12

[William Chen [:wchen]]

Orange not caused by this bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/91ad28afdd77

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91ad28afdd77