Closed Bug 1433414 Opened 4 years ago Closed 4 years ago

Crash in nsTreeSanitizer::SanitizeChildren

Categories

(Core :: DOM: Security, defect)

Unspecified
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: calixte, Assigned: freddy)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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
Flags: needinfo?(kmaglione+bmo)
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.
Assignee: nobody → fbraun
Attachment #8945759 - Flags: review?(kmaglione+bmo) → review?(bugs)
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.
Attachment #8945759 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/176a681a1788
Add missing NS_ENSURE_SUCCESS r=smaug
er, too soon. This isn't enough.
FinishFragmentParsing may return NS_OK, but null fragment.
(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)
https://hg.mozilla.org/mozilla-central/rev/176a681a1788
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.