1566678 - Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: test.zip

STR:

1. unpack test.zip
2. using a fuzzing build, a clean profile and the included prefs.js launch the browser
3. open launcher.html
4. wait 30 - 45 seconds

Marking as s-s to be safe.

I can consistently reproduce the issue with a fuzzing debug build.

Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496

0|0|libxul.so|JSObject::setFlags(JSContext*, JS::Handle<JSObject*>, js::BaseShape::Flag, JSObject::GenerateShape)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Shape.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|1496|0x49
0|1|libxul.so|js::SetImmutablePrototype(JSContext*, JS::Handle<JSObject*>, bool*)|hg:hg.mozilla.org/mozilla-central:js/src/vm/JSObject.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|2999|0x12
0|2|libxul.so|js::ForwardingProxyHandler::setImmutablePrototype(JSContext*, JS::Handle<JSObject*>, bool*) const|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Wrapper.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|111|0x10
0|3|libxul.so|js::ForwardingProxyHandler::setImmutablePrototype(JSContext*, JS::Handle<JSObject*>, bool*) const|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Wrapper.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|111|0x10
0|4|libxul.so|SetImmutablePrototype|hg:hg.mozilla.org/mozilla-central:js/src/builtin/TestingFunctions.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|4254|0x11
0|5|libxul.so|CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|448|0x16
0|6|libxul.so|js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|540|0x12
0|7|libxul.so|InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|595|0xd
0|8|libxul.so|Interpret|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|599|0x13
0|9|libxul.so|js::RunScript(JSContext*, js::RunState&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|425|0xb
0|10|libxul.so|js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|787|0x5
0|11|libxul.so|EvalKernel|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Eval.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|340|0x16
0|12|libxul.so|js::DirectEval(JSContext*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Eval.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|453|0x37
0|13|libxul.so|js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/jit/BaselineIC.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|3197|0x2a
...
see attached full log.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: full_log.txt

Comment 2

[Jan de Mooij [:jandem]]

It's calling the setImmutablePrototype testing function on a proxy with ForwardingProxyHandler, and the target is in a different compartment. Boris, this looks like our friend OuterWindowProxy :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, it does; verifying that now, but waiting on builds. Assuming it is, what should actually happen in this case? The obvious options are that either the thing ForwardingProxyHandler::setImmutablePrototype calls should not assert anything about compartments or we should enter the compartment of the underlying object, right? Or just have MaybeCrossOriginObject::setImmutablePrototype throw (or nsOuterWindowProxy::setImmutablePrototype).

Web content can't actually trigger this codepath, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, when we hit the assert we're definitely doing a setImmutablePrototype on a xpc::CrossOriginObjectWrapper for a nsOuterWindowProxy.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

Or just have MaybeCrossOriginObject::setImmutablePrototype throw (or nsOuterWindowProxy::setImmutablePrototype).

That makes sense to me.

Web content can't actually trigger this codepath, right?

Right, as far as I can tell. Proxy::setImmutablePrototype is called by js::SetImmutablePrototype, and the only interesting caller there is JS_SetImmutablePrototype. That's called for globals and WindowNamedPropertiesHandler:

https://searchfox.org/mozilla-central/search?q=symbol:_Z24JS_SetImmutablePrototypeP9JSContextN2JS6HandleIP8JSObjectEEPb&redirect=false

Comment 6

[Boris Zbarsky [:bzbarsky]]

So what I can't figure out is how to write a sane case for this. Which makes it hard to test a fix, because the original steps to reproduce takes tens of minutes to run here.

It seems that I need the following:

1. Two globals that are not same-compartment.
2. A cross-compartment wrapper between them that is not an Xray.
3. The JS testing functions available in one of the globals.

So for example, if I do a mochitest-plain and get the JS testing functions via SpecialPowers, then by the time I call the testing function I'm in the chrome compartment and condition 2 is violated if I am passing in a content window.

If I do a mochitest-chrome, then I can make condition 2 work out, but then all my stuff ends up same-compartment.

I don't even know how the fuzzing testcase ends up getting its hands on testing functions. The only places that do that are shell's NewGlobalObject and Components.utils and WorkerScope... Any idea how that's working?

Comment 7

[Daniel Veditz [:dveditz]]

calling sec-moderate on the belief that web code can't trigger this bug.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't even know how the fuzzing testcase ends up getting its hands on testing functions.

Ah, now I do. It's this block in FinishObjectClassInit:

#ifdef FUZZING
  if (cx->options().fuzzing()) {
    if (!DefineTestingFunctions(cx, global, /* fuzzingSafe = */ true,
                                /* disableOOMFunctions = */ false)) {
      return false;
    }
  }
#endif

so I think this codepath actually can't be reached at all in non-fuzzing builds... and you have to have the fuzzing.enabled pref set.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: test. Needs an --enable-fuzzing debug build to really fail

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1566678. Handle attempts to setImmutablePrototype on a WindowProxy more gracefully. r=jandem

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/37fd68a3d10d0ec85350177152ab9af8425f0d3c
https://hg.mozilla.org/mozilla-central/rev/37fd68a3d10d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting or can it ride the trains? If yes to backporting, which branches are affected?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I wouldn't bother backporting this unless it's causing problems for the fuzzing folks. There's no way to trigger this codepath in a non-fuzzing environment.

Comment 14

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0
Build ID: 20190903094847

I can confirm that on the latest fuzzing debug build, I do not get the "Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496" and that the "target page" tab does not crash anymore (it's in a continuum loading) , but I'm seeing another Assertion failure:
Assertion failure: NS_IsMainThread(), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ClearOnShutdown.h:127

Boris, could you please tell me if this is expected?

Comment 15

[Boris Zbarsky [:bzbarsky]]

It's not expected by me, but it's not really related to this bug either.... If it's consistently reproducible, please file a bug with steps to reproduce (including the link to the exact build involved)?

Comment 16

[Simona Badau, Desktop QA]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

It's not expected by me, but it's not really related to this bug either.... If it's consistently reproducible, please file a bug with steps to reproduce (including the link to the exact build involved)?

Thanks Boris, logged Bug 1579395 to cover that assertion failure.

Verified that the "Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496" is not reproducible on the latest Firefox 70 beta fuzzing version (https://tools.taskcluster.net/index/gecko.v2.mozilla-beta.latest.firefox/linux64-fuzzing-debug). Based on this and on Comment 14, marking this bug as VERIFIED FIXED.