neuter(asmJSArrayBuffer, "change-data") hits an assertion

RESOLVED FIXED in Firefox 30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(firefox30 fixed, firefox31 fixed, firefox32 fixed, firefox-esr2430+ fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Clearly we didn't think this through enough:

  function f(stdlib, foreign, heap) {
    "use asm";
    var h = new stdlib.Uint8Array(heap);
    function g() {}
    return g;
  }
  var buf = new ArrayBuffer(4096);
  f(this, {}, buf)();
  neuter(buf, "change-data");

results in:

Assertion failure: !isAsmJSArrayBuffer(), at /home/jwalden/moz/slots/js/src/vm/ArrayBufferObject.cpp:374
Segmentation fault (core dumped)

What to do, what to do.  Maybe just make any attempt to neuter an ArrayBuffer whose contents can't change, with "change-data" as disposition, throw a TypeError?
Doh! Ok, that was dumb.

Yeah, seems like the options are to throw an error or silently ignore the request to change the data pointer. The error is gross in that it exposes internal implementation goop (if you delete "use asm", the behavior changes.)

I guess the explicit throw is still better. I guess.

Do we have any tests that compare output with and without "use asm" directives?
(In reply to Steve Fink [:sfink] from comment #1)
> Do we have any tests that compare output with and without "use asm"
> directives?

There are some differential testing that does it so there must be some in the test suites (jit-tests/tests/asm.js).

However, I don't think it's a problem in this case, because every time we call an asm.js exported function (as it's the case here), we check in the C++ wrapper if the arraybuffer has been neutered, and in this case throw an internal error: http://dxr.mozilla.org/mozilla-central/source/js/src/jit/AsmJSLink.cpp#463-470

Could we just remove the assertion?
To wit: this whole throw-on-neuter-while-running-asm.js issue is a kludge to deal with this neutering corner case.  If we wanted to, we could do the extra work to fix this properly: mprotect the whole buffer so that any access will fault and produce the out-of-bounds value.  The only issue is that we'll need to record all AsmJSHeapAccess's for x86/ARM (currently we don't have to) and additionally record the AsmJSHeapAccess's for constant-address heap accesses (which we currently don't record since they otherwise cannot fault).
(Assignee)

Comment 4

5 years ago
That extra work probably has to happen at some point.  Don't want to do it here, tho.  :-)

On more thinking, given this is technically all "unobservable" behavior, just ignoring change-data for the must-remain-constant case seems not unreasonable to me.  Maybe.  Such internal fugly here.  (Also, JS_NeuterArrayBuffer should be friend API if it's not now -- no real reason for this internal detail, or even for neutering itself as distinct from transferring/stealing, to be public stable-ish API.)
Comment on attachment 8414839 [details] [diff] [review]
Ignore the change-data/same-data argument to neuter() and JS_NeuterArrayBuffer when the buffer being neutered doesn't support the given data disposition

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

Oh, right. Inline buffer data doesn't change the data pointer when neutered. This patch aligns neuter's hasStealableData with JS_NeuterArrayBuffer.
Attachment #8414839 - Flags: review?(sphink) → review+
(Assignee)

Comment 7

5 years ago
Pushed with the assertion in ArrayBufferObject::neuter removed, because it happens one caller disowns elements prior to calling the method (so things are in a partially inconsistent state, which ArrayBufferObject::neuter fixes up):

https://hg.mozilla.org/integration/mozilla-inbound/rev/997a2d66710e
Target Milestone: --- → mozilla32
https://hg.mozilla.org/mozilla-central/rev/997a2d66710e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

5 years ago
Comment on attachment 8414839 [details] [diff] [review]
Ignore the change-data/same-data argument to neuter() and JS_NeuterArrayBuffer when the buffer being neutered doesn't support the given data disposition

I'd like this on aurora to run various trunk-targeted tests against aurora.

The original bug (bug 999755) landed when aurora was trunk.  I might want to backport it and this patch to beta and earlire branches, for the same reasons I want to backport this patch.  This approval request doesn't concern itself with that, but I figured I'd give a heads-up seeing as that may happen soon.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 999755
User impact if declined: inability to run some tests targeted at trunk, against branch builds
Testing completed (on m-c, etc.): on m-c/inbound for a week and a half
Risk to taking this patch (and alternatives if risky): very little, this is a pretty simple/clear patch
String or IDL/UUID changes made by this patch: N/A
Attachment #8414839 - Flags: approval-mozilla-aurora?
status-firefox31: --- → affected
status-firefox32: --- → fixed
Attachment #8414839 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 10

5 years ago
Anyone thinking about pushing this to aurora, hold off.  My last recollection was of posting the aurora-targeted patch here *after* I'd finished it.  But, looking at the patch in my aurora tree right now, it's very slightly different from the one posted here.  I do remember having some fun backporting this and/or bug 999755 to aurora and beta in separate repos, so I may have made some tweaks after attachment here.

Potential actual patch pushed to try here -- if that's fine, I'll push to aurora likely tomorrow:

https://tbpl.mozilla.org/?tree=Try&rev=b5f4cd04c5a7
(Assignee)

Comment 11

5 years ago
...or no, obviously I didn't post this patch, as a patch specifically for aurora, at all.  :-)  I probably decided that the delta between original patch and aurora-targeted patch was small enough to not need to post a special patch exactly for it.  In any case, still hold off on landing.
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/90c1b5961d6b
status-firefox31: affected → fixed
Landed on other branches by way of being rolled into the bug 999755 roll-up patch.
status-b2g-v1.2: --- → fixed
status-b2g-v1.3: --- → fixed
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
status-b2g-v2.0: --- → fixed
status-firefox30: --- → fixed
status-firefox-esr24: --- → fixed
tracking-firefox-esr24: --- → 30+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.