1055842 - js1_8_5/extensions/clone-transferables.js crashes starting with c1ea647e6a70

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

This test consistently crashes after the landing of bug 1037358.  I briefly tried debugging, but my debugger seems to say it crashes inside malloc internals (!) and won't let me backtrace, step, or anything else, I think maybe because some sort of lock is held.  Filing now in case someone else sees the obvious issue here.

Comment 1

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

Started looking into this, but got distracted by a topcrash. What I know so far is that it's a result of unmapping a pointer. Perhaps it was a malloced pointer? Anyway, will keep looking.

Comment 2

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

The problem is that when stealing the contents of a mapped array buffer, the contents returned might be mapped or malloced. Specifically what is happening is that an asmjs arraybuffer, which is mapped, is transferred via structured clone. Since asmjs buffers cannot simply be neutered/detached, they are instead duplicated and left alone, returning freshly-malloced data. But because isMappedArrayBuffer() returned true, the structured clone code erroneously assumes that the stolen pointer is also to mapped data.

Previously, isMappedArrayBuffer() returned false for asmjs buffers, even though they're mapped, because there was a separate bit for "asmjs mapped" vs "other mapped".

Neither the old nor the new approach is really right. The structured clone code should not be guessing what type of memory JS_StealArrayBufferContents is returning. I'll look into making it self-describing.

I guess this hasn't been widely noticed because asm.js buffers aren't commonly transferred?

Comment 3

[Douglas Crosher [:dougc]]

Attachment: hack workaround

sfink's suggested workaround, from irc.

Comment 4

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

Attachment: Communicate ownership of stolen ArrayBuffer contents

JS_StealTransferableContents is still footgunny, but I'd like to get a targeted fix in before thinking about expanding the public API.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8480997 [details] [diff] [review]
Communicate ownership of stolen ArrayBuffer contents

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +786,1 @@
>      } else {

Pre-existing else after return here.

@@ +1167,5 @@
>          return nullptr;
>      }
>  
>      Rooted<ArrayBufferObject*> buffer(cx, &obj->as<ArrayBufferObject>());
> +    return ArrayBufferObject::stealContents(cx, buffer).data();

Maybe add some sort of warning printf or something if the contents kind is "screwball"?

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4122a2e5ec86

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4122a2e5ec86