1313810 - Compartment mismatch or Assertion failure: cx->isInsideCurrentCompartment(proto.toObject()), at js/src/vm/ObjectGroup.cpp:470

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, critical)

Description

[Gary Kwong [:gkw] [:nth10sd]]

The following testcase crashes on mozilla-central revision 1561c917ee27 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

// jsfunfuzz-generated
toString = (function z() {
    return function() {
        Promise = evalcx('lazy').Promise;
    };
})();
[].join(this, toString);
// Adapted from randomly chosen test: js/src/tests/ecma_6/Promise/promise-subclassing.js
class SubPromise extends Promise {}
SubPromise.all([new Promise(function(){})]);


Backtrace:

0   js-dbg-64-dm-clang-darwin-1561c917ee27	0x000000010254cf0a js::ObjectGroup::defaultNewGroup(js::ExclusiveContext*, js::Class const*, js::TaggedProto, JSObject*) + 3226 (ObjectGroup.cpp:470)
1   js-dbg-64-dm-clang-darwin-1561c917ee27	0x0000000102341533 js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) + 243 (RootingAPI.h:802)
2   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001026d1b49 CreatePromiseObjectInternal(JSContext*, JS::Handle<JSObject*>, bool) + 217 (jsobjinlines.h:744)
3   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001026d3222 js::PromiseObject::create(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) + 210 (RootingAPI.h:802)
4   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001026d4a91 js::PromiseConstructor(JSContext*, unsigned int, JS::Value*) + 817 (RootingAPI.h:802)
5   js-dbg-64-dm-clang-darwin-1561c917ee27	0x000000010251236d js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 93 (jscntxtinlines.h:240)
/snip

For detailed crash information, see attachment.

Setting s-s because on opt builds, the following message shows:

*** Compartment mismatch 0x7fa0f1d48ac0 vs. 0x7fa0f1d3bce0
Segmentation fault: 11

Comment 1

[Gary Kwong [:gkw] [:nth10sd]]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160716060553" and the hash "a80fdfc128b0c29dc34dd3fc28868252111a5d52".
The "bad" changeset has the timestamp "20160716064053" and the hash "a97ebab940c6ba0f19348c38d17a2193ee75c9ab".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a80fdfc128b0c29dc34dd3fc28868252111a5d52&tochange=a97ebab940c6ba0f19348c38d17a2193ee75c9ab

Till, is bug 1242662 a likely regressor?

Comment 3

[Till Schneidereit [:till]]

> Till, is bug 1242662 a likely regressor?

No, this is caused by bug 911216. I'm not sure why the regression range is off. Here's a simpler test case:

class SubPromise extends newGlobal().Promise {}
new SubPromise(()=>{});

Patch coming up.

Comment 4

[Till Schneidereit [:till]]

Attachment: Properly handle Promise-subclassing with a cross-compartment Promise superclass

Turns out the way xray wrapper handling was set up for Promises didn't work for normal CCWs. I don't think this is s-s: it crashes release builds in a predictable, if unfortunate, way, but shouldn't be exploitable.

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8805768 [details] [diff] [review]
Properly handle Promise-subclassing with a cross-compartment Promise superclass

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

looks good.

as talked in IRC, we could remove wrap and unwrap for needsWrapping==true case, as it's used only in that case, and wrapped value isn't used.
but it's about API design (what should be wrapped and not), we could do in followup bug if necessary.

::: js/src/jit-test/tests/promise/promise-cross-compartment-subclassing.js
@@ +1,2 @@
> +global = newGlobal();
> +OtherPromise = global.Promise;

I'd prefer having var or let there.

Comment 6

[Gary Kwong [:gkw] [:nth10sd]]

(In reply to Till Schneidereit [:till] from comment #4)
> I don't think this is s-s: it crashes release builds in a
> predictable, if unfortunate, way, but shouldn't be exploitable.

Opening up.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc058bf7a5b
Properly handle Promise-subclassing with a cross-compartment Promise superclass. r=arai

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0cc058bf7a5b