Closed
Bug 1225618
Opened 9 years ago
Closed 9 years ago
Reentrance from TimelineMarker during cycle collection
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox42 | --- | wontfix |
firefox43 | --- | affected |
firefox44 | --- | affected |
firefox45 | --- | fixed |
firefox-esr38 | --- | unaffected |
People
(Reporter: mccr8, Assigned: fitzgen)
References
Details
(Keywords: sec-moderate, Whiteboard: [adv-main45+][post-critsmash-triage])
Attachments
(1 file)
8.90 KB,
patch
|
vporof
:
review+
|
Details | Diff | Splinter Review |
Here is what looks to be going on in bug 1224880, based on the stack: something triggers a GC, which calls into CycleCollectedJSRuntime::OnGC(). We're apparently in the middle of an incremental CC, so we want to finish the CC before we start the GC, so we call nsCycleCollector::FinishAnyCurrentCollection(). This triggers the AutoGlobalTimelineMarker, which calls js::SavedStacks::saveCurrentStack(), which tries to create a new object, which ends up triggering a new GC, so we call back into CycleCollectedJSRuntime::OnGC(), which ends up failing to finish the current CC because we are running it nested, causing the assertion. I'm not sure what the right fix here is. It looks like this landed in bug 1141614.
Reporter | ||
Comment 1•9 years ago
|
||
Here's the stack: Assertion failure: IsIdle() || (mActivelyCollecting && mIncrementalPhase != GraphBuildingPhase) (Reentered CC during graph building), at /builds/slave/m-in-l64-d-0000000000000000000/build/src/xpcom/base/nsCycleCollector.cpp:3706 #01: mozilla::CycleCollectedJSRuntime::OnGC(JSGCStatus) [xpcom/base/CycleCollectedJSRuntime.cpp:1308] #02: AutoNotifyGCActivity::AutoNotifyGCActivity [js/src/jsgc.cpp:1612] #03: js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) [js/src/jsgc.cpp:6218] #04: js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) [js/src/jsgc.cpp:6382] #05: js::gc::GCRuntime::startGC(JSGCInvocationKind, JS::gcreason::Reason, long) [js/src/jsgc.cpp:6446] #06: js::gc::GCRuntime::gcIfRequested(JSContext*) [js/src/jsgc.cpp:6679] #07: js::gc::GCRuntime::gcIfNeededPerAllocation(JSContext*) [js/src/gc/Allocator.cpp:39] #08: JSObject* js::Allocate<JSObject, (js::AllowGC)1>(js::ExclusiveContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap, js::Class const*) [js/src/gc/Allocator.cpp:55] #09: JSObject::create(js::ExclusiveContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::ObjectGroup*>) [js/src/jsobjinlines.h:332] #10: NewObject [js/src/jsobj.cpp:669] #11: js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) [js/src/jsobj.cpp:729] #12: js::SavedFrame::create(JSContext*) [js/public/RootingAPI.h:666] #13: js::SavedStacks::createFrameFromLookup(JSContext*, js::SavedFrame::HandleLookup) [js/public/RootingAPI.h:666] #14: js::SavedStacks::getOrCreateSavedFrame(JSContext*, js::SavedFrame::HandleLookup) [js/public/RootingAPI.h:666] #15: js::SavedStacks::insertFrames(JSContext*, js::FrameIter&, JS::MutableHandle<js::SavedFrame*>, unsigned int) [js/src/vm/SavedStacks.cpp:1202] #16: js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, unsigned int) [js/src/vm/Stack.h:1883] #17: JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, unsigned int) [js/src/jsapi.cpp:6197] #18: mozilla::TimelineMarker::CaptureStack() [docshell/base/timeline/TimelineMarker.cpp:52] #19: mozilla::detail::UniqueSelector<mozilla::TimelineMarker>::SingleObject mozilla::MakeUnique<mozilla::TimelineMarker, char const*&, mozilla::MarkerTracingType&>(char const*&&&, mozilla::MarkerTracingType&&&) [mfbt/Pair.h:71] #20: mozilla::TimelineConsumers::AddMarkerForAllObservedDocShells(char const*, mozilla::MarkerTracingType) [mfbt/Pair.h:71] #21: mozilla::AutoGlobalTimelineMarker::AutoGlobalTimelineMarker(char const*, mozilla::detail::GuardObjectNotifier&&) [docshell/base/timeline/AutoGlobalTimelineMarker.cpp:26] #22: nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [mfbt/Maybe.h:386] #23: nsCycleCollector::FinishAnyCurrentCollection() [xpcom/base/nsGZFileWriter.cpp:112] #24: mozilla::CycleCollectedJSRuntime::OnGC(JSGCStatus) [xpcom/base/CycleCollectedJSRuntime.cpp:1308] #25: AutoNotifyGCActivity::AutoNotifyGCActivity [js/src/jsgc.cpp:1612] #26: js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) [js/src/jsgc.cpp:6218] #27: js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) [js/src/jsgc.cpp:6382] #28: js::gc::GCRuntime::startGC(JSGCInvocationKind, JS::gcreason::Reason, long) [js/src/jsgc.cpp:6446]
Assignee | ||
Comment 2•9 years ago
|
||
I don't think it would be too onerous to skip saving a stack for CC markers.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #2) > I don't think it would be too onerous to skip saving a stack for CC markers. I assume this is some kind of JS stack that is being captured. Web content shouldn't be on the stack when we trigger a CC, so there won't be any information there anyways. If there is no JS on the stack, I guess TimelineMarker::CaptureStack() won't do anything? If that's the case, this won't be a real security problem, except for nested event loops when we're probably in trouble anyways.
Assignee | ||
Comment 4•9 years ago
|
||
If there is no JS on the stack, then no SavedFrame objects are created and a nullptr is returned. That is at odds with "js::SavedStacks::saveCurrentStack(), which tries to create a new object, which ends up triggering a new GC", so I guess there must be a nested event loop? Should I continue with disabling stack capture for CC timeline markers?
Reporter | ||
Comment 5•9 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #4) > If there is no JS on the stack, then no SavedFrame objects are created and a > nullptr is returned. Thanks for the confirmation. > That is at odds with "js::SavedStacks::saveCurrentStack(), which tries to > create a new object, which ends up triggering a new GC", so I guess there > must be a nested event loop? In this case, the GC is triggered from js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:416]. Because this is a devtools test, I was figuring that it is probably chrome JS explicitly triggering a GC, but I'm not entirely sure of that. (Whether this can happen from chrome or content is mainly a matter of deciding the security implications of this bug.) > Should I continue with disabling stack capture for CC timeline markers? I think disabling the stack capture is the right thing to do here, even though you will miss out on some information with nested event loops.
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8688652 -
Flags: review?(vporof)
Assignee | ||
Comment 7•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e5e4159a6e7
Comment 8•9 years ago
|
||
Comment on attachment 8688652 [details] [diff] [review] Do not capture stacks for cycle collection timeline markers Review of attachment 8688652 [details] [diff] [review]: ----------------------------------------------------------------- This is sad.
Attachment #8688652 -
Flags: review?(vporof) → review+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dac03558135
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1dac03558135
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Reporter | ||
Comment 12•9 years ago
|
||
This probably only affects running tests in practice, so I don't know that we need to backport it.
status-firefox42:
--- → wontfix
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox-esr38:
--- → unaffected
Keywords: sec-moderate
Updated•8 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [adv-main45+]
Updated•8 years ago
|
Whiteboard: [adv-main45+] → [adv-main45+][post-critsmash-triage]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•