Closed Bug 1089463 Opened 5 years ago Closed 5 years ago

crash in mozilla::RestyleTracker::AddPendingRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint)

Categories

(Core :: Layout, defect, critical)

35 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 + fixed
firefox36 + fixed

People

(Reporter: jbecerra, Assigned: heycam)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-056ffc70-194c-4602-a5d0-2c99d2141020.
=============================================================

It's #25 in the list of top crashers for Firefox 36 (nightly), and it's mostly being reported on Windows 8.1, 7, and XP. It's also in the list of explosive reports. There are no comments so far. It's been happening for a while, but it looks like the number of crashers started to increase around 10/13. 

More reports at: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3ARestyleTracker%3A%3AAddPendingRestyle%28mozilla%3A%3Adom%3A%3AElement%2A%2C+nsRestyleHint%2C+nsChangeHint%29

0 	xul.dll 	mozilla::RestyleTracker::AddPendingRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) 	layout/base/RestyleTracker.h
1 	xul.dll 	mozilla::RestyleManager::PostRestyleEventCommon(mozilla::dom::Element*, nsRestyleHint, nsChangeHint, bool) 	layout/base/RestyleManager.cpp
2 	xul.dll 	mozilla::RestyleManager::AttributeChanged(mozilla::dom::Element*, int, nsIAtom*, int) 	layout/base/RestyleManager.cpp
3 	xul.dll 	PresShell::AttributeChanged(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int) 	layout/base/nsPresShell.cpp
4 	xul.dll 	nsNodeUtils::AttributeChanged(mozilla::dom::Element*, int, nsIAtom*, int) 	content/base/src/nsNodeUtils.cpp
5 	xul.dll 	mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) 	content/base/src/Element.cpp
6 	xul.dll 	nsStyledElementNotElementCSSInlineStyle::SetInlineStyleRule(mozilla::css::StyleRule*, nsAString_internal const*, bool) 	content/base/src/nsStyledElement.cpp
7 	xul.dll 	nsDOMCSSAttributeDeclaration::SetCSSDeclaration(mozilla::css::Declaration*) 	layout/style/nsDOMCSSAttrDeclaration.cpp
8 	xul.dll 	nsDOMCSSDeclaration::RemoveProperty(nsCSSProperty) 	layout/style/nsDOMCSSDeclaration.cpp
9 	xul.dll 	nsDOMCSSAttributeDeclaration::SetPropertyValue(nsCSSProperty, nsAString_internal const&) 	layout/style/nsDOMCSSAttrDeclaration.cpp
10 	xul.dll 	mozilla::dom::CSS2PropertiesBinding::set_display 	obj-firefox/dom/bindings/CSS2PropertiesBinding.cpp
11 	xul.dll 	mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp
12 	mozjs.dll 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
13 	mozjs.dll 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
14 	mozjs.dll 	js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
15 	mozjs.dll 	js::CallSetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int, bool, JS::MutableHandle<JS::Value>) 	js/src/jscntxtinlines.h
16 	mozjs.dll 	js::SetPropertyIgnoringNamedGetter(JSContext*, js::BaseProxyHandler const*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JSPropertyDescriptor>, bool, bool, JS::MutableHandle<JS::Value>) 	js/src/proxy/BaseProxyHandler.cpp
17 	xul.dll 	mozilla::dom::DOMProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) 	dom/bindings/DOMJSProxyHandler.cpp
18 	mozjs.dll 	js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) 	js/src/proxy/Proxy.cpp
19 	mozjs.dll 	js::proxy_SetGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) 	js/src/proxy/Proxy.cpp
20 	mozjs.dll 	JSObject::nonNativeSetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) 	js/src/jsobj.cpp
21 	mozjs.dll 	js::DirectProxyHandler::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) 	js/src/proxy/DirectProxyHandler.cpp
22 	mozjs.dll 	js::CrossCompartmentWrapper::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) 	js/src/proxy/CrossCompartmentWrapper.cpp
23 	mozjs.dll 	js::Proxy::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>) 	js/src/proxy/Proxy.cpp
24 	mozjs.dll 	js::proxy_SetGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) 	js/src/proxy/Proxy.cpp
25 	mozjs.dll 	JSObject::nonNativeSetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, bool) 	js/src/jsobj.cpp
26 	mozjs.dll 	js::SetObjectElement(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, bool, JS::Handle<JSScript*>, unsigned char*) 	js/src/vm/Interpreter.cpp
27 	mozjs.dll 	js::jit::DoSetElemFallback 	js/src/jit/BaselineIC.cpp
28 		@0x549fda
Null curData, looks like bug 931668 and bug 1062732:

if (!(curData->mRestyleHint & eRestyle_ForceDescendants)) {
59ed3fca f70100020000    test    dword ptr [ecx],200h ds:002b:00000000=????????

ni dbaron since heycam has an away message.
Flags: needinfo?(dbaron)
Example crash report:  bp-96094435-2013-4d66-b0d0-ebd0d2141027

Probably don't have time to look until next week.

Seems like it's equally big in 35 and 36.
... until *at least* next week.
Attached file test (crashes Firefox)
We are getting our restyle bits and mPendingRestyles contents out of sync.  In CollectRestyles, we can return early when |element->GetCrossShadowCurrentDoc() != collector->tracker->Document()| and when we do, we leave the restyle bits intact on the element, even though up in ProcessOneRestyle we'll clear mRestyleRoots.  When we re-insert the element, it will have restyle bits (and for our crash here it must have a root bit) but no entry in mRestyleRoots.  If we then restyle a descendant of this element, we'll find it when searching for a restyle root and find no data for it in mRestyleRoots.

There are couple of places that end up clearing restyle bits on the element that we removed, avoiding ending up in this inconsistent state:

(1) in Element::BindToTree, we'll clear the restyle flags if we just inserted into a document:

  http://mxr.mozilla.org/mozilla-central/source/dom/base/Element.cpp#1466

but this doesn't happen if we're inserting into a shadow tree, which currently isn't considered as an insertion into the document (though I think Boris and William were thinking of changing that?)

(2) even if we do insert into a shadow tree, if that causes frames to get constructed, we'll clear the restyle flags here:

  http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9812

So to trigger the crash the element that we remove and reinsert must be display:none and it must be inserted into a shadow tree.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Attached patch patchSplinter Review
I have no idea if the other flags that are cleared in the |if (aDocument)| branch make sense to be cleared when inserting into a shadow tree too.
Attachment #8520423 - Flags: review?(bzbarsky)
Flags: needinfo?(dbaron)
Comment on attachment 8520423 [details] [diff] [review]
patch

I think you should definitely clear NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES.  It probably doesn't matter too much on the FORCE_XBL_BINDINGS bit, but clearing it would be more consistent.
Attachment #8520423 - Flags: review?(bzbarsky) → review+
Blocks: 1062732
OS: Linux → All
Version: 36 Branch → 35 Branch
Comment on attachment 8520423 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1062732
[User impact if declined]: crashes with certain restyling operations
[Describe test coverage new/current, TBPL]: tested manually, tbpl link above, just landed on inbound
[Risks and why]: low, small change
[String/UUID change made/needed]: N/A
Attachment #8520423 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9b77a97a378b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8520423 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.