1133104 - crash in mozilla::dom::HTMLImageElement::UnbindFromTree(bool, bool)

Status: VERIFIED FIXED

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

Description

[Wayne Mery (:wsmwk)]

The first crash I can find is build 20141028030203 TB36.0a1
is currently #4 crash for TB37.0a2

bp-1a0f0a14-a2bb-4e8d-a2b2-190a32141028
=============================================================
0 	xul.dll	mozilla::dom::HTMLImageElement::UnbindFromTree(bool, bool)	dom/html/HTMLImageElement.cpp
1 	xul.dll	nsElementDeletionObserver::NodeWillBeDestroyed(nsINode const*)	editor/libeditor/nsHTMLAnonymousUtils.cpp
2 	xul.dll	nsNodeUtils::LastRelease(nsINode*)	dom/base/nsNodeUtils.cpp
3 	xul.dll	nsGenericDOMDataNode::Release()	dom/base/FragmentOrElement.cpp
4 	xul.dll	nsTArray_Impl<nsRefPtr<mozilla::dom::Touch>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned __int64, unsigned __int64)	xpcom/glue/nsTArray.h
5 	xul.dll	ContentUnbinder::Run()	dom/base/FragmentOrElement.cpp 

http://hg.mozilla.org/mozilla-central/annotate/a255a234946e/editor/libeditor/nsHTMLAnonymousUtils.cpp#l121
117  if (aNode == mNativeAnonNode) {
118    mObservedNode->RemoveMutationObserver(this);
119  } else {
120    mNativeAnonNode->RemoveMutationObserver(this);
121    static_cast<nsIContent*>(mNativeAnonNode)->UnbindFromTree();

http://hg.mozilla.org/mozilla-central/annotate/a255a234946e/dom/html/HTMLImageElement.cpp#l624
622  if (aNullParent &&
623      nsINode::GetParentNode()->Tag() == nsGkAtoms::picture &&
624      HTMLPictureElement::IsPictureEnabled()) {

The lines above which landed 2014-10-24 from bug 1037643

And time frame for Firefox crashes is similar, starting 2014-10-25 [1] https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Adom%3A%3AHTMLImageElement%3A%3AUnbindFromTree%28bool%2C+bool%29&product=Firefox&query_type=is_exactly&range_unit=weeks&process_type=any&hang_type=any&date=2014-10-26+21%3A00%3A00&range_value=4#tab-reports

So we can suspect bug 1037643

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

nsINode::GetParentNode() may return null.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8564408 [details] [diff] [review]
patch

Ouch.  r=me

Should probably get this on Aurora and beta...

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8564408 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1037643
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: NA
[Risks and why]: low risk, null check, but fixed 'element tag' check to include also namespace, as it should be.
[String/UUID change made/needed]: NA

Comment 6

[leichixian]

Also happens on nightly, see this: https://crash-stats.mozilla.com/report/index/8872b293-e886-45cf-8959-da8552150216

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This hasn't landed to mozilla-central yet, only mozilla-inbound, so Nightly doesn't have the fix.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9d47b2795b4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/041646b83f93
https://hg.mozilla.org/releases/mozilla-beta/rev/7f4a993f3336

Comment 10

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows no more crashes in builds after February 17th.

[1] - https://crash-stats.mozilla.com/report/list?product=Thunderbird&range_unit=days&range_value=28&signature=mozilla%3A%3Adom%3A%3AHTMLImageElement%3A%3AUnbindFromTree%28bool%2C+bool%29#tab-reports

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #10)
> Socorro [1] shows no more crashes in builds after February 17th.
> 
> [1] -
> https://crash-stats.mozilla.com/report/
> list?product=Thunderbird&range_unit=days&range_value=28&signature=mozilla%3A%
> 3Adom%3A%3AHTMLImageElement%3A%3AUnbindFromTree%28bool%2C+bool%29#tab-reports

Thanks for updating this. Agreed, it is probably gone.

For future however, bear in mind that one must be careful judging short term results based on aurora and nightly crash-stats. Rates can be low even if it's a "topcrash". Plus we sometimes have build breakages (esp on nightly) so we lose continuity on builds.  In this case 37.0a2 was averaging only 1 crash per day (some days having no crashes) and we have not quite a week of 37.0a2 data after patch landings because 37.0a2 ended 23-Feb due to merge.  :)