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

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Waldo, Assigned: sfink)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: nobody → sphink
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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?
Created attachment 8480985 [details] [diff] [review]
hack workaround

sfink's suggested workaround, from irc.
(Assignee)

Comment 4

4 years ago
Created attachment 8480997 [details] [diff] [review]
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.
Attachment #8480997 - Flags: review?(jwalden+bmo)
(Reporter)

Comment 5

4 years ago
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"?
Attachment #8480997 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/4122a2e5ec86
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.