1369504 - Crash in js::ErrorObject::create due to failing release assert about being able to access the saved frame stack from the new error object

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P3)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-dafa7f07-ecf7-48f2-89fe-93a850170601.
=============================================================

I came across this odd crash report. There's a very specific set of steps to reproduce, but I couldn't find any existing bug on file for it.

The URL of the crash report is: https://hacks.mozilla.org/2017/05/debugger-html-call-stack-improvements/

The user comment is as follows:
1. Open this page and also open devtools, swap to "Debugger" panel
2. Wait for the page fully loaded, hit "pause" button(at the top of panel's right section)
3. reload page, it may stopped at "ExtensionContent.jsm" file's line 329, codes are "result = Cu.evalInSandbox(this.options.jsCode, context.cloneScope, "latest");"
4. At the "scopes" section, click "<this>",then page crashed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So that's failing the assert in js::AssertObjectIsSavedFrameOrWrapper.

That assert is asserting that the passed-in object is either a SavedFrame object or a wrapper that can be CheckedUnwrapped to get a SavedFrame object.

We're getting here via js::CopyErrorObject which is doing:

    RootedObject stack(cx, err->stack());
    if (!cx->compartment()->wrap(cx, &stack))
        return nullptr;

And then passing "stack" to ErrorObject::create, which calls AssertObjectIsSavedFrameOrWrapper.

OK, so in this case we start off in DebuggerObject::getOwnPropertyDescriptor.  We enter the compartment of "referent" and then do the GetOwnPropertyDescriptor call in that compartment.  That's throwing, presumably.  Then we try to copy that extension to whatever compartment we were in when we entered DebuggerObject::getOwnPropertyDescriptor, and if that compartment doesn't subsume the place where the exception got thrown we'll fail that assert that we're failing, right?

Comment 2

[Jim Blandy :jimb]

Boris, are you still able to reproduce this? I've checked out revision 7b8937970f9c, but when it pauses, it stops on line 47 of the main web page:

46  <script type="text/javascript">
47    window.hacks = {};
48    // http://cfsimplicity.com/61/removing-analytics-clutter-from-campaign-urls

Clicking on <this> in the top frame's scope seems to work.

I was using a debug build, but I'll try a release build.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Jim, I never reproduced this myself.  It may well depend on having certain extensions installed, to trigger the exception when doing GetOwnPropertyDescriptor on the debugger's referent.  In particular, I would expect that the steps above require the referent to be a chrome object while we're debugging a content webpage or some such.

It would probably be simpler to try to create a testcase in shell using globals that do or don't subsume each other and debugger.  The key is to have GetOwnPropertyDescriptor on the referent throw (which means referent is a proxy) and for the compartment where we entered DebuggerObject::getOwnPropertyDescriptor to not subsume the compartment of the referent, so we get an opaque security wrapper around the exception.  That assumes that shell can do opaque wrappers...

Comment 4

[Jim Blandy :jimb]

I can get a different crash out of this in a release build.

(gdb) where 10
#0  0x00007fffe96a5c7c in mozilla::layers::APZCTreeManager::NotifyScrollbarDragRejected(mozilla::layers::ScrollableLayerGuid const&) const (this=<optimized out>, aGuid=...)
    at /home/jimb/moz/dbg/gfx/layers/apz/src/APZCTreeManager.cpp:598
#1  0x00007fffe96a7fcb in mozilla::layers::APZCTreeManager::StartScrollbarDrag(mozilla::layers::ScrollableLayerGuid const&, mozilla::layers::AsyncDragMetrics const&) (this=0x7fffc8b3a400, aGuid=..., aDragMetrics=...) at /home/jimb/moz/dbg/gfx/layers/apz/src/APZCTreeManager.cpp:585
#2  0x00007fffea38cf5d in mozilla::detail::RunnableMethodArguments<mozilla::layers::ScrollableLayerGuid, mozilla::layers::AsyncDragMetrics>::applyImpl<mozilla::layers::IAPZCTreeManager, void (mozilla::layers::IAPZCTreeManager::*)(mozilla::layers::ScrollableLayerGuid const&, mozilla::layers::AsyncDragMetrics const&), StoreCopyPassByConstLRef<mozilla::layers::ScrollableLayerGuid>, StoreCopyPassByConstLRef<mozilla::layers::AsyncDragMetrics>, 0ul, 1ul>(mozilla::layers::IAPZCTreeManager*, void (mozilla::layers::IAPZCTreeManager::*)(mozilla::layers::ScrollableLayerGuid const&, mozilla::layers::AsyncDragMetrics const&), mozilla::Tuple<StoreCopyPassByConstLRef<mozilla::layers::ScrollableLayerGuid>, StoreCopyPassByConstLRef<mozilla::layers::AsyncDragMetrics> >&, mozilla::IndexSequence<0ul, 1ul>) (
    args=..., m=<optimized out>, o=<optimized out>)
    at /home/jimb/moz/dbg/obj-rel/dist/include/nsThreadUtils.h:1084
#3  0x00007fffea38cf5d in mozilla::detail::RunnableMethodArguments<mozilla::layers::ScrollableLayerGuid, mozilla::layers::AsyncDragMetrics>::apply<mozilla::layers::IAPZCTreeManager, void (mozilla::layers::IAPZCTreeManager::*)(mozilla::layers::ScrollableLayerGuid const&, mozilla::layers::AsyncDragMetrics const&)>(mozilla::layers::IAPZCTreeManager*, void (mozilla::layers::IAPZCTreeManager::*)(mozilla::layers::ScrollableLayerGuid const&, mozilla::layers::AsyncDragMetrics const&)) (m=<optimized out>, o=<optimized out>, this=<optimized out>) at /home/jimb/moz/dbg/obj-rel/dist/include/nsThreadUtils.h:1091
#4  0x00007fffea38cf5d in mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::layers::IAPZCTreeManager>, void (mozilla::layers::IAPZCTreeManager::*)(mozilla::layers::ScrollableLayerGuid const&, mozilla::layers::AsyncDragMetrics const&), true, (mozilla::RunnableKind)0, mozilla::layers::ScrollableLayerGuid, mozilla::layers::AsyncDragMetrics>::Run() (this=<optimized out>)
    at /home/jimb/moz/dbg/obj-rel/dist/include/nsThreadUtils.h:1133
#5  0x00007fffe96c85fc in mozilla::layers::APZThreadUtils::RunOnControllerThread(already_AddRefed<mozilla::Runnable>) (aTask=...) at /home/jimb/moz/dbg/gfx/layers/apz/util/APZThreadUtils.cpp:64
#6  0x00007fffea38bf3c in nsBaseWidget::StartAsyncScrollbarDrag(mozilla::layers::AsyncDragMetrics const&) (this=0x7fffcbde6000, aDragMetrics=...) at /home/jimb/moz/dbg/widget/nsBaseWidget.cpp:1887
#7  0x00007fffea681a76 in nsSliderFrame::StartAPZDrag(mozilla::WidgetGUIEvent*) (this=this@entry=
    0x7fffb126c240, aEvent=aEvent@entry=0x7fffffffbc40)
    at /home/jimb/moz/dbg/layout/xul/nsSliderFrame.cpp:1128
#8  0x00007fffea682a99 in nsSliderFrame::StartDrag(nsIDOMEvent*) (this=0x7fffb126c240, aEvent=<optimized out>) at /home/jimb/moz/dbg/layout/xul/nsSliderFrame.cpp:1200
#9  0x00007fffe9e81c53 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) (this=this@entry=0x7fffb126a040, aListener=<optimized out>, aListener@entry=0x7fffb126e130, aDOMEvent=0x7fffbff6d980, aCurrentTarget=<optimized out>, 
    aCurrentTarget@entry=0x7fffb1269d70) at /home/jimb/moz/dbg/dom/events/EventListenerManager.cpp:1146
#10 0x00007fffe9e82397 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (this=0x7fffb126a040, aPresContext=<optimized out>, aEvent=0x7fffffffbc40, aDOMEvent=0x7fffffffb6a0, aCurrentTarget=<optimized out>, aEventStatus=<optimized out>) at /home/jimb/moz/dbg/dom/events/EventListenerManager.cpp:1320

Comment 5

[Jim Blandy :jimb]

> That assumes that shell can do opaque wrappers...

I don't think the shell does any wrappers at all. Perhaps xpcshell would work.

Comment 6

[Jim Blandy :jimb]

Err, no non-trivial wrappers.

Comment 7

[Jim Blandy :jimb]

Can't reproduce on current central. I'll try constructing a test case per comment 3.

Comment 8

[Jim Blandy :jimb]

Okay, the shell can do opaque wrappers now, working on a test case.

Comment 9

[Jim Blandy :jimb]

Okay, I think I've reproduced this crash with a synthetic test case:

#0  0x0000000000aca1d5 in js::AssertObjectIsSavedFrameOrWrapper(JSContext*, JS::Handle<JSObject*>) (cx=0x7ffff6073000, stack=(JSObject * const) 0x7fffed1dc100 [object Proxy])
    at /home/jimb/moz/dbg/js/src/vm/SavedStacks-inl.h:26
#1  0x0000000000d2271e in js::ErrorObject::create(JSContext*, JSExnType, JS::Handle<JSObject*>, JS::Handle<JSString*>, unsigned int, unsigned int, js::ScopedJSFreePtr<JSErrorReport>*, JS::Handle<JSString*>, JS::Handle<JSObject*>) (cx=0x7ffff6073000, errorType=JSEXN_ERR, stack=(JSObject * const) 0x7fffed1dc100 [object Proxy], fileName="@evaluate", lineNumber=3, columnNumber=54, report=0x7fffffff8c40, message="yeah", protoArg=0x0)
    at /home/jimb/moz/dbg/js/src/vm/ErrorObject.cpp:94
#2  0x0000000000b0387e in js::CopyErrorObject(JSContext*, JS::Handle<js::ErrorObject*>) (cx=0x7ffff6073000, err=(js::ErrorObject * const) 0x7fffed1c20a0 [object Error]) at /home/jimb/moz/dbg/js/src/jsexn.cpp:1092
#3  0x0000000000c5b15e in js::ErrorCopier::~ErrorCopier() (this=0x7fffffff8e30, __in_chrg=<optimized out>)
    at /home/jimb/moz/dbg/js/src/proxy/Wrapper.cpp:453
#4  0x0000000000d080c2 in js::DebuggerObject::getOwnPropertyDescriptor(JSContext*, JS::Handle<js::DebuggerObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::PropertyDescriptor>) (cx=0x7ffff6073000, object=(js::DebuggerObject * const) 0x7fffed1d8190 [object Object], id=$jsid("arbitrary"), desc=
  {obj = 0x0, attrs = 0, getter = 0x0, setter = 0x0, value = JSVAL_VOID})
    at /home/jimb/moz/dbg/js/src/vm/Debugger.cpp:10644
#5  0x0000000000d0386f in js::DebuggerObject::getOwnPropertyDescriptorMethod(JSContext*, unsigned int, JS::Value*) (cx=0x7ffff6073000, argc=1, vp=0x7fffecd4d120) at /home/jimb/moz/dbg/js/src/vm/Debugger.cpp:9753
#6  0x0000000000588307 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (cx=0x7ffff6073000, native=0xd036dc <js::DebuggerObject::getOwnPropertyDescriptorMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/jimb/moz/dbg/js/src/jscntxtinlines.h:293
#7  0x0000000000565a0c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (cx=0x7ffff6073000, args=..., construct=js::NO_CONSTRUCT) at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:470
#8  0x0000000000565d62 in InternalCall(JSContext*, js::AnyInvokeArgs const&) (cx=0x7ffff6073000, args=...)
    at /home/jimb/moz/dbg/js/src/vm/Interpreter.cpp:515
#9  0x0000000000565d8c in js::CallFromStack(JSContext*, JS::CallArgs const&) (cx=0x7ffff6073000, args=...)

Comment 10

[Jim Blandy :jimb]

A saved stack is a linked list of SavedFrame objects, all in the compartment in which the stack was captured, even when the stack passes through many compartments of varying privilege levels. Each SavedFrame records the principals of the compartment of the frame it represents, so if you capture a stack in compartment A that includes frames in compartment B, you get SavedFrame objects in A that have recorded the principals of B.

We address these use cases:

1) We can capture a mixed-principals stack in a content compartment, and view it from that compartment, concealing frames content should not see.

2) We can view such a saved stack from chrome code via an Xray wrapper and see the full stack.

We do not address this use case:

3) We cannot capture a frame in a privileged compartment and then use it from a less-privileged compartment.

To address 1), all SavedFrame property accessors compare the caller's principals with those recorded in the SavedFrame, and actually skip younger frames until they find one subsumed by the caller. So these accessors don't operate on the `this` they're given: they find the youngest acceptable `this` and operate on it.

To address 2), we need to somehow let the SavedFrame accessors see that the request originates in a privileged compartment. Simply entering the SavedFrame's own compartment before calling the accessor, as we usually do, obscures the privileges of the original caller. So we treat SavedFrame objects specially in the Gecko wrapper policy:

http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/js/xpconnect/wrappers/XrayWrapper.cpp#88

But when we take a SavedFrame in a privileged compartment and wrap it for use by an unprivileged content compartment, there's no similar special case. This means that when ErrorCopier rewraps the Error's saved stack, it gets an opaque wrapper, which Error::create cannot verify is a valid SavedFrame.

The easiest fix would be for ErrorCopier to simply copy the entire stack into the compartment it's copying the Error into. Then we're using case 1) to handle access checks. But it would be nicer to have a representation for saved stacks that can be shared among compartments arbitrarily.

Comment 11

[Jim Blandy :jimb]

For reference, much of the original work here was done for bug 1117242.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Since the crash volume is low (less than 5 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.