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

RESOLVED FIXED in Firefox 32

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: jruderman, Assigned: wchen)

Tracking

(Blocks 2 bugs, {assertion, crash, testcase})

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32+ fixed)

Details

(crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

Posted file 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&) ]
Posted file stack for the crash
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.
Flags: needinfo?(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.
Flags: needinfo?(wchen)
Change template element creation to not fail.
Uses the owner document if we fail at creating a template contents owner.
Attachment #8431953 - Flags: review?(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 on attachment 8431953 [details] [diff] [review]
Handle null template contents owner when creating template element

I'll look into doing that.
Attachment #8431953 - Flags: review?(bzbarsky)
Updated patch with comments addressed.
Assignee: nobody → wchen
Attachment #8431953 - Attachment is obsolete: true
Attachment #8432035 - Flags: review?(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.
Attachment #8432035 - Flags: review?(bzbarsky) → review+
(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
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/91ad28afdd77
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1022869
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.