Closed Bug 1225618 Opened 9 years ago Closed 9 years ago

Reentrance from TimelineMarker during cycle collection

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

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)

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.
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
(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?
(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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.

Attachment

General

Created:
Updated:
Size: