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

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 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

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 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 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.

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.

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

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

(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+.