1529298 - Make ArrayBuffer's handling of kinds and data-ownership sane and simple and understandable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

The complexity has sprawled. I fear to review or touch the code because I can't understand it enough to safely touch it. Bug 1502562 was the last straw, where like a five-line patch I felt safer/more confident not taking and leaking, than taking and not leaking but maybe terrible things happen too?

So, have a gonzo patch stack of mostly a zillion smallish transformation, only a few of which actually change how the world functions, and in the end we have an understandable system.

sfink has volunteered as tribute. Or been volunteered; I forget which.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Add an extra bit to ArrayBufferObject::BufferKind's KIND_MASK to permit adding additional buffer kinds beyond the current four

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Change the comment by ArrayBufferFlags's FOR_ASMJS to indicate that buffers prepared for asm.js must be PLAIN, MAPPED, or EXTERNAL

This becomes clearer in what prepareForAsmJS does and doesn't mark, at some distant point in this patch stack, but for now a comment on this seems helpful.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Add a BufferContents::createFailed() to use in situations where a failure occurs, rather than overloading createPlain(nullptr)

I'm not sure I like the idea of BufferContents as a thing to signal failure, but it exists, we do have places that need to return a data pointer and one of a few different kinds, so it's not absolutely terrible.  Meanwhile, having a thing for creating failure instances of it seems useful for calling out that possibility.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Split PLAIN into PLAIN_DATA and USER_OWNED ArrayBuffer data types to clearly segregate the two, rather than categorizing them both as the same thing

So the problem with the existing kinds is they act at cross purposes with OwnsData/DoesntOwnData.  Really we want kind to identify ownership.  Splitting PLAIN up a few obvious ways is a step in that direction.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Rename JS_NewArrayBufferWithExternalContents to JS_NewArrayBufferWithUserOwnedContents to better accord with the USER_OWNED ArrayBufferKind name, and to avoid namespace overload with the semantically distinct JS_NewExternalArrayBuffer and JS_ExternalizeAr

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Split PLAIN into INLINE_DATA/MALLOCED for ArrayBuffer kinds

Obviously inline data is either never "owned", or always "owned".  So it having its own kind is a no-brainer.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Move most of JS_ExternalizeArrayBufferContents into a static member function on ArrayBufferObject so that internals-observing code isn't smeared across two functions

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity

jorendorff is thumbs-down on this API:

https://mozilla.logbot.info/jsapi/20190213#c15962840

Ix-nay.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Remove ArrayBufferObject::hasData and perform its operation in its sole caller in a more straightforward manner

This made sense when the prototype was an ArrayBuffer.  It isn't now.  This doesn't make sense now.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Remove trailing arguments to ArrayBufferObject::createForContents that are identical for every caller

Detritus from when there was just one function that was a floor wax and a dessert topping and a Swiss army knife and a footgun.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Make AllocateArrayBufferContents return uint8_t*, and make its callers consistently not redundantly report OOMs

I tried reading through the callocCanGC implementation earlier, and I would have sworn to it reporting OOM on failure.  But it apparently does not.

We could rathole on whether OOM failure should just stop execution in some of these callers as this patch makes it, or fail-without-OOM handled more gracefully is better.  I argue this excess complexity does not pay off to have to think about.

Moreover, at the end of all the patches here, the only two callers of AABC basically are in functions that really should have OOM reporting by default, so this is the way to go.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Only pass BufferContents containing a non-null pointer to |ArrayBufferObject::createForContents|

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Simplify ArrayBufferObject::createForContents some now that it's only ever passed non-null contents

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Consistently use "ArrayBuffer" instead of the vaguer "array buffer" in ArrayBuffer-related JSAPI descriptions

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Add BufferContents::createFoo functions for every kind so that the templaty, harder-to-search-for create function can be removed

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Remove ArrayBufferOffset::offsetOfFlags as unused

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Add BufferKind::NO_DATA for ArrayBuffers that have no data (whether because byteLength is zero or because the ArrayBuffer is detached), for which the value of |ownsData()| is irrelevant

So NO_DATA will ultimately get used in two places:

* for ArrayBuffers that become detached
* for wasm ArrayBuffers created by a memory.grow operation, whose contents are then overwritten by an original buffer's data after successful size increase

Rather than overload malloc'd data and allow such data to be null, or making these users be INLINE_DATA at cost of more onerous initialization, just add another kind for null data.  Easy.  Simple.

|ownsData()| and all related code will be very gradually phased out as this patch stack proceeds; I'm not sure just how much I made NO_DATA here truly owns-agnostic.  Don't sweat it too hard when reviewing, it all comes together in the end.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Consistently initialize all ArrayBuffers that store their data inline

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Remove |ArrayBufferObject::hasStealableContents()| and replace it with its contents, appropriately simplified for each calling location

This function is a huge part of the horror of the current code.  That, and the |bool hasStealableContents| argument to the |stealContents| function.  The replacements here do not necessarily make things as absolutely clearer as you'd want.  But it's a step in the right direction.  Small steps in small patches.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Remove most uses of DoesntOwnData for NO_DATA and USER_OWNED and make current users that depend on |ownsData()| not do so

Okay, this starts to get a little tricky, sort of.

The ultimate aim is for |ownsData()| and the OwnsState enum and the flag-bit to all disappear.  I achieve this end by gradually making everything OwnsData.  Once everything is OwnsData, anything that queries that is an opportunity to eliminate dead code.

*Sometimes* the approach for this is to handle a kind identically regardless of ownership.  For example, releaseData is gradually changed to do this.

Other places, you have to fill in some fallback code in the case where |ownsData()| now is true for more-exotic kinds that we could just skate past before.

And further, the code that *creates* these ArrayBuffers for these contents, needs to deal with |ownsData()| no longer being the right test necessarily.

I tried to audit every user of ownsData to deal with this stuff.  There isn't a great way to represent that/make that verifiable in a patch.  The XXX breadcrumbs with explanations are the best I could do, I think.  (By end of patch stack they've all been removed.)

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Make NoteViewBufferWasDetached a lambda instead of a global function

[cx] versus [&cx] is pretty eh in my book.  At this point now I don't even remember which I picked.

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Implement an ArrayBufferObject::stealMallocedContents for use in JS_StealArrayBufferContents, rather than reusing the hoary ArrayBufferObject::stealContents with finicky caller-side should-this-steal logic

Channeling three separate ideas through the same function with the caller passing a boolean to direct the particular mode of operation in a very finicky way is the worst thing in the world.  Let's undo that one caller at a time.

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Make ArrayBufferObject::wasmGrowToSizeInPlace do its work *not* using ArrayBufferObject::stealContents, rather using the exact operations desired, far more simply

This alternative is so much simpler than using ABO::stealContents it's not even funny.

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Implement an ArrayBufferObject::extractStructuredCloneContents for structured-cloning an ArrayBuffer

I'm not sure the "replace contents by nulling them out in a way that doesn't release, then detach using the exact same contents" is aesthetically pleasing or best.  It gets the job done far more clearly than the current thing.  I would be open to suggestions for improving on it once all this is landed, for sure.  (Might be I'll even come up with those improvements, myself.  But I've spent about a week and a half on all this to be able to process about a single five-line review, so I am a little un-energized now.)

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Remove ArrayBufferObject::stealContents now that it's unused

Okay, from here on it's all clear sailing -- ABO::stealContents was the last user of DoesntOwnData!  Just a stream of a bunch of patches incrementally pruning dead code, inlining arguments constant across all callers, etc.

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Remove the useless OwnsState argument from ArrayBufferObject::changeContents

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Remove the useless OwnsState argument from ArrayBufferObject::setNewData

Comment 29

[Jeff Walden [:Waldo]]

Attachment: Remove the useless OwnsState argument from ArrayBufferObject::createForContent

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Inline the contents of a fresh |if (true) { ... }| in ArrayBufferObject::createForContent

Could have done this in the previous patch, figured better to keep changes minimal-er.

Comment 31

[Jeff Walden [:Waldo]]

Attachment: Remove the useless OwnsState argument from ArrayBufferObject::initialize

Comment 32

[Jeff Walden [:Waldo]]

Attachment: Remove the useless OwnsState argument from ArrayBufferObject::setDataPointer

Comment 33

[Jeff Walden [:Waldo]]

Attachment: Now that |ownsData()| is always true, inline that true value into all callers

Comment 34

[Jeff Walden [:Waldo]]

Attachment: Remove setOwnsData and the OWNS_DATA flag

\o/

Comment 35

[Jeff Walden [:Waldo]]

Attachment: Remove ArrayBufferObject::change{,View}Contents as unused

...oh hey, these functions are not actually used any more after a point in this stack, I could have removed them earlier.

Comment 36

[Jeff Walden [:Waldo]]

Attachment: Remove the |newContents| argument from ArrayBufferObject::detach that's now identical for every caller

This complexity was always a very terrible thing.  :-(  Glad to see it die.

Comment 37

[Jeff Walden [:Waldo]]

Attachment: Inline ArrayBufferObject::setNewData into its sole caller

Comment 38

[Jeff Walden [:Waldo]]

Attachment: Full rollup patch, if you want to see the full sum of changes posted in this bug

And that's it! ("that") Hopefully I kept everything small enough this should be mostly simple to run through begin to end.

I can also upload ABO.{cpp,h} and StructuredClone.cpp if the full final states of those files ends up helpful for understanding where things end up, if you want.

Comment 39

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

Comment on attachment 9045266 [details] [diff] [review]
Add an extra bit to ArrayBufferObject::BufferKind's KIND_MASK to permit adding additional buffer kinds beyond the current four

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

::: js/src/vm/ArrayBufferObject.h
@@ +177,5 @@
>    enum BufferKind {
> +    PLAIN = 0b000,  // malloced or inline data
> +    WASM = 0b001,
> +    MAPPED = 0b010,
> +    EXTERNAL = 0b011,

I really enjoy our new style constraints, don't you?

@@ +205,5 @@
>      // Array buffers which do not own their data include buffers that
>      // allocate their data inline, and buffers that are created lazily for
>      // typed objects with inline storage, in which case the buffer points
>      // directly to the typed object's storage.
> +    OWNS_DATA = 0b1'0000,

Whoa. 0b? ' in numeric literals? What is this world coming to?

Comment 40

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

Comment on attachment 9045277 [details] [diff] [review]
Split PLAIN into PLAIN_DATA and USER_OWNED ArrayBuffer data types to clearly segregate the two, rather than categorizing them both as the same thing

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

A truly righteous change, thank you.

::: js/src/shell/js.cpp
@@ +1915,5 @@
>  static bool CacheEntry_setBytecode(JSContext* cx, HandleObject cache,
>                                     uint8_t* buffer, uint32_t length) {
>    MOZ_ASSERT(CacheEntry_isCacheEntry(cache));
>  
> +  using BufferContents = ArrayBufferObject::BufferContents;

`using ArrayBufferObject::BufferContents;` doesn't work?

::: js/src/vm/ArrayBufferObject.cpp
@@ +1801,5 @@
> +  // The caller assumes that a plain malloc'd buffer is returned.  To steal
> +  // actual contents, then, we must have |hasStealableContents()| *and* the
> +  // contents must be |isPlainData()|.  (Mapped data would not be malloc'd;
> +  // user-provided data we flat-out know nothing about at all -- although it
> +  // *should* have not passed the |hasStealableContents()| check anyway.)

Ugh. This stuff all sucks. Hopefully this patch stack will make it tolerable?

::: js/src/vm/ArrayBufferObject.h
@@ +179,5 @@
> +    PLAIN_DATA = 0b000,
> +
> +    /**
> +     * User-owned memory.  The associated buffer must be manually detached
> +     * before the user-owned memory becomes invalid.

On first read, this sounded very wrong -- it sounded to me like you should manually detach in order to correctly invalidate the memory, which doesn't make sense and confused me. I just automatically read it as "this is the correct series of events: (1) detach, (2) the memory becomes invalid."

Uh... "The owner must not allow the memory to become invalid without manually detaching it first."? "The owner must keep the memory valid until the associated buffer is detached."? I don't know.

Comment 41

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] [:s:] from comment #40)

• using BufferContents = ArrayBufferObject::BufferContents;

using ArrayBufferObject::BufferContents; doesn't work?

That's the syntax for accessing inherited members (using declarations), not for declaring type aliases. I'm not sure why C++ doesn't let you use both when there's not ambiguity or something.

Ugh. This stuff all sucks. Hopefully this patch stack will make it tolerable?

Maybe you think it be like it is, and it do.

• /**

• * User-owned memory.  The associated buffer must be manually detached
  

• * before the user-owned memory becomes invalid.
  

On first read, this sounded very wrong -- it sounded to me like you should
manually detach in order to correctly invalidate the memory, which doesn't
make sense and confused me. I just automatically read it as "this is the
correct series of events: (1) detach, (2) the memory becomes invalid."

Uh... "The owner must not allow the memory to become invalid without
manually detaching it first."? "The owner must keep the memory valid until
the associated buffer is detached."? I don't know.

Some patch somewhere, maybe this bug for all I remember, adds docs by JS_NewArrayBufferWithExternalContents (or its renaming, can't remember where that is either) that says what you have to do when. IMO.

Comment 42

[Jeff Walden [:Waldo]]

Try-run is surprisingly clean, only four failures in the whole of it without even categorizing anything or anything.

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

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9063e76442
Add an extra bit to ArrayBufferObject::BufferKind's KIND_MASK to permit adding additional buffer kinds beyond the current four.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/f24c3019443f
Change the comment by ArrayBufferFlags's FOR_ASMJS to indicate that buffers prepared for asm.js must be PLAIN, MAPPED, or EXTERNAL.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e25dc78ef1e
Add a BufferContents::createFailed() to use in situations where a failure occurs, rather than overloading createPlain(nullptr).  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/2849a2838ea5
Split PLAIN into PLAIN_DATA and USER_OWNED ArrayBuffer data types to clearly segregate the two, rather than categorizing them both as the same thing.  r=sfink

Comment 44

[Jeff Walden [:Waldo]]

Attachment: Implement ArrayBufferObject::prepareForAsmJS using a switch, not a series of ifs that's less obviously exhaustive

Obviously it would be better if we did more exhaustive switchin', less ad hoc series of if-tests.  Strangely, this is more or less the only place that can be so improved: without entirely trying I have switches in the obvious places at this point.  But if you happen to see more places that could benefit from this, feel free to note 'em.

Comment 45

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

Comment on attachment 9045279 [details] [diff] [review]
Split PLAIN into INLINE_DATA/MALLOCED for ArrayBuffer kinds

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

\o/

Comment 46

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/6a9063e76442
https://hg.mozilla.org/mozilla-central/rev/f24c3019443f
https://hg.mozilla.org/mozilla-central/rev/6e25dc78ef1e
https://hg.mozilla.org/mozilla-central/rev/2849a2838ea5

Comment 47

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

Comment on attachment 9045281 [details] [diff] [review]
Move most of JS_ExternalizeArrayBufferContents into a static member function on ArrayBufferObject so that internals-observing code isn't smeared across two functions

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

::: js/src/vm/ArrayBufferObject.h
@@ +338,5 @@
>  
>    static size_t objectMoved(JSObject* obj, JSObject* old);
>  
> +  // Convert this ArrayBuffer's storage to memory allocated using the standard
> +  // SpiderMonkey allocator (this may be a no-op), then return  pointer to that

bcdefghijklmnopqrstuvwxyz

(though if this functionality is going to die later in this series, then never mind.)

Comment 48

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

Comment on attachment 9045282 [details] [diff] [review]
Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity

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

...that didn't take long.

Comment 49

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

Comment on attachment 9045284 [details] [diff] [review]
Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1322,5 @@
> +    }
> +    return nullptr;
> +  }
> +
> +  MOZ_ASSERT(!gc::IsInsideNursery(buffer));

How does this work? The object is allocated with GenericObject, not TenuredObject, but then is asserted here. I don't see why GetGCObjectKind(nslots) would enforce a non-nursery object?

At the very least, it seems highly confusing to use GenericObject but assert tenured.

::: js/src/vm/TypedArrayObject.cpp
@@ +78,5 @@
>    }
>  
>    AutoRealm ar(cx, tarray);
>    Rooted<ArrayBufferObject*> buffer(
> +      cx, ArrayBufferObject::createZeroed(cx, tarray->byteLength()));

It would be nice to have those stolen 19 columns returned to us.

Comment 50

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

Comment on attachment 9045285 [details] [diff] [review]
Remove trailing arguments to ArrayBufferObject::createForContents that are identical for every caller

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

Nice how the default was the opposite of what was used.

Comment 51

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

Comment on attachment 9045286 [details] [diff] [review]
Make AllocateArrayBufferContents return uint8_t*, and make its callers consistently not redundantly report OOMs

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +442,5 @@
>    return true;
>  }
>  
> +static uint8_t* AllocateArrayBufferContents(JSContext* cx, uint32_t nbytes) {
> +  auto* p = cx->pod_callocCanGC<uint8_t>(nbytes, js::ArrayBufferContentsArena);

There's really no way to remember whether this reports or not. I wonder if we should have a mandatory ReportOOM/NoReportOOM flag or something.

Comment 52

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

Comment on attachment 9045288 [details] [diff] [review]
Simplify ArrayBufferObject::createForContents some now that it's only ever passed non-null contents

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1236,5 @@
>        }
>  
> +      // ...curious why "mapped" bytes are being fed into a "malloc" counter?
> +      // Me too.  ¯\_(ツ)_/¯
> +      cx->updateMallocCounter(nAllocated);

Bug 1037358, some guy named Waldo reviewed it a mere 5 years ago. Basically, we were running out of address space. So we're kind of cheating and using the malloc counter for both virtual and physical memory usage. Probably worth a comment.

Comment 53

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

Comment on attachment 9045289 [details] [diff] [review]
Consistently use "ArrayBuffer" instead of the vaguer "array buffer" in ArrayBuffer-related JSAPI descriptions

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

Fair complaint.

::: js/src/jsfriendapi.h
@@ +1853,1 @@
>   * typed array view.

The spec doesn't have some weirdo TypedArrayView semantic-only base class?

Comment 54

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

Comment on attachment 9045290 [details] [diff] [review]
Add BufferContents::createFoo functions for every kind so that the templaty, harder-to-search-for create function can be removed

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

I still kinda regret the "create" terminology too. It's not really creating anything of interest, just bundling up a pointer with a descriptor. Oh well.

Comment 55

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

Comment on attachment 9045295 [details] [diff] [review]
Add BufferKind::NO_DATA for ArrayBuffers that have no data (whether because byteLength is zero or because the ArrayBuffer is detached), for which the value of |ownsData()| is irrelevant

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

Whatever. Just make it go away.

::: js/src/vm/ArrayBufferObject.cpp
@@ +1814,5 @@
> +  // and would signal failure if we returned it, plus |hasStealableContents()|
> +  // specifically excludes it; user-provided data we know nothing about at all
> +  // -- although it *should* have not passed the |hasStealableContents()| check
> +  // anyway because it's not owned; mapped data wouldn't be malloc'd; external
> +  // data has to be freed using a provided function.)

A perfect description about how this code jumped the shark at some point.

::: js/src/vm/ArrayBufferObject.h
@@ +272,5 @@
>        return BufferContents(static_cast<uint8_t*>(data), MALLOCED);
>      }
>  
> +    static BufferContents createNoData() {
> +      return BufferContents(nullptr, NO_DATA);

♫ Yes, we have no bananas.
We have no bananas today! ♬

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a391fd808f
Rename JS_NewArrayBufferWithExternalContents to JS_NewArrayBufferWithUserOwnedContents to better accord with the USER_OWNED ArrayBufferKind name, and to avoid namespace overload with the semantically distinct JS_NewExternalArrayBuffer and JS_ExternalizeArrayBufferContents functions.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9294b0d54597
Split PLAIN into INLINE_DATA/MALLOCED for ArrayBuffer kinds.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/3512de18097c
Move most of JS_ExternalizeArrayBufferContents into a static member function on ArrayBufferObject so that internals-observing code isn't smeared across two functions.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/432b2e88c651
Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/5be8cb19ad3d
Remove ArrayBufferObject::hasData and perform its operation in its sole caller in a more straightforward manner.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1477b8eb5442
Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9f62f10eeb5
Remove trailing arguments to ArrayBufferObject::createForContents that are identical for every caller.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e668a95d32
Make AllocateArrayBufferContents return uint8_t*, and make its callers consistently not redundantly report OOMs.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7fad41d68e0
Only pass BufferContents containing a non-null pointer to |ArrayBufferObject::createForContents|.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/deaa41ca96da
Simplify ArrayBufferObject::createForContents some now that it's only ever passed non-null contents.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80ce47bba52
Consistently use "ArrayBuffer" instead of the vaguer "array buffer" in ArrayBuffer-related JSAPI descriptions.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc3cd982953
Add BufferContents::createFoo functions for every kind so that the templaty, harder-to-search-for create function can be removed.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f66e6c059df
Remove ArrayBufferOffset::offsetOfFlags as unused.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/086849ef0dd5
Add BufferKind::NO_DATA for ArrayBuffers that have no data (whether because byteLength is zero or because the ArrayBuffer is detached), for which the value of |ownsData()| is irrelevant.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/d80b681a68e6
Initialize all ArrayBuffers that store their data inline using a single function.  r=sfink

Comment 57

[Natalia Csoregi [:nataliaCs]]

Backed out for spidermonkey bustages on /testAtomicOperations.cpp

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/eed1098d0d6c9e3af5b02154295e452c6c21bb48

push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d80b681a68e6a273d1663d5048c3dfcd7b4debd1&searchStr=spidermonkey&group_state=expanded&selectedJob=229805289

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229805289&repo=mozilla-inbound&lineNumber=2861

[task 2019-02-22T03:05:14.469Z] In file included from /builds/worker/workspace/build/src/obj-spider/js/src/jsapi-tests/Unified_cpp_js_src_jsapi-tests1.cpp:2:0:
[task 2019-02-22T03:05:14.469Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp: In member function 'virtual bool cls_testAtomicOperationsU8Clamped::run(JS::HandleObject)':
[task 2019-02-22T03:05:14.469Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:294:9: error: 'uint8_clamped' does not name a type
[task 2019-02-22T03:05:14.469Z] const uint8_clamped A(0xab);
[task 2019-02-22T03:05:14.469Z] ^~~~~~~~~~~~~
[task 2019-02-22T03:05:14.469Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:295:9: error: 'uint8_clamped' does not name a type
[task 2019-02-22T03:05:14.470Z] const uint8_clamped B(0x37);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:24: error: 'uint8_clamped' was not declared in this scope
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:283:3: note: in definition of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] T* q = (T*)hidePointerValue((void*)atomicMem);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:283:6: error: 'q' was not declared in this scope
[task 2019-02-22T03:05:14.470Z] T* q = (T*)hidePointerValue((void*)atomicMem);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:3: note: in expansion of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~~~~~~~~
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:283:13: error: expected primary-expression before ')' token
[task 2019-02-22T03:05:14.470Z] T* q = (T*)hidePointerValue((void*)atomicMem);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:3: note: in expansion of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~~~~~~~~
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:39: error: 'A' was not declared in this scope
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:284:8: note: in definition of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] q = A;
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:285:15: error: template argument 1 is invalid
[task 2019-02-22T03:05:14.470Z] SharedMem<T> p =
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:3: note: in expansion of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~~~~~~~~

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd2d5e7ec57
Rename JS_NewArrayBufferWithExternalContents to JS_NewArrayBufferWithUserOwnedContents to better accord with the USER_OWNED ArrayBufferKind name, and to avoid namespace overload with the semantically distinct JS_NewExternalArrayBuffer and JS_ExternalizeArrayBufferContents functions.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0aee08543ad
Split PLAIN into INLINE_DATA/MALLOCED for ArrayBuffer kinds.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5e10661a78
Move most of JS_ExternalizeArrayBufferContents into a static member function on ArrayBufferObject so that internals-observing code isn't smeared across two functions.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/76600605e1e3
Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a304d4b53c0
Remove ArrayBufferObject::hasData and perform its operation in its sole caller in a more straightforward manner.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/765bc7a86f72
Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/92fc3d9b6699
Remove trailing arguments to ArrayBufferObject::createForContents that are identical for every caller.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d373ab477b
Make AllocateArrayBufferContents return uint8_t*, and make its callers consistently not redundantly report OOMs.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/4771a589408d
Only pass BufferContents containing a non-null pointer to |ArrayBufferObject::createForContents|.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c09226e55b
Simplify ArrayBufferObject::createForContents some now that it's only ever passed non-null contents.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/76e407233988
Consistently use "ArrayBuffer" instead of the vaguer "array buffer" in ArrayBuffer-related JSAPI descriptions.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdddf46e820
Add BufferContents::createFoo functions for every kind so that the templaty, harder-to-search-for create function can be removed.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cead3a873e2
Remove ArrayBufferOffset::offsetOfFlags as unused.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/3556d94cbc81
Add BufferKind::NO_DATA for ArrayBuffers that have no data (whether because byteLength is zero or because the ArrayBuffer is detached), for which the value of |ownsData()| is irrelevant.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b7a07b449ba
Initialize all ArrayBuffers that store their data inline using a single function.  r=sfink

Comment 59

[Jeff Walden [:Waldo]]

Comment on attachment 9045284 [details] [diff] [review]
Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1322,5 @@
> +    }
> +    return nullptr;
> +  }
> +
> +  MOZ_ASSERT(!gc::IsInsideNursery(buffer));

We ironed this out over IRC -- ArrayBuffers are never in the nursery because they have a finalizer, and not a nursery-finalizer-skippable one.  I added a reason-string to this fresh call and to the one in ABO::createForContents, above, from which this was copied.

Comment 60

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/efd2d5e7ec57
https://hg.mozilla.org/mozilla-central/rev/c0aee08543ad
https://hg.mozilla.org/mozilla-central/rev/9e5e10661a78
https://hg.mozilla.org/mozilla-central/rev/76600605e1e3
https://hg.mozilla.org/mozilla-central/rev/5a304d4b53c0
https://hg.mozilla.org/mozilla-central/rev/765bc7a86f72
https://hg.mozilla.org/mozilla-central/rev/92fc3d9b6699
https://hg.mozilla.org/mozilla-central/rev/51d373ab477b
https://hg.mozilla.org/mozilla-central/rev/4771a589408d
https://hg.mozilla.org/mozilla-central/rev/b2c09226e55b
https://hg.mozilla.org/mozilla-central/rev/76e407233988
https://hg.mozilla.org/mozilla-central/rev/dcdddf46e820
https://hg.mozilla.org/mozilla-central/rev/1cead3a873e2
https://hg.mozilla.org/mozilla-central/rev/3556d94cbc81
https://hg.mozilla.org/mozilla-central/rev/5b7a07b449ba

Comment 61

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

Comment on attachment 9045298 [details] [diff] [review]
Remove |ArrayBufferObject::hasStealableContents()| and replace it with its contents, appropriately simplified for each calling location

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

This change seems safe, and it is great to see that function go.

::: js/src/vm/ArrayBufferObject.h
@@ -351,5 @@
>    static BufferContents stealContents(JSContext* cx,
>                                        Handle<ArrayBufferObject*> buffer,
>                                        bool hasStealableContents);
>  
> -  bool hasStealableContents() const {

This function is the main reason I've been averse to touching anything ArrayBuffer related. I don't like having to re-think through the logic every time (especially when I'm still fuzzy on the wasm and asm.js constraints.)

Comment 62

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

Comment on attachment 9045302 [details] [diff] [review]
Remove most uses of DoesntOwnData for NO_DATA and USER_OWNED and make current users that depend on |ownsData()| not do so

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1711,5 @@
> +  // pointer, so all that changes is the pointer's plain identity, and it seems
> +  // okay to say "don't do that".  Ownership state is unchanged.  And the
> +  // kind-change shouldn't matter because we only test
> +  // |buffer->hasUserOwnedData()| in ABO::prepareForAsmJS -- but only *after*
> +  // anything length-zero would have been filtered out, so no change there.

Phew. I retraced through this logic, and it seems correct to me.

Comment 63

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

Comment on attachment 9045313 [details] [diff] [review]
Implement an ArrayBufferObject::extractStructuredCloneContents for structured-cloning an ArrayBuffer

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1492,5 @@
> +      MOZ_ASSERT_UNREACHABLE("bad kind when stealing malloc'd data");
> +      break;
> +  }
> +
> +  return BufferContents::createFailed();

Shouldn't there be another MOZ_ASSERT_UNREACHABLE for a bogus kind here?

::: js/src/vm/StructuredClone.cpp
@@ +1875,2 @@
>          if (!bufContents) {
>            return false;  // out of memory

Hm... there seems to be an OOM reporting problem here, in some earlier patch that I missed.

This condition is now OOM or WASM, though the wasm case will never happen because https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838

I see https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838 that reports OOM, but then all the other paths do not. Given https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1903 it really seems like everything should report OOM.

Perhaps this should be

    if (!bufContents) {
      MOZ_ASSERT(!arrayBuffer->isWasm());
      ReportOutOfMemory(cx);
      return false;
    }

at least for now?

I'm still marking this r+ because the basic change is (very) good.

Comment 64

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

Comment on attachment 9045320 [details] [diff] [review]
Remove the useless OwnsState argument from ArrayBufferObject::createForContent

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +1258,5 @@
>    // the ArrayBuffer for use as raw storage to store such information.
>    size_t reservedSlots = JSCLASS_RESERVED_SLOTS(&class_);
>  
>    size_t nslots = reservedSlots;
> +  if (true) {

Heh, thanks.

Comment 65

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

Comment on attachment 9045328 [details] [diff] [review]
Remove ArrayBufferObject::change{,View}Contents as unused

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

Whoa, I somehow didn't expect that. Most excellent.

Comment 66

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

Comment on attachment 9045329 [details] [diff] [review]
Remove the |newContents| argument from ArrayBufferObject::detach that's now identical for every caller

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

\o/ the last vestiges gone

Comment 67

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

Comment on attachment 9045330 [details] [diff] [review]
Inline ArrayBufferObject::setNewData into its sole caller

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

That was an awesome refactoring series. Thank you for breaking it down to that level, it really helped.

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3ad3942d67
Remove |ArrayBufferObject::hasStealableContents()| and replace it with its contents, appropriately simplified for each calling location.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a4ce77f2f1
Remove most uses of DoesntOwnData for NO_DATA and USER_OWNED and make current users that depend on |ownsData()| not do so.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/f487c864d2ab
Make NoteViewBufferWasDetached a lambda instead of a global function.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/715e9b139ebb
Implement an ArrayBufferObject::stealMallocedContents for use in JS_StealArrayBufferContents, rather than reusing the hoary ArrayBufferObject::stealContents with finicky caller-side should-this-steal logic.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9638eeea757
Make ArrayBufferObject::wasmGrowToSizeInPlace do its work *not* using ArrayBufferObject::stealContents, rather using the exact operations desired, far more simply.  r=sfink

Comment 69

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] [:s:] from comment #63)

::: js/src/vm/StructuredClone.cpp
@@ +1875,2 @@

     if (!bufContents) {
       return false;  // out of memory

Hm... there seems to be an OOM reporting problem here, in some earlier patch
that I missed.

This condition is now OOM or WASM, though the wasm case will never happen
because
https://searchfox.org/mozilla-central/rev/
dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838

I see
https://searchfox.org/mozilla-central/rev/
dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838
that reports OOM, but then all the other paths do not. Given
https://searchfox.org/mozilla-central/rev/
dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1903
it really seems like everything should report OOM.

Perhaps this should be

if (!bufContents) {
  MOZ_ASSERT(!arrayBuffer->isWasm());
  ReportOutOfMemory(cx);
  return false;
}

at least for now?

As of this patch and the prior ones, you have

/* static */ ArrayBufferObject::BufferContents
ArrayBufferObject::extractStructuredCloneContents(
    JSContext* cx, Handle<ArrayBufferObject*> buffer) {
  CheckStealPreconditions(buffer, cx);

  BufferContents contents = buffer->contents();

  switch (contents.kind()) {
    case INLINE_DATA:
    case NO_DATA:
    case USER_OWNED: {
      uint8_t* copiedData = NewCopiedBufferContents(cx, buffer);
      if (!copiedData) {
        return BufferContents::createFailed();
      }

      ArrayBufferObject::detach(cx, buffer, BufferContents::createNoData());
      return BufferContents::createMalloced(copiedData);
    }

    case MALLOCED:
    case MAPPED: {
      MOZ_ASSERT(contents);

      // Overwrite the old data pointer *without* releasing old data.
      BufferContents newContents = BufferContents::createNoData();
      buffer->setDataPointer(newContents, OwnsData);

      // Detach |buffer| now that doing so won't release |oldContents|.
      ArrayBufferObject::detach(cx, buffer, newContents);
      return contents;
    }

    case WASM:
      JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                                JSMSG_WASM_NO_TRANSFER);
      return BufferContents::createFailed();

    case EXTERNAL:
      MOZ_ASSERT_UNREACHABLE(
          "external ArrayBuffer shouldn't have passed the "
          "structured-clone preflighting");
      break;

    case BAD1:
      MOZ_ASSERT_UNREACHABLE("bad kind when stealing malloc'd data");
      break;
  }

  MOZ_ASSERT_UNREACHABLE("garbage kind computed");
  return BufferContents::createFailed();
}

The external/bad1/fallthrough cases can be silent halt to script execution, that's fine and perhaps even good. WASM will obviously set a pending exception. MALLOCED/MAPPED always return non-null contents. INLINE/NO/USER if NewCopiedBufferContents fails, it reported OOM -- I changed this earlier in this bug -- so we're good there. And then the copiedData returned is non-null.

So I don't think there's an OOM reporting problem here. I think you're comparing against current code without remembering everything all these other patches here have done?

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63536a044a29
Implement an ArrayBufferObject::extractStructuredCloneContents for structured-cloning an ArrayBuffer.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb592de5e03
Remove ArrayBufferObject::stealContents now that it's unused.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a39ff06158
Remove the useless OwnsState argument from ArrayBufferObject::changeContents.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/713088adfe5f
Remove the useless OwnsState argument from ArrayBufferObject::setNewData.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/870a55710969
Remove the useless OwnsState argument from ArrayBufferObject::createForContent.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4101f442782
Inline the contents of a fresh |if (true) { ... }| in ArrayBufferObject::createForContent.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/a06864bc8352
Remove the useless OwnsState argument from ArrayBufferObject::initialize.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4c27821811
Remove the useless OwnsState argument from ArrayBufferObject::setDataPointer.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/650bd5a18809
Now that |ownsData()| is always true, inline that true value into all callers.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/694fe0c43793
Remove setOwnsData and the OWNS_DATA flag.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/973c3800d5d6
Remove ArrayBufferObject::change{,View}Contents as unused.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab967077f8a3
Remove the |newContents| argument from ArrayBufferObject::detach that's now identical for every caller.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9103748036d1
Inline ArrayBufferObject::setNewData into its sole caller.  r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb879a6a95f8
Implement ArrayBufferObject::prepareForAsmJS using a switch, not a series of ifs that's less obviously exhaustive.  r=sfink

Comment 71

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] [:s:] from comment #67)

That was an awesome refactoring series. Thank you for breaking it down to
that level, it really helped.

Didn't much have a choice, as I wouldn't have been smart enough to do it any other way. :-) Gordian knot, you have to pull a little bit at a time to get the whole thing untangled given the unholy kind/ownership pairing.

Comment 72

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ac3ad3942d67
https://hg.mozilla.org/mozilla-central/rev/d8a4ce77f2f1
https://hg.mozilla.org/mozilla-central/rev/f487c864d2ab
https://hg.mozilla.org/mozilla-central/rev/715e9b139ebb
https://hg.mozilla.org/mozilla-central/rev/a9638eeea757
https://hg.mozilla.org/mozilla-central/rev/63536a044a29
https://hg.mozilla.org/mozilla-central/rev/5cb592de5e03
https://hg.mozilla.org/mozilla-central/rev/a7a39ff06158
https://hg.mozilla.org/mozilla-central/rev/713088adfe5f
https://hg.mozilla.org/mozilla-central/rev/870a55710969
https://hg.mozilla.org/mozilla-central/rev/f4101f442782
https://hg.mozilla.org/mozilla-central/rev/a06864bc8352
https://hg.mozilla.org/mozilla-central/rev/ef4c27821811
https://hg.mozilla.org/mozilla-central/rev/650bd5a18809
https://hg.mozilla.org/mozilla-central/rev/694fe0c43793
https://hg.mozilla.org/mozilla-central/rev/973c3800d5d6
https://hg.mozilla.org/mozilla-central/rev/ab967077f8a3
https://hg.mozilla.org/mozilla-central/rev/9103748036d1
https://hg.mozilla.org/mozilla-central/rev/bb879a6a95f8