Last Comment Bug 760904 - "Assertion failure: srcArgs.thisv().isMagic(JS_IS_CONSTRUCTING) || &srcArgs.thisv().toObject() == wrapper"
: "Assertion failure: srcArgs.thisv().isMagic(JS_IS_CONSTRUCTING) || &srcArgs.t...
Status: VERIFIED FIXED
[advisory-tracking-]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla16
Assigned To: Steve Fink [:sfink] [:s:]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
Depends on:
Blocks: 326633 594645 741041
  Show dependency treegraph
 
Reported: 2012-06-02 23:39 PDT by Jesse Ruderman
Modified: 2012-09-23 15:33 PDT (History)
6 users (show)
anthony.s.hughes: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
verified
verified
unaffected


Attachments
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews (2.32 KB, patch)
2012-06-04 14:26 PDT, Steve Fink [:sfink] [:s:]
luke: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Jesse Ruderman 2012-06-02 23:39:06 PDT
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:] 2012-06-04 10:59:36 PDT
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:] 2012-06-04 14:26:43 PDT
Created attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews
Comment 3 Luke Wagner [:luke] 2012-06-04 15:13:19 PDT
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:] 2012-06-05 16:43:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f9aa8d3ac5
Comment 5 Ed Morley [:emorley] 2012-06-06 08:24:43 PDT
https://hg.mozilla.org/mozilla-central/rev/91f9aa8d3ac5
Comment 6 Daniel Veditz [:dveditz] 2012-06-22 11:11:55 PDT
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:] 2012-06-28 09:28:37 PDT
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] 2012-07-02 16:32:37 PDT
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:] 2012-07-03 21:49:08 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/d41d84397116
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 15:23:57 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 15:24:40 PDT
Follow-up question, did we want this testcase in-testsuite?
Comment 12 Steve Fink [:sfink] [:s:] 2012-08-21 15:52:23 PDT
I included an equivalent testcase in the patch, so it's already in.
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 16:15:19 PDT
(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+.

Note You need to log in before you can comment on or make changes to this bug.