Reentrance from TimelineMarker during cycle collection

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mccr8, Assigned: fitzgen)

Tracking

(Blocks 1 bug, {sec-moderate})

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 affected, firefox44 affected, firefox45 fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [adv-main45+][post-critsmash-triage])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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

4 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]
I don't think it would be too onerous to skip saving a stack for CC markers.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
(Reporter)

Comment 3

4 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.
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

4 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.
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+
Thanks, Victor!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1dac03558135
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Comment 12

3 years ago
This probably only affects running tests in practice, so I don't know that we need to backport it.
Group: core-security → core-security-release
Whiteboard: [adv-main45+]
Whiteboard: [adv-main45+] → [adv-main45+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.