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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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?

Comment 1

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

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?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 3

[Luke Wagner [:luke]]

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

Comment 4

[Jeff Walden [:Waldo]]

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 5

[Jeff Walden [:Waldo]]

Attachment: Ignore the change-data/same-data argument to neuter() and JS_NeuterArrayBuffer when the buffer being neutered doesn't support the given data disposition

Comment 6

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

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.

Comment 7

[Jeff Walden [:Waldo]]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/997a2d66710e

Comment 9

[Jeff Walden [:Waldo]]

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

Comment 10

[Jeff Walden [:Waldo]]

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

Comment 11

[Jeff Walden [:Waldo]]

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

Comment 12

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90c1b5961d6b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Landed on other branches by way of being rolled into the bug 999755 roll-up patch.