Closed Bug 1376130 Opened 7 years ago Closed 7 years ago

stylo: frequent crashes in js::Allocate<T>: MOZ_RELEASE_ASSERT(!inUnsafeRegion) ([AutoAssertNoGC] possible GC in GC-unsafe region)

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: johnp, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-7d5a883a-447e-4c93-a2a1-d44fb0170624.
=============================================================
Frame 	Module 	Signature 	Source
0 	libxul.so 	js::Allocate<js::BaseShape, (js::AllowGC)1u> 	js/src/jscntxt.h:582
1 	libxul.so 	js::BaseShape::getUnowned(JSContext*, js::StackBaseShape&) 	
2 	libxul.so 	js::EmptyShape::getInitialShape(JSContext*, js::Class const*, js::TaggedProto, unsigned long, unsigned int) 	
3 	libxul.so 	js::NewObjectWithGivenTaggedProto(JSContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) 	
4 	libxul.so 	JS_NewObjectWithUniqueType(JSContext*, JSClass const*, JS::Handle<JSObject*>) 	
5 	libxul.so 	XPCWrappedNativeProto::Init(nsIXPCScriptable*, bool) 	
6 	libxul.so 	XPCWrappedNativeProto::GetNewOrUsed(XPCWrappedNativeScope*, nsIClassInfo*, nsIXPCScriptable*, bool) 	
7 	libxul.so 	XPCWrappedNative::GetNewOrUsed(xpcObjectHelper&, XPCWrappedNativeScope*, XPCNativeInterface*, XPCWrappedNative**) 	
8 	libxul.so 	XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, bool, nsresult*) 	
9 	libxul.so 	nsXPConnect::WrapNative(JSContext*, JSObject*, nsISupports*, nsID const&, JSObject**) 	
10 	libxul.so 	xpc_NewIDObject(JSContext*, JS::Handle<JSObject*>, nsID const&) 	
11 	libxul.so 	nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject(JSContext*, JSObject*, nsID const&) 	
12 	libxul.so 	nsXPCWrappedJS::GetNewOrUsed(JS::Handle<JSObject*>, nsID const&, nsXPCWrappedJS**) 	
13 	libxul.so 	nsXPCWrappedJSClass::DelegatedQueryInterface(nsXPCWrappedJS*, nsID const&, void**) 	
14 	libxul.so 	nsXPCWrappedJS::QueryInterface(nsID const&, void**) 	
15 	libxul.so 	nsQueryReferent::operator() 	xpcom/base/nsWeakReference.cpp:148
16 	libxul.so 	nsCOMPtr_base::assign_from_helper 	xpcom/base/nsCOMPtr.cpp:117
17 	libxul.so 	nsObserverList::FillObserverArray(nsCOMArray<nsIObserver>&) 	
18 	libxul.so 	nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) 	
19 	libxul.so 	nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) 	
20 	libxul.so 	nsDocument::OnPageHide(bool, mozilla::dom::EventTarget*) 	
21 	libxul.so 	mozilla::image::SVGDocumentWrapper::DestroyViewer() 	
22 	libxul.so 	mozilla::image::SVGDocumentWrapper::~SVGDocumentWrapper 	image/SVGDocumentWrapper.cpp:58
23 	libxul.so 	libxul.so@0x117a597 	
24 	libxul.so 	mozilla::image::VectorImage::~VectorImage 	mfbt/RefPtr.h:40
25 	libxul.so 	libxul.so@0x117a77b 	
26 	libxul.so 	imgRequest::~imgRequest() 	
27 	libxul.so 	imgRequestProxy::~imgRequestProxy() 	
28 	libxul.so 	imgRequestProxy::Release() 	
29 	libxul.so 	PLDHashTable::Iterator::Remove 	xpcom/ds/PLDHashTable.cpp:661
30 	libxul.so 	mozilla::css::ImageValue::~ImageValue 	layout/style/nsCSSValue.cpp:3088
31 	libxul.so 	mozilla::css::ImageValue::~ImageValue 	layout/style/nsCSSValue.cpp:3090
32 	libxul.so 	mozilla::css::URLValueData::Release 	layout/style/nsCSSValue.h:146
33 	libxul.so 	core::ptr::drop_in_place<collections::vec::Vec<style::values::Either<style::values::None_, style::values::generics::image::Image<style::values::generics::image::Gradient<style::values::specified::image::LineDirection, style::values::specified::length::Length, style::values::specified::length::LengthOrPercentage, style::values::generics::position::Position<style::values::specified::position::PositionComponent<style::values::specified::position::X>, style::values::specified::position::PositionComponent<style::values::specified::position::Y>>, style::values::specified::color::RGBAColor>, style::values::generics::image::ImageRect<style::values::specified::NumberOrPercentage>>>>> 	servo/components/style/gecko_bindings/sugar/refptr.rs:250
34 	libxul.so 	servo_arc::Arc<style::shared_lock::Locked<style::properties::declaration_block::PropertyDeclarationBlock>>::drop_slow<style::shared_lock::Locked<style::properties::declaration_block::PropertyDeclarationBlock>> 	/checkout/src/libcore/ptr.rs:66
35 	libxul.so 	servo_arc::Arc<style::shared_lock::Locked<style::stylesheets::style_rule::StyleRule>>::drop_slow<style::shared_lock::Locked<style::stylesheets::style_rule::StyleRule>> 	servo/components/servo_arc/lib.rs:350
36 	libxul.so 	servo_arc::Arc<style::shared_lock::Locked<style::stylesheets::rule_list::CssRules>>::drop_slow<style::shared_lock::Locked<style::stylesheets::rule_list::CssRules>> 	/checkout/src/libcore/ptr.rs:66
37 	libxul.so 	servo_arc::Arc<style::stylesheets::stylesheet::Stylesheet>::drop_slow<style::stylesheets::stylesheet::Stylesheet> 	servo/components/servo_arc/lib.rs:350
38 	libxul.so 	style::gecko::arc_types::Servo_StyleSheet_Release 	servo/components/servo_arc/lib.rs:350
39 	libxul.so 	mozilla::ServoStyleSheetInner::~ServoStyleSheetInner 	layout/style/ServoArcTypeList.h:10
40 	libxul.so 	mozilla::ServoStyleSheetInner::~ServoStyleSheetInner 	layout/style/ServoStyleSheet.cpp:56
41 	libxul.so 	mozilla::StyleSheet::LastRelease() 	
42 	libxul.so 	mozilla::StyleSheet::Release() 	
43 	libxul.so 	nsCycleCollector::CollectWhite() 	
44 	libxul.so 	nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) 	
45 	libxul.so 	nsCycleCollector_collectSlice 	xpcom/base/nsCycleCollector.cpp:4201
46 	libxul.so 	nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) 	
47 	libxul.so 	ICCRunnerFired(mozilla::TimeStamp, void*) 	
48 	libxul.so 	CollectorRunner::Run 	dom/base/nsJSEnvironment.cpp:264
49 	libxul.so 	nsThread::ProcessNextEvent(bool, bool*) 	
50 	libxul.so 	NS_ProcessNextEvent(nsIThread*, bool) 	
51 	libxul.so 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	
52 	libxul.so 	MessageLoop::Run() 	
53 	libxul.so 	nsBaseAppShell::Run 	widget/nsBaseAppShell.cpp:156
54 	libxul.so 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp:896
55 	libxul.so 	MessageLoop::Run() 	
56 	libxul.so 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp:712
57 	firefox 	content_process_main 	ipc/contentproc/plugin-container.cpp:64
58 	firefox 	_init 	
Ø 59 	libc-2.25.so 	libc-2.25.so@0x204d9 	
60 	firefox 	firefox@0x1182f 	
61 	firefox 	firefox@0x1aeef 	
62 	firefox 	firefox@0x1182f 	
63 	firefox 	mozilla::ReadAheadLib(char const*) 	
Ø 64 	ld-2.25.so 	ld-2.25.so@0x1132f 	
65 	firefox 	firefox@0x1aeef 	
66 	firefox 	_start


Another slightly different crash:

This bug was filed from the Socorro interface and is 
report bp-3eafe5ed-849b-4725-9eb6-6c2a50170624.
=============================================================
Frame 	Module 	Signature 	Source
0 	libxul.so 	js::Allocate<JSObject, (js::AllowGC)1u> 	js/src/jscntxt.h:582
1 	libxul.so 	js::NewObjectWithGivenTaggedProto(JSContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) 	
2 	libxul.so 	JS_NewObjectWithGivenProto(JSContext*, JSClass const*, JS::Handle<JSObject*>) 	
3 	libxul.so 	XPCWrappedNative::Init(nsIXPCScriptable*) 	
4 	libxul.so 	XPCWrappedNative::GetNewOrUsed(xpcObjectHelper&, XPCWrappedNativeScope*, XPCNativeInterface*, XPCWrappedNative**) 	
5 	libxul.so 	XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, bool, nsresult*) 	
6 	libxul.so 	nsXPConnect::WrapNative(JSContext*, JSObject*, nsISupports*, nsID const&, JSObject**) 	
7 	libxul.so 	xpc_NewIDObject(JSContext*, JS::Handle<JSObject*>, nsID const&) 	
8 	libxul.so 	nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject(JSContext*, JSObject*, nsID const&) 	
9 	libxul.so 	nsXPCWrappedJS::GetNewOrUsed(JS::Handle<JSObject*>, nsID const&, nsXPCWrappedJS**) 	
10 	libxul.so 	nsXPCWrappedJSClass::DelegatedQueryInterface(nsXPCWrappedJS*, nsID const&, void**) 	
11 	libxul.so 	nsXPCWrappedJS::QueryInterface(nsID const&, void**) 	
12 	libxul.so 	nsQueryReferent::operator() 	xpcom/base/nsWeakReference.cpp:148
13 	libxul.so 	nsCOMPtr_base::assign_from_helper 	xpcom/base/nsCOMPtr.cpp:117
14 	libxul.so 	nsObserverList::FillObserverArray(nsCOMArray<nsIObserver>&) 	
15 	libxul.so 	nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) 	
16 	libxul.so 	nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) 	
17 	libxul.so 	nsDocument::OnPageHide(bool, mozilla::dom::EventTarget*) 	
18 	libxul.so 	mozilla::image::SVGDocumentWrapper::DestroyViewer() 	
19 	libxul.so 	mozilla::image::SVGDocumentWrapper::~SVGDocumentWrapper 	image/SVGDocumentWrapper.cpp:58
20 	libxul.so 	libxul.so@0x117a597 	
21 	libxul.so 	mozilla::image::VectorImage::~VectorImage 	mfbt/RefPtr.h:40
22 	libxul.so 	libxul.so@0x117a77b 	
23 	libxul.so 	imgRequest::~imgRequest() 	
24 	libxul.so 	imgRequestProxy::~imgRequestProxy() 	
25 	libxul.so 	imgRequestProxy::Release() 	
26 	libxul.so 	PLDHashTable::Iterator::Remove 	xpcom/ds/PLDHashTable.cpp:661
27 	libxul.so 	mozilla::css::ImageValue::~ImageValue 	layout/style/nsCSSValue.cpp:3088
28 	libxul.so 	mozilla::css::ImageValue::~ImageValue 	layout/style/nsCSSValue.cpp:3090
29 	libxul.so 	mozilla::css::URLValueData::Release 	layout/style/nsCSSValue.h:146
30 	libxul.so 	core::ptr::drop_in_place<collections::vec::Vec<style::values::Either<style::values::None_, style::values::generics::image::Image<style::values::generics::image::Gradient<style::values::specified::image::LineDirection, style::values::specified::length::Length, style::values::specified::length::LengthOrPercentage, style::values::generics::position::Position<style::values::specified::position::PositionComponent<style::values::specified::position::X>, style::values::specified::position::PositionComponent<style::values::specified::position::Y>>, style::values::specified::color::RGBAColor>, style::values::generics::image::ImageRect<style::values::specified::NumberOrPercentage>>>>> 	servo/components/style/gecko_bindings/sugar/refptr.rs:250
31 	libxul.so 	servo_arc::Arc<style::shared_lock::Locked<style::properties::declaration_block::PropertyDeclarationBlock>>::drop_slow<style::shared_lock::Locked<style::properties::declaration_block::PropertyDeclarationBlock>> 	/checkout/src/libcore/ptr.rs:66
32 	libxul.so 	servo_arc::Arc<style::shared_lock::Locked<style::stylesheets::style_rule::StyleRule>>::drop_slow<style::shared_lock::Locked<style::stylesheets::style_rule::StyleRule>> 	servo/components/servo_arc/lib.rs:350
33 	libxul.so 	servo_arc::Arc<style::shared_lock::Locked<style::stylesheets::rule_list::CssRules>>::drop_slow<style::shared_lock::Locked<style::stylesheets::rule_list::CssRules>> 	/checkout/src/libcore/ptr.rs:66
34 	libxul.so 	servo_arc::Arc<style::stylesheets::stylesheet::Stylesheet>::drop_slow<style::stylesheets::stylesheet::Stylesheet> 	servo/components/servo_arc/lib.rs:350
35 	libxul.so 	style::gecko::arc_types::Servo_StyleSheet_Release 	servo/components/servo_arc/lib.rs:350
36 	libxul.so 	mozilla::ServoStyleSheetInner::~ServoStyleSheetInner 	layout/style/ServoArcTypeList.h:10
37 	libxul.so 	mozilla::ServoStyleSheetInner::~ServoStyleSheetInner 	layout/style/ServoStyleSheet.cpp:56
38 	libxul.so 	mozilla::StyleSheet::LastRelease() 	
39 	libxul.so 	mozilla::StyleSheet::Release() 	
40 	libxul.so 	nsCycleCollector::CollectWhite() 	
41 	libxul.so 	nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) 	
42 	libxul.so 	nsCycleCollector_collectSlice 	xpcom/base/nsCycleCollector.cpp:4201
43 	libxul.so 	nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) 	
44 	libxul.so 	ICCRunnerFired(mozilla::TimeStamp, void*) 	
45 	libxul.so 	CollectorRunner::Run 	dom/base/nsJSEnvironment.cpp:264
46 	libxul.so 	CollectorRunner::TimedOut 	dom/base/nsJSEnvironment.cpp:281
47 	libxul.so 	nsTimerImpl::Fire(int) 	
48 	libxul.so 	nsTimerEvent::Run() 	
49 	libxul.so 	nsThread::ProcessNextEvent(bool, bool*) 	
50 	libxul.so 	NS_ProcessNextEvent(nsIThread*, bool) 	
51 	libxul.so 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	
52 	libxul.so 	MessageLoop::Run() 	
53 	libxul.so 	nsBaseAppShell::Run 	widget/nsBaseAppShell.cpp:156
54 	libxul.so 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp:896
55 	libxul.so 	MessageLoop::Run() 	
56 	libxul.so 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp:712
57 	firefox 	content_process_main 	ipc/contentproc/plugin-container.cpp:64
58 	firefox 	_init 	
Ø 59 	libc-2.25.so 	libc-2.25.so@0x204d9 	
60 	firefox 	firefox@0x1182f 	
61 	firefox 	firefox@0x1aeef 	
Ø 62 	libfreetype.so.6.13.0 	libfreetype.so.6.13.0@0x255fff 	
63 	firefox 	firefox@0x1182f 	
64 	firefox 	mozilla::ReadAheadLib(char const*) 	
Ø 65 	ld-2.25.so 	ld-2.25.so@0x1132f 	
66 	firefox 	firefox@0x1aeef 	
67 	firefox 	_start


I don't have any specific STR, the tab crash just happens seemingly randomly when browsing e.g. YouTube and twitter (50+ times today) since setting layout.css.servo.enabled=true. Disabling the flag makes the crashes go away. (except for some cases where I think the tabs were loaded when stylo was still enabled - if that makes sense)
After some investigating it seems to be that the crash only happens when LastPass (4.1.54a) is enabled in my profile.
STR for fresh profile:

* Install LastPass 4.1.54a: https://addons.mozilla.org/en-US/firefox/addon/lastpass-password-manager/versions/beta
* Visit https://www.youtube.com
* Browse a bit -> crash (tab crash or whole browser crash)
* Result: bp-3d26fb7a-ba7e-4bd1-ad3f-ed9d70170624
Has STR: --- → yes
(STR addendum: I have only been able to reproduce the bug when logged into my LastPass account)
I came here from this crash report and I do not use Lastpass https://crash-stats.mozilla.com/report/index/25cb18a6-daed-4963-ab7b-ad4c90170625
The signature 'js::Allocate<T>' has reappeared with buildid 20170623100152. There are 181 crashes.
:jonco, could you investigate ?
Component: CSS Parsing and Computation → JavaScript: GC
Flags: needinfo?(jcoppeard)
(In reply to Calixte Denizet (:calixte) from comment #5)
> The signature 'js::Allocate<T>' has reappeared with buildid 20170623100152.
> There are 181 crashes.
> :jonco, could you investigate ?

This is a Stylo issue. Stylo is somehow causing SVG to be destroyed, which is notifying observers, which is causing JS to run. I'm not sure why Stylo in particular is triggering this.
Component: JavaScript: GC → CSS Parsing and Computation
Flags: needinfo?(jcoppeard)
We have an assertion that JS can't be called during CollectWhite. I'm not sure how critical that is to anything, but it would be unfortunate to regress this nice property incidentally.
Looking at the stack, it is unclear to me why this only happens in Stylo either. It doesn't seem to me there is anything in Gecko stopping ImageValue from being destroyed when StyleSheet gets destroyed.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8)
> Looking at the stack, it is unclear to me why this only happens in Stylo
> either. It doesn't seem to me there is anything in Gecko stopping ImageValue
> from being destroyed when StyleSheet gets destroyed.

This is happening on the last release of the style sheet, not when the style sheet is destroyed. Because the style sheet is cycle collected, the actual dtor call will happen later.
This is a recent change (bug 1373484) to both Gecko and Stylo.

And yeah, it is on the last release. But on the last release, there doesn't seem to be anything in Gecko to stop that as well, so in theory this should happen to Gecko as well.

Is CollectWhite called only when the object is cycle collected? IIUC, if the object is not cycle referenced, LastRelease should be called immediately at the last Release call.
I'm still not able to reproduce this crash with a testcase.

What I did is:
* add an observer in script for content-page-hidden
* load a document with SVG image referenced in a stylesheet from a <style>
* remove the <style> element

I can see that observer is triggered for the SVG document, but it doesn't crash.

One thing I noticed is that, the observer is triggered for the SVG document significantly later than the document which references it, and neither CC nor GC can trigger that, which probably means we are keeping it in some cache. If we always keep it in some cache, presumably it shouldn't be released from stylesheet release at all I suppose.
Assignee: nobody → xidorn+moz
Priority: -- → P1
OK, so I have a STR now. Thanks for dholbert's help.

1. add a script observer to content-page-hidden. I added one in browser.js.
2. open a page which has SVG referenced from stylesheet, and the image is actually used in the document.
3. press Ctrl-Shift-Del and clear the cache
4. then close the page

And why it is different than Gecko is because, in Gecko, StyleRule holds the image value, and StyleRule is somehow always destroyed by SnowWhiteKiller, while in Stylo, StyleSheet holds the values via Servo side Stylesheet somehow, and that is destroyed by some other thing, which can trigger this crash.
So there are two ideas how we can fix this:
1. Don't notify that observer (and probably many other things) for SVG-as-image (which can be checked via nsIDocument::mIsBeingUsedAsImage)
2. Put Servo_StyleSheet_Release into a runnable and make it run asynchonrously

I guess actually it's worth doing both.

For the first one, it doesn't seem to make much sense to trigger lots of such things for a document which is used as image. That could be annoying for their observers as well, and they can potentially trigger similar crash in other places.

For the second, there is nothing guarantees that we wouldn't have other places trigger this kind of issues.
Why do we want the patch? Why we want to special case some type of documents?
Feels odd that we dispatch pageshow/hide events but don't fire the notification. I don't like that inconsistency.
Comment on attachment 8882627 [details]
Bug 1376130 - Don't notify page-{shown,hidden} observers or dispatch pageshow/pagehide events when the document is an SVG used as image.

https://reviewboard.mozilla.org/r/153708/#review158882

So, this feels like a hack, which makes pagehide/show inconsistent. Could we do the other approach?
Attachment #8882627 - Flags: review?(bugs) → review-
In general, I don't think anyone should be able to access document inside SVG-as-image. If nothing is able to access the document, nothing can hook an event handler on it.

I wonder whether there is a general way we can fully avoid the document to be accessed.
Don't we fire a notification when a document is created or document element added to it.
It seems this is not the first time we hit OnPageHide for SVG-as-image document in some weird place. See also bug 1309960 which happens when we tried doing GC and painting in parallel. This time, we hit this inside CC.

I guess this case can happen for Gecko in some cases as well, just more tricky. Actually, the current design of Gecko isn't meant to protect from this case at all. It just happens to not crash. (It is also released from CC according to the stack, but somehow not in a path which GC is forbidden.)

Basically, places handling image don't usually take script into account, because in general that doesn't happen. That only happens when it is an SVG-as-image, and some addon tries to add observer of content-page-hidden.

It is also tricky to write test for this kind of crash, because the ownership of related objects is kind of complicated. That means we are usually aware of this kind of crash only after it actually hits some people, which doesn't sound very helpful.

We really should avoid notifying that.

If you are concerned about the inconsistency between the notification and the pageshow/pagehide event, I can move them into the if-block as well. Are you fine with that?
See Also: → 1309960
That sounds better yes.
Comment on attachment 8882627 [details]
Bug 1376130 - Don't notify page-{shown,hidden} observers or dispatch pageshow/pagehide events when the document is an SVG used as image.

https://reviewboard.mozilla.org/r/153708/#review159090
Attachment #8882627 - Flags: review?(bugs) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/00be20066a63
Don't notify page-{shown,hidden} observers or dispatch pageshow/pagehide events when the document is an SVG used as image. r=smaug
https://hg.mozilla.org/mozilla-central/rev/00be20066a63
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1309960

Copying crash signatures from duplicate bugs.

Crash Signature: [@ js::Allocate<T>] → [@ js::Allocate<T>] [@ js::BarrierMethods<T>::exposeToJS]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: