760904 - "Assertion failure: srcArgs.thisv().isMagic(JS_IS_CONSTRUCTING) || &srcArgs.thisv().toObject() == wrapper"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

var w = newGlobal("new-compartment");
var b = (new Int8Array).buffer;
w.DataView(b);

Assertion failure: srcArgs.thisv().isMagic(JS_IS_CONSTRUCTING) || &srcArgs.thisv().toObject() == wrapper, at js/src/jswrapper.cpp:688

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/73b380d3edd8
user:        Steve Fink
date:        Wed Mar 28 14:43:02 2012 -0700
summary:     Bug 741041 - Use UnwrapObjectChecked, and ensure ArrayBufferViews and their buffers are in the same compartment. r=luke

Comment 1

[Steve Fink [:sfink] [:s:]]

Oddly, I am seeing a different error:

Assertion failure: isObject(), at /home/sfink/src/MI-typedarray/js/src/jsapi.h:476

but it's almost the same thing, since the isObject() is being called by toObject() in srcArgs.thisv().toObject().

I think the problem is that my custom machinery for dealing with cross-compartment array buffer views doesn't do the right thing wrt constructor calls vs regular calls. If I change the 3rd line to |new w.DataView(b);| it works.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews

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

I think this can be made non-s-s.

::: js/src/jstypedarray.cpp
@@ +2289,5 @@
>  
> +        // Bug 760904: |this| must be forced to JS_IS_CONSTRUCTING because the
> +        // DataView constructor may have been invoked as a normal call, not a
> +        // constructor.
> +        argv[1].setMagic(JS_IS_CONSTRUCTING);

I generally take bug references in comments to indicate "some awful hack" and so, naturally, I don't like to see them anywhere.  This isn't really an awful hack; it's just appeasing a righteous assertion.  I'd just say:

// Appease 'thisv' assertion in CrossCompartmentWrapper::nativeCall

Comment 4

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91f9aa8d3ac5

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/91f9aa8d3ac5

Comment 6

[Daniel Veditz [:dveditz]]

Does this turn out to be a security problem or just an over-eager assertion? Do we want this patch on Aurora(15) and Beta(14)?

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 741041
User impact if declined: none in opt builds; crash with assertion in debug builds when using a particular pattern of DataView object creation
Testing completed (on m-c, etc.): has been on m-c for quite a while now
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none

Not a security problem. Not exactly an over-eager assertion, either.

Originally, the assertion was stronger but was never tripped because nothing proxied construction. I weakened the assertion so that I could proxy typed array construction, but failed to mark all instances of construction as being construction (that's the JS_IS_CONSTRUCTOR bit).

None of this landed in 14, so it is unaffected.

This *did* land in 15, and it is affected.

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews

[Triage Comment]
New regression in FF15 with a low risk fix, let's land this on Aurora.

Comment 9

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d41d84397116

Comment 10

[u279076]

Confirmed the testcase crashes/asserts 2012-06-02 mozilla-central debug JS shell. Does not crash/assert 2012-06-07 mozilla-central debug JS shell, nor 2012-07-04 mozilla-aurora debug JS shell.

Comment 11

[u279076]

Follow-up question, did we want this testcase in-testsuite?

Comment 12

[Steve Fink [:sfink] [:s:]]

I included an equivalent testcase in the patch, so it's already in.

Comment 13

[u279076]

(In reply to Steve Fink [:sfink] from comment #12)
> I included an equivalent testcase in the patch, so it's already in.

Thanks Steve. Marking in-testsuite+.