1240984 - Large transferables (worker, postMessage) cause OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[soeren]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.71 Safari/537.36

Steps to reproduce:

Transferables (i.e., ArrayBuffers that are moved [not copied] into a worker's address space) are prone to cause an OOM in Firefox, where other browsers happily continue to work. Basically, even transferring a moderately large ArrayBuffer object (e.g., 32MB) a few times will deplete the JS heap and cause the OOM to be thrown.  


Actual results:

Instead of transferring the ArrayBuffer, an OOM is thrown. I must presume that there is a memory leak in the code that does actually copy the ArrayBuffer, instead of just moving it. 


Expected results:

All other browsers I tested (Chrome and Safari on OSX, Edge in Windows 10) do happily transfer ArrayBuffers into and out of workers arbitrarily often without throwing an OOM. For the purpose of easily reproducing the issue, I have created an example code: https://github.com/sbalko/firefox-transferables-oom

Comment 1

[michalcharemza]

I can confirm issue occurs in

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0

I cannot reproduce it on Mozilla/5.0 (Windows NT 6.3; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0

Comment 2

[michalcharemza]

Using mozregression, the issue occurred at some point in 099d6cd6725e

Comment 3

[michalcharemza]

I have converted the provided test case to use a SharedWorker, and the same issue occurs.

Comment 4

[michalcharemza]

Looking into about:memory when the supplied test case runs, at 099d6cd6725e and with the current nightly, about:memory shows a large amount of ArrayBuffer used

│  │  │  │  │  ├──2,432.00 MB (98.49%) -- class(ArrayBuffer)
│  │  │  │  │  │  ├──2,432.00 MB (98.49%) -- objects
│  │  │  │  │  │  │  ├──2,432.00 MB (98.49%) ── malloc-heap/elements/non-asm.js
│  │  │  │  │  │  │  └──────0.00 MB (00.00%) ── gc-heap
│  │  │  │  │  │  └──────0.00 MB (00.00%) ++ shapes/gc-heap

And before this, specifically at changeset 4a71f48a4e2c, far less seems to be used whenever I measure

│  │  │  ├──160.19 MB (45.63%) -- classes
│  │  │  │  ├──160.00 MB (45.57%) -- class(ArrayBuffer)
│  │  │  │  │  ├──160.00 MB (45.57%) -- objects
│  │  │  │  │  │  ├──160.00 MB (45.57%) ── malloc-heap/elements/non-asm.js
│  │  │  │  │  │  └────0.00 MB (00.00%) ── gc-heap
│  │  │  │  │  └────0.00 MB (00.00%) ++ shapes/gc-heap

...

│  │  │  │  ├──128.00 MB (36.46%) -- class(ArrayBuffer)
│  │  │  │  │  ├──128.00 MB (36.46%) -- objects
│  │  │  │  │  │  ├──128.00 MB (36.46%) ── malloc-heap/elements/non-asm.js
│  │  │  │  │  │  └────0.00 MB (00.00%) ── gc-heap
│  │  │  │  │  └────0.00 MB (00.00%) ++ shapes/gc-heap

Comment 5

[Arnaud Bienner]

I can't reproduce the problem here on Linux (Ubuntu 14.04) nor on OS X, both using the latest stable release (44)

Comment 6

[michalcharemza]

Not sure if you're running 32 or 64 bit, but the OOM would only occur on 32 bit builds. When I used the 64 bit one it seemed to use a lot of memory for the ArrayBuffers (as in the above snippet from about:memory), but never threw the OOM.

Comment 7

[michalcharemza]

Just tested on 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:44.0) Gecko/20100101 Firefox/44.0

And although I don't get an OOM, according to about:memory, the ArrayBuffer usage grows to ~2GB, before it (I assume) gets garbage collected, drops, and then grows back up.

Comment 8

[Arnaud Bienner]

(In reply to michalcharemza from comment #6)
> the OOM would only occur on 32 bit builds.

That totally makes sense, my bad.

Comment 9

[soeren]

Hi Mozilla,

the more constrained heap size in 32-bit builds in AFAICS a separate "issue" (actually, a non-issue, if you consider restricting the heap size to ~1GB acceptable when asm.js now makes porting games and other code bases to JS quite possible).

What we see in this case looks like a memory leak or at least like a race condition between the allocator and the garbage collector. What we do not understand is why new memory gets allocated in the first place. Isn't this what "transferables" were supposed to make redundant? It looks like the ArrayBuffers actually get copied with new memory being allocated for that purpose with each postMessage.

Any thoughts? We have had to put various workarounds in place to make this issue less likely to occur. Essentially, we avoid using transferables on Firefox for the time being.

Thank you!
Soeren

Comment 10

[michalcharemza]

In case anyone is interested, there is a bounty on this bug at https://www.bountysource.com/issues/30497691-large-transferables-worker-postmessage-cause-oom

Comment 11

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

All this npm + gulp craziness never works for me. I was able to reproduce by checking out https://github.com/sbalko/firefox-transferables-oom and then running python -mSimpleHTTPServer within it.

This *might* be a side effect of a security bug workaround that should no longer be needed. But if so, then I would expect this to be an address space exhaustion OOM with lots of decommitted memory. That is not what I see in my 64-bit about:memory, so something weird is happening.

I will look into this. I wish I'd seen it sooner.

Comment 12

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

Attachment: Remove dummy ArrayBufferContents backstop

Yes, removing the memory backstop seems to fix the problem in my quick testing.

The patch is not final; right now it just clears the low bit of mozPoisonValue() and I'd like something a little more principled to leave in the detached data pointer.

Comment 13

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

Attachment: Remove dummy ArrayBufferContents backstop

Comment 14

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

Attachment: Ignore the DetachDataDisposition argument

Comment 15

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

Attachment: Remove the same-data/change-data distinction when detaching ArrayBuffers

Comment 16

[michalcharemza]

I've tried the test case with the supplied patches, and cannot reproduce the issue.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 8726038 [details] [diff] [review]
Remove dummy ArrayBufferContents backstop

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

To sum up: missing null-check, hasStealableContents() needs changing, and you need to guard that memcpy now that it may be copying from a null pointer.

::: js/src/vm/ArrayBufferObject.cpp
@@ +721,5 @@
>      if (hasStealableContents) {
> +        // Return the old contents and reset the detached buffer's data
> +        // pointer. This pointer should never be accessed.
> +        BufferContents newContents(nullptr);
> +        buffer->setOwnsData(DoesntOwnData); // Do not free the stolen data.

Hoo boy.  I had this long comment written expressing concern that disclaiming the data like this would result in a leak if ABO::detach failed.  Then I started writing a testcase, but I got in far enough to realize: ABO::detach only fails for asm.js/wasm-ified buffers, which are non-malloced and so are not considered stealable by any callers of this method.

I have memories of being profoundly dissatisfied with the flow of the detachment algorithms, when working on them in the past.  I guess each new generation of me is doomed to repeat this process of rediscovery and ensuing malaise.

There has to be a better way than this.

@@ +730,5 @@
>      }
>  
>      // Create a new chunk of memory to return since we cannot steal the
>      // existing contents away from the buffer.
> +    BufferContents newContents = AllocateArrayBufferContents(cx, buffer->byteLength());

You need to add/preserve:

    if (!newContents)
        return BufferContents::createPlain(nullptr);

here.

@@ +735,1 @@
>      memcpy(newContents.data(), oldContents.data(), buffer->byteLength());

The structured-clone path into this code, written as you have it, would trigger undefined behavior, in the case where a detached ArrayBuffer is being transferred.  That is, imagine:

  var ab = new ArrayBuffer(8);
  var toTransfer =
    Object.defineProperty([ab, null],
                          1,
                          { get() { detachArrayBuffer(ab); return null; },
                            enumerable: true });

and then trying to transfer |toTransfer|.  The ArrayBuffer when encountered could (wrongly, see bug 919259) pass the not-detached gauntlet, if fed through it correctly, and so would induce a transfer attempt.

So then we'd end up in ABO::stealContents.  We would have |!hasStealableContents| because the buffer would be |isDetached()|.  (Incidentally, you need to update |ABO::hasStealableContents()| as part of this patch.)  Detaching zeroes out byte length, so we'd try to allocate contents of length zero.  (Permitted, of course, because |new ArrayBuffer(0)| is a perfectly cromulent thing.  But frowny-face.)  *Then* we would try to copy data from the old contents to new.  BUT, that copies from nullptr.

*Even though* it's copying zero bytes, that's still undefined behavior.  And some compilers do make assumptions about non-nullness of arguments passed to memcpy/memmove:

https://gcc.gnu.org/gcc-4.9/porting_to.html

So this needs to be guarded by a byte-length check.

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 8726040 [details] [diff] [review]
Ignore the DetachDataDisposition argument

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +720,5 @@
>  
>      if (hasStealableContents) {
>          // Return the old contents and reset the detached buffer's data
>          // pointer. This pointer should never be accessed.
> +        auto newContents = BufferContents::createPlain(nullptr);

This change should be in the first patch, right?  Seems like the first patch alone shouldn't compile, if this change had to happen (and it appears it does have to happen).

@@ +1173,2 @@
>          ArrayBufferObject::BufferContents newContents =
>              AllocateArrayBufferContents(cx, buffer->byteLength());

This looks wrong to me.  Isn't this allocating duplicate space to what's in the ABO already, exactly the thing we want to avoid?  This looks like it should be fixed in the first patch.

Really, I guess you probably should smash the first two patches together and do one patch that *really* fixes this.  (That was my intuition on first seeing you had two parts.)

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 8726041 [details] [diff] [review]
Remove the same-data/change-data distinction when detaching ArrayBuffers

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

One out of three ain't bad, right?

I see a bunch of file names here containing "neuter" -- could you rename them to "detach" (being careful to make "detach"'s object be an ArrayBuffer, *not* a view of one, e.g. "neutertypedobjsizedarray" where really it's the typed object's underlying buffer that becomes detached)?  Separate patch and bug.  I guess I forgot about that when doing the detach-rename recently.  :-\

::: js/src/builtin/TestingFunctions.cpp
@@ +2154,5 @@
>      if (!JS_ValueToObject(cx, args[0], &obj))
>          return false;
>  
>      if (!obj) {
>          JS_ReportError(cx, "detachArrayBuffer must be passed an object");

Rather than JS_ValueToObject, how about testing args[0].isObject() and then assigning &args.toObject() if so?  Mini-cleanup, close enough to this patch's code to just do here.

::: js/src/jit-test/tests/ion/inlining/typedarray-data-inlining-neuter-samedata.js
@@ +4,5 @@
>  var INLINABLE_INT8_AMOUNT = 4;
>  
>  // Large arrays with non-inline data
>  
> +// Detaching

As noted on a different test, just get rid of the two "Detaching" comments.

::: js/src/jit-test/tests/ion/inlining/typedarray-length-inlining-neuter.js
@@ +2,5 @@
>  var OUT_OF_LINE_INT8_AMOUNT = 237;
>  
>  // Small and inline
>  
> +// Detaching

Might as well remove this "detaching" comment (and the one below) if it's the only thing really being tested here.

Comment 20

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> Comment on attachment 8726038 [details] [diff] [review]
> Remove dummy ArrayBufferContents backstop
> 
> Review of attachment 8726038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> To sum up: missing null-check, hasStealableContents() needs changing, and
> you need to guard that memcpy now that it may be copying from a null pointer.
> 
> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +721,5 @@
> >      if (hasStealableContents) {
> > +        // Return the old contents and reset the detached buffer's data
> > +        // pointer. This pointer should never be accessed.
> > +        BufferContents newContents(nullptr);
> > +        buffer->setOwnsData(DoesntOwnData); // Do not free the stolen data.
> 
> Hoo boy.  I had this long comment written expressing concern that
> disclaiming the data like this would result in a leak if ABO::detach failed.
> Then I started writing a testcase, but I got in far enough to realize:
> ABO::detach only fails for asm.js/wasm-ified buffers, which are non-malloced
> and so are not considered stealable by any callers of this method.
> 
> I have memories of being profoundly dissatisfied with the flow of the
> detachment algorithms, when working on them in the past.  I guess each new
> generation of me is doomed to repeat this process of rediscovery and ensuing
> malaise.
> 
> There has to be a better way than this.

Ugh, yeah, I just now traced through that stuff too. In the end, I concluded that the bool return value of detach() is causing problems, so I removed it. :-) Specifically, I made detach infallible by moving the wasm check out to the two callers, and deleted the original third caller (fun_transfer) which seems to have lost its body somewhere along the way. (It was left behind when the function was removed from the spec.) That eliminates the impossible-looking failure cleanup.

> @@ +730,5 @@
> >      }
> >  
> >      // Create a new chunk of memory to return since we cannot steal the
> >      // existing contents away from the buffer.
> > +    BufferContents newContents = AllocateArrayBufferContents(cx, buffer->byteLength());
> 
> You need to add/preserve:
> 
>     if (!newContents)
>         return BufferContents::createPlain(nullptr);
> 
> here.

Yep, thanks.

> 
> @@ +735,1 @@
> >      memcpy(newContents.data(), oldContents.data(), buffer->byteLength());
> 
> The structured-clone path into this code, written as you have it, would
> trigger undefined behavior, in the case where a detached ArrayBuffer is
> being transferred.  That is, imagine:
> 
>   var ab = new ArrayBuffer(8);
>   var toTransfer =
>     Object.defineProperty([ab, null],
>                           1,
>                           { get() { detachArrayBuffer(ab); return null; },
>                             enumerable: true });
> 
> and then trying to transfer |toTransfer|.  The ArrayBuffer when encountered
> could (wrongly, see bug 919259) pass the not-detached gauntlet, if fed
> through it correctly, and so would induce a transfer attempt.
> 
> So then we'd end up in ABO::stealContents.  We would have
> |!hasStealableContents| because the buffer would be |isDetached()|. 
> (Incidentally, you need to update |ABO::hasStealableContents()| as part of
> this patch.)  Detaching zeroes out byte length, so we'd try to allocate
> contents of length zero.  (Permitted, of course, because |new
> ArrayBuffer(0)| is a perfectly cromulent thing.  But frowny-face.)  *Then*
> we would try to copy data from the old contents to new.  BUT, that copies
> from nullptr.
> 
> *Even though* it's copying zero bytes, that's still undefined behavior.  And
> some compilers do make assumptions about non-nullness of arguments passed to
> memcpy/memmove:
> 
> https://gcc.gnu.org/gcc-4.9/porting_to.html
> 
> So this needs to be guarded by a byte-length check.

Good catch, thanks.

Ok, I ended up changing a fair amount of additional stuff during my second run through this patch. The hasStealableContents() logic and handling is still pretty lame, but I guess that would be a different patch and I'm not sure exactly what to do with it anyway.

> >      if (hasStealableContents) {
> >          // Return the old contents and reset the detached buffer's data
> >          // pointer. This pointer should never be accessed.
> > +        auto newContents = BufferContents::createPlain(nullptr);
> 
> This change should be in the first patch, right?  Seems like the first patch
> alone shouldn't compile, if this change had to happen (and it appears it
> does have to happen).

Uh... yes? I could've sworn I compiled the patches separately. Maybe I decided to get reel smrat and reorder the patches at some point? I don't know. Anyway, I folded them together.

> @@ +1173,2 @@
> >          ArrayBufferObject::BufferContents newContents =
> >              AllocateArrayBufferContents(cx, buffer->byteLength());
> 
> This looks wrong to me.  Isn't this allocating duplicate space to what's in
> the ABO already, exactly the thing we want to avoid?  This looks like it
> should be fixed in the first patch.

Yes.

Comment 21

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

Attachment: Remove dummy ArrayBufferContents backstop

Folded and fixed.

Comment 22

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> Comment on attachment 8726041 [details] [diff] [review]
> Remove the same-data/change-data distinction when detaching ArrayBuffers
> 
> Review of attachment 8726041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One out of three ain't bad, right?
> 
> I see a bunch of file names here containing "neuter" -- could you rename
> them to "detach" (being careful to make "detach"'s object be an ArrayBuffer,
> *not* a view of one, e.g. "neutertypedobjsizedarray" where really it's the
> typed object's underlying buffer that becomes detached)?  Separate patch and
> bug.  I guess I forgot about that when doing the detach-rename recently.  :-\

bug 1254771

> ::: js/src/builtin/TestingFunctions.cpp
> @@ +2154,5 @@
> >      if (!JS_ValueToObject(cx, args[0], &obj))
> >          return false;
> >  
> >      if (!obj) {
> >          JS_ReportError(cx, "detachArrayBuffer must be passed an object");
> 
> Rather than JS_ValueToObject, how about testing args[0].isObject() and then
> assigning &args.toObject() if so?  Mini-cleanup, close enough to this
> patch's code to just do here.

I'm never sure when to use which. It seems weird to me to require an object in an API, when it seems like most js spec stuff uses ToObject (even when the result is kinda silly).

I mean, I could imagine doing something like Number.prototype.buffer = new ArrayBuffer(8); detachArrayBuffer(4). Which is of course irrelevant, since it has no [[BufferData]] or [[WhateverItsCalled]] attribute, but at least you get an error like "that's the wrong type of object" (so you can tell it did ToObject) instead of "that isn't even an object" (which makes the shell builtins feel different).

But I'll stop whining and just change it.

Comment 23

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

Attachment: Remove dummy ArrayBufferContents backstop

Update: needed to fix a couple of nullptr assertions that no longer hold.

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 8728656 [details] [diff] [review]
Remove dummy ArrayBufferContents backstop

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

::: js/src/jit-test/tests/asm.js/testNeuter.js
@@ +27,1 @@
>                         InternalError);

Uh, that looks like a syntax error to me...

@@ +27,2 @@
>                         InternalError);
>  assertThrowsInstanceOf(() => detachArrayBuffer(buffer, "same-data"),

...and this?

@@ +40,5 @@
>  
>  var buffer = new ArrayBuffer(BUF_MIN);
>  function ffi1()
>  {
> +    assertThrowsInstanceOf(() => detachArrayBuffer(buffer), Error);

Hey, this looks sane.

::: js/src/vm/ArrayBufferObject.cpp
@@ +254,4 @@
>  ArrayBufferObject::detach(JSContext* cx, Handle<ArrayBufferObject*> buffer,
>                            BufferContents newContents)
>  {
> +    MOZ_ASSERT(!buffer->isWasm());

Yum.  Maybe add "buffers used by asm.js/WASM can't be detached" as a reason here.

And/or, maybe we could rename this to detachNonWASM or something, that means callers have to have previously considered (or clearly failed to  whether the buffer was(n't) WASM.  'cause (see later) clearly some callers haven't, right now.

@@ +719,5 @@
> +        // pointer. This pointer should never be accessed.
> +        auto newContents = BufferContents::createPlain(nullptr);
> +        buffer->setOwnsData(DoesntOwnData); // Do not free the stolen data.
> +        ArrayBufferObject::detach(cx, buffer, newContents);
> +        buffer->setOwnsData(DoesntOwnData); // Do not free the nullptr.

These "// Do not free"s are useful documentation, but emblematic of a little rot in a location south of Lapland.  IMO.  Seems like if you have plain data, null shouldn't be affected by ownness.  But I dunno if there's a good way to clean this up.  And certainly it shouldn't delay this patch, to happen.

@@ +731,5 @@
>          return BufferContents::createPlain(nullptr);
> +
> +    if (buffer->byteLength() > 0)
> +        memcpy(contentsCopy.data(), oldContents.data(), buffer->byteLength());
> +    ArrayBufferObject::detach(cx, buffer, oldContents);

Could you justify to me that this method, overall, doesn't need an is-WASM guard anywhere near it?  I see this method called from JS_StealArrayBufferContents, for example, which is permissibly called on any object and any ArrayBufferObject, including an is-WASM ABO.  And the structured-clone case seems like it also might qualify.

It might (might?) be the case that the hasStealableContents arm is safe because all WASM is owned -- but it seems basically impossible that *both* arms of this function are safe.

::: js/src/vm/ArrayBufferObject.h
@@ +237,5 @@
>                                          bool hasStealableContents);
>  
>      bool hasStealableContents() const {
>          // Inline elements strictly adhere to the corresponding buffer.
> +        return ownsData();

Might be we should get rid of the second verb, now that they're the same.  We should regroup when these patches have landed to take a look.

@@ +263,5 @@
>      void changeContents(JSContext* cx, BufferContents newContents);
>  
>      // Detach this buffer from its original memory.  (This necessarily makes
>      // views of this buffer unusable for modifying that original memory.)
> +    static void

Wonder what happens if you put MOZ_WARN_UNUSED_RESULT on a void method...

Typically I think we'd have

  static void detach(JSContext* cx, ...,
                     ...);

for formatting here, no?

Comment 25

[michalcharemza]

Bump + reminder that there is a bounty for this at https://www.bountysource.com/issues/30497691-large-transferables-worker-postmessage-cause-oom. Would be great if this could be finished off and merged in!

Comment 26

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

Someone just made me look at this code, and I saw to my horror that the backstop stuff was still there. Wow, it's been a while.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> Comment on attachment 8728656 [details] [diff] [review]
> Remove dummy ArrayBufferContents backstop
> 
> Review of attachment 8728656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/asm.js/testNeuter.js
> @@ +27,1 @@
> >                         InternalError);
> 
> Uh, that looks like a syntax error to me...
> 
> @@ +27,2 @@
> >                         InternalError);
> >  assertThrowsInstanceOf(() => detachArrayBuffer(buffer, "same-data"),
> 
> ...and this?

Rebase wreckage, it looks like.

> @@ +719,5 @@
> > +        // pointer. This pointer should never be accessed.
> > +        auto newContents = BufferContents::createPlain(nullptr);
> > +        buffer->setOwnsData(DoesntOwnData); // Do not free the stolen data.
> > +        ArrayBufferObject::detach(cx, buffer, newContents);
> > +        buffer->setOwnsData(DoesntOwnData); // Do not free the nullptr.
> 
> These "// Do not free"s are useful documentation, but emblematic of a little
> rot in a location south of Lapland.  IMO.  Seems like if you have plain
> data, null shouldn't be affected by ownness.  But I dunno if there's a good
> way to clean this up.  And certainly it shouldn't delay this patch, to
> happen.

I was not happy about writing that code. And from skimming through, it looks like that last line is unnecessary. But it kinda feels better to me to have detached buffers not own anything. I could imagine a MOZ_ASSERT_IF(isDetached(), !ownsData()) somewhere just for the warm fuzzies.

> @@ +731,5 @@
> >          return BufferContents::createPlain(nullptr);
> > +
> > +    if (buffer->byteLength() > 0)
> > +        memcpy(contentsCopy.data(), oldContents.data(), buffer->byteLength());
> > +    ArrayBufferObject::detach(cx, buffer, oldContents);
> 
> Could you justify to me that this method, overall, doesn't need an is-WASM
> guard anywhere near it?  I see this method called from
> JS_StealArrayBufferContents, for example, which is permissibly called on any
> object and any ArrayBufferObject, including an is-WASM ABO.  And the
> structured-clone case seems like it also might qualify.
> 
> It might (might?) be the case that the hasStealableContents arm is safe
> because all WASM is owned -- but it seems basically impossible that *both*
> arms of this function are safe.

Right now wasm does indeed imply ownsData(). They're born that way, and you can't detach them. So we will always end up in the first branch.

I think you are correct. JS_StealArrayBufferContents could be called on a wasm buffer and assert or do evil things. The hasStealableContents bool param has an extra hasMallocedContents guard on top of the hasStealableContents() test -- but hasMallocedContents is special-cased to return true for a wasm-malloced buffer. :-( And we can't just change that, because although you'd get back a valid duplicate of the original memory, you can't detach the original because it's wasm. Ugh.

It could just return a pointer and silently not detach the buffer. But I think callers currently expect detachment. Especially the structured clone, which is completely and totally bogus right now. I'm going to ask luke what the desired semantics are these days, since it seems like we got to where we are when Odin removed change-heap support.

> ::: js/src/vm/ArrayBufferObject.h
> @@ +237,5 @@
> >                                          bool hasStealableContents);
> >  
> >      bool hasStealableContents() const {
> >          // Inline elements strictly adhere to the corresponding buffer.
> > +        return ownsData();
> 
> Might be we should get rid of the second verb, now that they're the same. 
> We should regroup when these patches have landed to take a look.

Yes, the current state is insanity. I don't know if wasm malloced buffers are supposed to be stealable or not, even.

Comment 27

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

Luke - what are the desired semantics with wasm buffers? What should happen if you try to detach (neuter) one? Is it the same for mapped vs malloced wasm buffers (I hope)?

Comment 28

[Luke Wagner [:luke]]

(Boy, I'd love to see all that backstop stuff go!)

Yes, a wasm ArrayBuffer should not be detachable as is the case today with asm.js ABs (in which we sorta cheat).

Comment 29

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

Attachment: Remove dummy ArrayBufferContents backstop

...and again.

MozReview-Commit-ID: h4oF04rDZn

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 8756550 [details] [diff] [review]
Remove dummy ArrayBufferContents backstop

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

I don't feel horribly good about this, 'cause this code is a morass.  :-(  But I don't think we can get to a nice, clean, clear system except incrementally, so we gotta do this.

::: js/src/vm/ArrayBufferObject.cpp
@@ +264,5 @@
>  ArrayBufferObject::detach(JSContext* cx, Handle<ArrayBufferObject*> buffer,
>                            BufferContents newContents)
>  {
> +    MOZ_ASSERT(!buffer->isWasm(), "WASM/asm.js ArrayBuffers cannot be detached");
> +    MOZ_ASSERT(buffer->hasDetachableContents(), "detaching the non-detachable");

Deep, man.

@@ +724,2 @@
>  {
> +    MOZ_ASSERT(!buffer->isWasm(), "WASM/asm.js ArrayBuffers cannot be detached");

As I noted last time around, this can't be asserted here due to JS_StealArrayBufferContents.  Either leave this as-is and add an explicit is-wasm check to that function, or perform that check here.  I guess I lean to the former, but this stuff is so gnarly I don't have much confidence in anything but a series of patches building on each other getting to a clean place with this stuff.

@@ +1187,4 @@
>      }
>  
> +    if (!buffer->hasDetachableContents()) {
> +        JS_ReportError(cx, "Cannot detach ArrayBuffer");

In the spec, DetachArrayBuffer is idempotent.  I think it's reasonable for us to make it idempotent as well.  Please add

  if (buffer->isDetached())
    return true;

so that it's possible to call this multiple times.

@@ +1288,2 @@
>  
> +    return ArrayBufferObject::stealContents(cx, buffer, forceCopy).data();

Add an is-wasm check and throw an error if that happens, just after the is-detached early exit.

::: js/src/vm/ArrayBufferObject.h
@@ +245,3 @@
>      static BufferContents stealContents(JSContext* cx,
>                                          Handle<ArrayBufferObject*> buffer,
> +                                        bool forceCopy);

This needs to be an enum, in an immediate followup patch.  But not in this one, 'cause I don't want to delay it.

@@ +248,5 @@
>  
> +    // Some types of buffer are simply not willing to be detached, eg Wasm
> +    // buffers.
> +    bool hasDetachableContents() const {
> +        return !isDetached() && (isPlain() || isMapped());

I might split this up across multiple lines, for better debuggability.

@@ +275,5 @@
>  
>      // Detach this buffer from its original memory.  (This necessarily makes
>      // views of this buffer unusable for modifying that original memory.)
> +    static void detach(JSContext* cx,
> +                       Handle<ArrayBufferObject*> buffer, BufferContents newContents);

I'd have guessed the Handle bit would fit on the first line, but maybe it doesn't.

Comment 31

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f9d7f6c7b5d814a95faa0352d82c0506d148336

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2cf760a4668
Remove dummy ArrayBufferContents backstop, r=Waldo

Comment 33

[Iris Hsiao [:ihsiao]]

Hi Steve, I have backed out your commit due to XPCShell failure, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=32281382&repo=mozilla-inbound#L3031 
Please help to check that, thanks

Comment 34

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56faf83bf0c3
Backed out changeset f2cf760a4668 for XPCShell failures

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69dca6366941
Remove dummy ArrayBufferContents backstop, r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8656ba3ad9
Remove the same-data/change-data distinction when detaching ArrayBuffers, r=Waldo

Comment 36

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

Hopefully this will stick.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69dca6366941
https://hg.mozilla.org/mozilla-central/rev/bb8656ba3ad9