1289428 - Gray stuff can leak into js::CallJSNative

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I finally (by running the test with a huge repeat count overnight) caught one of the assertion failures from bug 1283634 in a debugger.  This is the one that happens in image/test/mochitest/test_bug399925.html while doing CallJSNative.  We have a stack like so:

#3  0x000000010923462b in js::assertSameCompartment<JS::CallArgs> (cx=0x11be4b000, t1=@0x7fff5fbf7b10) at jscntxtinlines.h:162
#4  0x000000010921dc52 in js::CallJSNative (cx=0x11be4b000, native=0x1090b5a80 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbf7b10) at jscntxtinlines.h:234
#5  0x0000000109203059 in js::InternalCallOrConstruct (cx=0x11be4b000, args=@0x7fff5fbf7b10, construct=js::NO_CONSTRUCT) at Interpreter.cpp:441
#6  0x0000000109203569 in InternalCall (cx=0x11be4b000, args=@0x7fff5fbf7b10) at Interpreter.cpp:498
#7  0x00000001091e1226 in js::Call (cx=0x11be4b000, fval={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5fbf7ad8}, thisv={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x120a0c0a8}, args=@0x7fff5fbf7b10, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x120a0c0a0}) at Interpreter.cpp:517
#8  0x00000001091251c0 in js::Wrapper::call (this=0x10bdb6f20, cx=0x11be4b000, proxy={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7e68}, args=@0x7fff5fbf7e70) at Wrapper.cpp:165
#9  0x00000001090ae7a9 in js::CrossCompartmentWrapper::call (this=0x10bdb6f20, cx=0x11be4b000, wrapper={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7e68}, args=@0x7fff5fbf7e70) at CrossCompartmentWrapper.cpp:329
#10 0x0000000102b0241b in xpc::WaiveXrayWrapper::call (this=0x10bdb6f20, cx=0x11be4b000, wrapper={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7e68}, args=@0x7fff5fbf7e70) at WaiveXrayWrapper.cpp:69
#11 0x00000001090b4356 in js::Proxy::call (cx=0x11be4b000, proxy={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7e68}, args=@0x7fff5fbf7e70) at Proxy.cpp:401
#12 0x00000001090b5b53 in js::proxy_Call (cx=0x11be4b000, argc=2, vp=0x120a0c0a0) at Proxy.cpp:690
#13 0x000000010921dc8d in js::CallJSNative (cx=0x11be4b000, native=0x1090b5a80 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbfa0f8) at jscntxtinlines.h:235
#14 0x0000000109203059 in js::InternalCallOrConstruct (cx=0x11be4b000, args=@0x7fff5fbfa0f8, construct=js::NO_CONSTRUCT) at Interpreter.cpp:441
#15 0x0000000109203569 in InternalCall (cx=0x11be4b000, args=@0x7fff5fbfa0f8) at Interpreter.cpp:498
#16 0x000000010920336d in js::CallFromStack (cx=0x11be4b000, args=@0x7fff5fbfa0f8) at Interpreter.cpp:504
#17 0x00000001091f8372 in Interpret (cx=0x11be4b000, state=@0x7fff5fbfb1f8) at Interpreter.cpp:2873
#18 0x00000001091ed741 in js::RunScript (cx=0x11be4b000, state=@0x7fff5fbfb1f8) at Interpreter.cpp:399
#19 0x0000000109203269 in js::InternalCallOrConstruct (cx=0x11be4b000, args=@0x7fff5fbfb410, construct=js::NO_CONSTRUCT) at Interpreter.cpp:471
#20 0x0000000109203569 in InternalCall (cx=0x11be4b000, args=@0x7fff5fbfb410) at Interpreter.cpp:498
#21 0x00000001091e1226 in js::Call (cx=0x11be4b000, fval={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbaf8}, thisv={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5fbfb3c8}, args=@0x7fff5fbfb410, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfb8b8}) at Interpreter.cpp:517
#22 0x0000000108f013bf in JS_CallFunctionValue (cx=0x11be4b000, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbfbac0}, fval={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbaf8}, args=@0x7fff5fbfb868, rval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfb8b8}) at jsapi.cpp:2793
#23 0x0000000102bc63c4 in nsXPCWrappedJSClass::CallMethod (this=0x122b4d790, wrapper=0x1220cdb00, methodIndex=8, info_=0x11bdc3110, nativeParams=0x7fff5fbfbf60) at XPCWrappedJSClass.cpp:1211
#24 0x0000000102bc4f0c in nsXPCWrappedJS::CallMethod (this=0x1220cdb00, methodIndex=8, info=0x11bdc3110, params=0x7fff5fbfbf60) at XPCWrappedJS.cpp:614
#25 0x0000000101791a4f in PrepareAndDispatch (self=0x120b67ac0, methodIndex=8, args=0x7fff5fbfc080, gpregs=0x7fff5fbfc000, fpregs=0x7fff5fbfc030) at xptcstubs_x86_64_darwin.cpp:122
#26 0x000000010179045b in SharedStub () at nsDebug.h:81
#27 0x00000001037d6981 in mozilla::image::ScriptedNotificationObserver::Notify (this=0x120b67ae0, aRequest=0x1220ce900, aType=6) at ScriptedNotificationObserver.cpp:50
#28 0x0000000103980511 in nsImageLoadingContent::Notify (this=0x122c96fa8, aRequest=0x1220ce900, aType=6, aData=0x0) at ../../../mozilla/dom/base/nsImageLoadingContent.cpp:155

In particular, args.thisv() is a gray object!

The most proximate place args.thisv() was set is in CrossCompartmentWrapper::call (frame 9 in the stack), like so:

321             if (!cx->compartment()->wrap(cx, args.mutableThisv()))
322                 return false;

The result of that wrap() call is that args.argv_[-1] ends up with a Function object (_not_ a wrapper of any sort, note) which is gray.  Presumably, before this point args.argv_[-1] was a cross-compartment wrapper for the Function... and was not gray, because it got checked in frame 13?  How can that happen, exactly?  Shouldn't it have marked its target not-gray?   Unfortunately, I can't tell exactly what we used to have here, because it's the same cx and CallArgs being passed all the way through, so things got stomped on as we went... Is it possible that compartment gcs were involved somehow so we didn't mark the wrapper target?

I do know that frame 17 (the Interpret() call) is the first place where our thisv appears.  Before that point we're working with other things.  In particular, in frame 22 we did a gray check already on all the arguments.

The JS stack to this assertion point is:

0 SpecialPowersCallbackWrapper([xpconnect wrapped imgIRequest @ 0x11bee26e0 (native @ 0x1220ce900)]) ["chrome://specialpowers/content/specialpowersAPI.js":355]
    this = [object Object]

which is not terribly helpful, but also not surprising given the setup in the testcase.

The gray this value is, as I said, a Function.  It's a scripted function, and its JSScript has:

(gdb) p $57->filename()
$58 = 0x121624150 "http://mochi.test:8888/tests/image/test/mochitest/test_bug399925.html"
(gdb) p $57->lineno()
$59 = 74

That would correspond to http://searchfox.org/mozilla-central/rev/cb74fc1327028e13bb1f66817da07ca09e4edcec/image/test/mochitest/test_bug399925.html#74

Anyway, I have this in a debugger for now.  Hopefully the test harness won't come along and kill my debug session...

I am so far at a loss as to what's going on here.  :(

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, I finally caught this in rr, after a few days of running the test.  Now all is clear.

We have a cross-compartment wrapper W wrapping an object O.  At the beginning of our story, both W and O are gray.

Then we start a GC like so:

#7  0x00000f336af068d2 in js::gc::GCRuntime::startGC (this=0x3f6f1a002600, gckind=GC_NORMAL, reason=JS::gcreason::CC_WAITING, millis=0)
    at /home/bzbarsky/mozilla/vanilla/mozilla/js/src/jsgc.cpp:6345
#8  0x00000f336af09b88 in JS::StartIncrementalGC (cx=0x3f6f1a002000, gckind=GC_NORMAL, reason=JS::gcreason::CC_WAITING, millis=0)
    at /home/bzbarsky/mozilla/vanilla/mozilla/js/src/jsgc.cpp:7211
#9  0x00000f3366d930f8 in nsJSContext::GarbageCollectNow (aReason=JS::gcreason::CC_WAITING, aIncremental=nsJSContext::IncrementalGC, 
    aShrinking=nsJSContext::NonShrinkingGC, aSliceMillis=0) at /home/bzbarsky/mozilla/vanilla/mozilla/dom/base/nsJSEnvironment.cpp:1220
#10 0x00000f3366d942e0 in GCTimerFired (aTimer=0x703fbfd0, aClosure=0x24)
    at /home/bzbarsky/mozilla/vanilla/mozilla/dom/base/nsJSEnvironment.cpp:1716

This is using gcreason::CC_WAITING, so GarbageCollectNow doesn't do a full GC.  We mark W white but do not touch the mark bits on O.

Now before we have a chance to mark W, we run some script.  This script ends up using W as a thisv, then we enter the compartment of O, and now we have a gray thisv.

I can answer whatever other questions you want, probably, given that I have this in rr.  But I'm not sure there are any more questions about _what_ happened left.  The question is what we should actually be doing in this situation.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Conceptually, it seems to me there are the following points where we could do stuff here:

1)  When we mark W white, we could also mark O white.  Or something.
2)  When we return O from the JSCompartment::wrap() call, we could unmark it gray.

Any other bright ideas?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky [:bz] from comment #2)
Wrappers can be gray if they have no direct references from JS so I think 2 is the way to go.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Per conversation with terrence, we're going to make #2 happen and add a bunch more asserts to JSCompartment::wrap.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure JSCompartment::wrap never returns a gray object

The basic idea is that we assume the invariant that the arguments are not gray
(terrence and I have verified that existing is never gray, and we're rather
hoping the actual object being wrapped is never gray.  Starting from that
assumption, terrence and I audited all the return paths to ensure that gray
objects are never returned.  We found a few places, generally after crossing
compartments with UncheckedUnwrap, where we could have gray things and inserted
corresponding calls to ExposeObjectToActiveJS.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, I meant to add: Bobby, can you please review the xpconnect bits?  Jon, please do the SpiderMonkey bits.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8776055 [details] [diff] [review]
Make sure JSCompartment::wrap never returns a gray object

Review of attachment 8776055 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on WrapperFactory.cpp

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated a bit based on try results

Comment 9

[Boris Zbarsky [:bzbarsky]]

And of course it turns out we _do_ have gray stuff being passed as "obj" to JSCompartment::wrap... by js::RemapWrapper, called from js::RecomputeWrappers.  Terrence, should we just loosen all the asserts to assert that the return value of wrap() is non-gray only if the input is non-gray or something?

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8776141 [details] [diff] [review]
Updated a bit based on try results

Review of attachment 8776141 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I guess this will need to be updated to handle recomputing gray wrappers.

::: js/src/jscompartment.cpp
@@ +486,5 @@
>  
>      RootedObject wrapper(cx, cb->wrap(cx, existing, obj));
>      if (!wrapper)
>          return false;
> +    MOZ_ASSERT(wrapper == existing || !ObjectIsMarkedGray(wrapper));

Why is this allowed to return a gray object if it uses the existing wrapper?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Looks good, but I guess this will need to be updated to handle recomputing gray wrappers.

What does that mean?  ;)

> Why is this allowed to return a gray object if it uses the existing wrapper?

Because we very carefully pass in a gray existing wrapper right now with dire comments about how we do not want to unmark it gray.  See http://searchfox.org/mozilla-central/rev/480c0269eedd567906bfa098f18f51a9e7fa2e38/js/src/jscompartment.h#974-977 and those constructors are invoked from js::RecomputeWrappers and js::RemapAllWrappersForObject.  The point is that things don't escape in those cases, so terrence and I _think_ this is safe...

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> What does that mean?  ;)

Comment 9 implies that the assertions need to be changed.  Does this refer to something else?

> Because we very carefully pass in a gray existing wrapper right now with
> dire comments about how we do not want to unmark it gray.  See
> http://searchfox.org/mozilla-central/rev/
> 480c0269eedd567906bfa098f18f51a9e7fa2e38/js/src/jscompartment.h#974-977 and
> those constructors are invoked from js::RecomputeWrappers and
> js::RemapAllWrappersForObject.  The point is that things don't escape in
> those cases, so terrence and I _think_ this is safe...

OK, thanks for the explanation.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 8776141 [details] [diff] [review]
Updated a bit based on try results

Review of attachment 8776141 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you add a comment above the different assertion.

::: js/src/jscompartment.cpp
@@ +486,5 @@
>  
>      RootedObject wrapper(cx, cb->wrap(cx, existing, obj));
>      if (!wrapper)
>          return false;
> +    MOZ_ASSERT(wrapper == existing || !ObjectIsMarkedGray(wrapper));

I believe the idea is that navigating a tab can cause us to remap all compartments in a zone at once. Since those compartments generally make up a strongly connected component, we don't want to make them uncollectable to CC if they were otherwise collectable.

I think this would cause us to need a second GC/CC cycle to actually collect navigated or closed tabs, which is probably particularly important during shutdown.

Comment 14

[Terrence Cole [:terrence]]

Note that that comment was actually in reply to comment 10.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Comment 9 implies that the assertions need to be changed.

Oh, I see.

So fundamentally, as far as I can tell the wrapper remapping stuff can pass in gray obj _and_ gray existing.  So we can't just assert that obj is non-gray.

What I _think_ we can assert is the following:

1) MOZ_ASSERT_IF(!existing, !ObjectIsMarkedGray(obj)) at entry.
2) Replace all the places where we assert obj is non-gray with asserts that it's non-gray if !existing.
3) Leave the MOZ_ASSERT(wrapper == existing || !ObjectIsMarkedGray(wrapper)) bit, since I believe it should in fact still hold.

Thoughts?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: With looser asserts

Comment 17

[Terrence Cole [:terrence]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> > Comment 9 implies that the assertions need to be changed.
> 
> Oh, I see.
> 
> So fundamentally, as far as I can tell the wrapper remapping stuff can pass
> in gray obj _and_ gray existing.  So we can't just assert that obj is
> non-gray.
> 
> What I _think_ we can assert is the following:
> 
> 1) MOZ_ASSERT_IF(!existing, !ObjectIsMarkedGray(obj)) at entry.
> 2) Replace all the places where we assert obj is non-gray with asserts that
> it's non-gray if !existing.
> 3) Leave the MOZ_ASSERT(wrapper == existing || !ObjectIsMarkedGray(wrapper))
> bit, since I believe it should in fact still hold.
> 
> Thoughts?

Oh, yes, I suppose so! Uhg, in that case, I believe that your analysis is correct. The |existing| and |!existing| cases are different enough that they could arguably warrant separate functions, but it's probably not worth the code duplication.

Comment 18

[Terrence Cole [:terrence]]

Comment on attachment 8776640 [details] [diff] [review]
With looser asserts

Review of attachment 8776640 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review. I'm fine with landing this, but I think I'm going to try to experiment with splitting these to see how much duplication there is in practice or if we could manage it better.

::: js/src/jscompartment.cpp
@@ +439,5 @@
> +    // want to return gray things from here if !existingArg, so make sure it's
> +    // not.
> +    if (!existingArg) {
> +        ExposeObjectToActiveJS(obj);
> +    }

No {} here per SM style guide.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0981a42d17c4
Make sure JSCompartment::wrap never returns a gray object, except when it returns "existing".  r=bholley,jonco

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0981a42d17c4

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 8776640 [details] [diff] [review]
With looser asserts

Approval Request Comment
[Feature/regressing bug #]: Incremental CC, found by the assertions in bug 1283634.
[User impact if declined]: Intermittent crashes.
[Describe test coverage new/current, TreeHerder]: It's been on TH for a week and no longer hits the assertions.
[Risks and why]: There was a slow trickle of hard-to-exploit UAF crashes in previous branches caused by missing ExposeToActiveJS barriers in a few places. We added an assertion that catches these in bug 1283634. We'd like to uplift the fixes to Aurora to solve the crashes 6 weeks earlier than we otherwise might. The impact is relatively low, but the patches are also extremely simple and low risk. Aurora seems like the right balance here.
[String/UUID change made/needed]: None.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776640 [details] [diff] [review]
With looser asserts

Crash fix, has stabilized on Nightly for a few weeks, Aurora50+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49aad1174d84