1433414 - Crash in nsTreeSanitizer::SanitizeChildren

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-984d442d-fcb3-4498-b9cc-8d1920180126.
=============================================================

Top 10 frames of crashing thread:

0 libxul.so nsTreeSanitizer::SanitizeChildren xpcom/base/nsCOMPtr.h:624
1 libxul.so nsContentUtils::ParseFragmentXML dom/base/nsContentUtils.cpp:5234
2 libxul.so nsContentUtils::CreateContextualFragment [clone .cold.459] 
3 libxul.so mozilla::dom::FragmentOrElement::SetInnerHTMLInternal [clone .cold.425] 
4 libxul.so mozilla::dom::ElementBinding::set_innerHTML [clone .cold.447] 
5 libxul.so mozilla::dom::GenericBindingSetter 
6 libxul.so js::InternalCallOrConstruct 
7 libxul.so js::CallSetter 
8 libxul.so SetExistingProperty 
9 libxul.so js::NativeSetProperty<1u> js/src/vm/NativeObject.cpp:2784

=============================================================

There are 4 crashes (from 1 installation) in nightly 60 with buildid 20180126035135. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1432966.

[1] https://hg.mozilla.org/mozilla-central/rev?node=b23adcdd6052

Comment 1

[Frederik Braun [:freddy]]

Attachment: Bug 1433414: Add missing NS_ENSURE_SUCCESS

This adds a missing NS_ENSURE_SUCCESS, to catch XML parsing failures (e.g. unclosed tags).

Review commit: https://reviewboard.mozilla.org/r/215868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215868/

Comment 2

[Frederik Braun [:freddy]]

Ah, these crashes come from my local development build.
This patch should do the right thing, but I'm wandering into unknown territory here. Feel free to steal.

Comment 3

[Frederik Braun [:freddy]]

Comment on attachment 8945759 [details]
Bug 1433414: Add missing NS_ENSURE_SUCCESS

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215868/diff/1-2/

Comment 4

[Olli Pettay [:smaug]]

Comment on attachment 8945759 [details]
Bug 1433414: Add missing NS_ENSURE_SUCCESS

https://reviewboard.mozilla.org/r/215868/#review221674

I guess we can do this.
might be worth to check if all the callers of Sanitize ensure non-null value is passed.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/176a681a1788
Add missing NS_ENSURE_SUCCESS r=smaug

Comment 6

[Olli Pettay [:smaug]]

er, too soon. This isn't enough.
FinishFragmentParsing may return NS_OK, but null fragment.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #6)
> er, too soon. This isn't enough.
> FinishFragmentParsing may return NS_OK, but null fragment.

16:01 <emilio> smaug: mRoot is non-null since WillBuildModel
16:02 <@smaug> ahaa
16:02 <@smaug> that would fail?
16:02 <@smaug> if nlul
16:02 <@smaug> null
16:02 <@smaug> ok, thanks for checking
16:02 <@smaug> not  exactly good API
16:02 <@smaug> rather error prone
16:03 <emilio> smaug: yeah, indeed... WillBuildModel is called unconditionally if the result is ok, so should be fine
16:03 <emilio> smaug: and agree that the API is not great :)
16:10 <emilio> smaug: there's an NS_OK early-return that doesn't call WillBuildModel, but that shouldn't affect ParseFragmentXML (see bug 420008)

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/176a681a1788