1079844 - Refer to "detaching" instead of "neutering" of ArrayBuffers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

In recent ES6 draft, all references to "neuter" have been replaced with "detach".

Not really blocking anything, as this isn't exposed to content script right now. Since it's expected that an `isDetached` method on ArrayBuffer will be introduced and indexing into detached buffers is supposed to throw (with the associated error messages), it makes sense to be consistent about this, however.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Refer to "detaching" instead of "neutering" of ArrayBuffers, in JIT optimization tracking code

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Adjust TypedArray.js comments to refer to detachment rather than neutering

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Rename JS_ARRAYBUFFER_NEUTERED_FLAG to use detachment terminology

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Rename TI's ObjectKey flag to use detachment terminology

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Rename CheckForNeuteredTypedObject and the flag underlying it to use detachment terminology

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Rename isNeutered() to isDetached(), and rename isNeutered() on views to hasDetachedBuffer()

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Rename ABO::neuter to ABO::detach

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Rename ArrayBufferViewObject::neuter to ABVO::notifyBufferDetached

Detachment applies only to ArrayBuffers, and the methods don't do any detaching/neutering, so "notify" makes more sense as the verb here.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Rename ABO::neuterView to a standalone NoteViewBufferWasDetached function

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Convert the JS_NeuterArrayBufferObject API to detachment terminology

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Rename JS_IsNeuteredArrayBufferObject to JS_IsDetachedArrayBufferObject

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Rename the final scattered bits of 'neuter' terminology to detachment terminology

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Rename the shell builtin 'neuter' function to 'detachArrayBuffer', consistent with the spec name for the operation

Someone had to review it, sorry.  :-(

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Rename various tests to use 'detach' instead of 'neuter' in their file names

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Change various non-js/ files/tests/etc. to refer to detaching rather than neutering, wherever it's clear ArrayBuffer neutering was the 'neuter' referent

I'm in no rush whatsoever on this bit, so take as long as you need/want/find convenient.

I could split it up for more-precise reviewing if necessary, but I'm kind of leery of making too many people context-switch to do this.  Plus you're one of the more likely people to be able to hold a line on not using "neuter" in tests/names/code/etc. in the future, so it's worth keeping you aware of the change.

There are one or two additional places, after all these patches, that use "neuter" where I'm not certain whether ArrayBuffer detachment is the referent.  Given the mess of all this, I think I want to get everything *known*, fixed, then go back to clean up any slackers that might still need it.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d205c7574e0

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8711928 [details] [diff] [review]
Refer to "detaching" instead of "neutering" of ArrayBuffers, in JIT optimization tracking code

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

::: js/src/jit/IonBuilder.cpp
@@ +8679,5 @@
>      if (objPrediction.hasKnownArrayLength(&lenOfAll)) {
>          length = constantInt(lenOfAll);
>  
>          // If we are not loading the length from the object itself, only
>          // optimize if the array buffer can't have been neutered.

I'll assume later patches will address this comment + OBJECT_FLAG_TYPED_OBJECT_NEUTERED :)

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8711931 [details] [diff] [review]
Rename TI's ObjectKey flag to use detachment terminology

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

(In reply to Jan de Mooij [:jandem] from comment #17)
> I'll assume later patches will address this comment +
> OBJECT_FLAG_TYPED_OBJECT_NEUTERED :)

\o/

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 8711939 [details] [diff] [review]
Rename the final scattered bits of 'neuter' terminology to detachment terminology

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

::: js/src/proxy/CrossCompartmentWrapper.cpp
@@ -519,5 @@
>      MOZ_ASSERT(&p->value().unsafeGet()->toObject() == wobj);
>      wcompartment->removeWrapper(p);
>  
>      // When we remove origv from the wrapper map, its wrapper, wobj, must
> -    // immediately cease to be a cross-compartment wrapper. Neuter it.

Lol

Comment 20

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea7b807593c50b126367ae35ac8d121942c0987
Backed out changeset 1d205c7574e0 (bug 1079844) for build bustage

Comment 21

[Till Schneidereit [:till]]

Comment on attachment 8711937 [details] [diff] [review]
Convert the JS_NeuterArrayBufferObject API to detachment terminology

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

r=me

::: js/src/jsfriendapi.h
@@ +2027,5 @@
>   */
>  extern JS_FRIEND_API(JSObject*)
>  JS_GetArrayBufferViewBuffer(JSContext* cx, JS::HandleObject obj, bool* isSharedMemory);
>  
> +enum DetachDataDisposition {

Or even `enum class`?

Comment 22

[Till Schneidereit [:till]]

Comment on attachment 8711938 [details] [diff] [review]
Rename JS_IsNeuteredArrayBufferObject to JS_IsDetachedArrayBufferObject

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

r=me under the assumption that the removed checks were redundant. Please point out where this is still tested in a comment here, though.

::: js/src/jsapi-tests/testArrayBuffer.cpp
@@ +57,5 @@
>          // Steal the contents
>          void* contents = JS_StealArrayBufferContents(cx, obj);
>          CHECK(contents != nullptr);
>  
> +        CHECK(JS_IsDetachedArrayBufferObject(obj));

Why remove all the other checks? Is this tested somewhere else, too?

Comment 23

[Till Schneidereit [:till]]

Comment on attachment 8711941 [details] [diff] [review]
Rename various tests to use 'detach' instead of 'neuter' in their file names

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

And Splinter was even able to properly display this patch!

Comment 24

[Till Schneidereit [:till]]

Comment on attachment 8711940 [details] [diff] [review]
Rename the shell builtin 'neuter' function to 'detachArrayBuffer', consistent with the spec name for the operation

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

r=me. Reviewing this in Splinter would've been painful, but with the unified diff it's not so bad.

Comment 25

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

Comment on attachment 8711930 [details] [diff] [review]
Rename JS_ARRAYBUFFER_NEUTERED_FLAG to use detachment terminology

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

::: js/src/vm/ArrayBufferObject.h
@@ +163,2 @@
>                    "self-hosted code with burned-in constants must use the "
>                    "correct NEUTERED bit value");

Hopefully NEUTERED will be changing in a later patch?

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fecabf69ee9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be65753d249
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2aa640c696a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bab1e2c8491
https://hg.mozilla.org/integration/mozilla-inbound/rev/293ec7dbc37f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6b32b76f5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/332b15daa544
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c756122e168
https://hg.mozilla.org/integration/mozilla-inbound/rev/82fa5db23a4c

Comment 27

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

Comment on attachment 8711934 [details] [diff] [review]
Rename ABO::neuter to ABO::detach

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +278,5 @@
>          cx->compartment()->detachedTypedObjects = 1;
>      }
>  
> +    // Update all views of the buffer to account for the buffer having been
> +    // detached, and clear the buffer's list of views and its data.

"clear the buffer's data and list of views"? (I read this as "list of views and each view's data".)

::: js/src/vm/ArrayBufferObject.h
@@ +273,5 @@
>      void setNewOwnedData(FreeOp* fop, BufferContents newContents);
>      void changeContents(JSContext* cx, BufferContents newContents);
>  
> +    // Detach this buffer from its original memory.  (This necessarily makes
> +    // views of this buffer unusable to modify that original memory.)

s/to modify/for modifying/? The wording doesn't flow well.

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 8711938 [details] [diff] [review]
Rename JS_IsNeuteredArrayBufferObject to JS_IsDetachedArrayBufferObject

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

::: js/src/jsapi-tests/testArrayBuffer.cpp
@@ +57,5 @@
>          // Steal the contents
>          void* contents = JS_StealArrayBufferContents(cx, obj);
>          CHECK(contents != nullptr);
>  
> +        CHECK(JS_IsDetachedArrayBufferObject(obj));

The other checks are most assuredly tested somewhere else, like the jstests/jit-tests whose mini-modifications in another attachment here you reviewed.  But for this test's purposes, it truly doesn't care about those accessors or their behavior at all -- it wants to ask exactly the question made here.  Because C++ has the tools to answer that question precisely (where ES6 hasn't provided them to us yet, really), it seems like that's the question to ask here, with no digression for the behavior of irrelevant accessors, even in a comment.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faa7773ee97f
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e7bff461f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9429a672f3e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d08bf5b0f989
https://hg.mozilla.org/integration/mozilla-inbound/rev/79782eaeb501

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8711943 [details] [diff] [review]
Change various non-js/ files/tests/etc. to refer to detaching rather than neutering, wherever it's clear ArrayBuffer neutering was the 'neuter' referent

>+++ b/dom/canvas/test/test_imagebitmap_transfer.html
>+       "After transfer, ImageBitmap becomes detached");

ImageBitmap is not a typed array or array buffer... The terminology currently used for it in the spec is in fact "Neuter".  See https://html.spec.whatwg.org/multipage/webappapis.html#images:transfer-a-transferable-object step 3 which links to https://html.spec.whatwg.org/multipage/infrastructure.html#concept-transferable-neutered (but of course it's talking about neutering ArrayBuffer too; needs a spec bug for that).  

So probably ok to change this, as long as you file an HTML spec bug to update its terminology.

>+++ b/dom/canvas/test/test_offscreencanvas_neuter.html

Rename the file too?

r=me

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fecabf69ee9
https://hg.mozilla.org/mozilla-central/rev/9be65753d249
https://hg.mozilla.org/mozilla-central/rev/c2aa640c696a
https://hg.mozilla.org/mozilla-central/rev/5bab1e2c8491
https://hg.mozilla.org/mozilla-central/rev/293ec7dbc37f
https://hg.mozilla.org/mozilla-central/rev/ec6b32b76f5f
https://hg.mozilla.org/mozilla-central/rev/332b15daa544
https://hg.mozilla.org/mozilla-central/rev/5c756122e168
https://hg.mozilla.org/mozilla-central/rev/82fa5db23a4c
https://hg.mozilla.org/mozilla-central/rev/faa7773ee97f
https://hg.mozilla.org/mozilla-central/rev/86e7bff461f8
https://hg.mozilla.org/mozilla-central/rev/9429a672f3e5
https://hg.mozilla.org/mozilla-central/rev/d08bf5b0f989
https://hg.mozilla.org/mozilla-central/rev/79782eaeb501

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9fb3bb07c79

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9fb3bb07c79

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9fb3bb07c79

Comment 35

[Jeff Walden [:Waldo]]

Attachment: Last micro-change to detachment terminology

After this patch, the only remaining neuter references in the tree, that should instead be to detachment, are in web-platform-tests that won't be changed until https://www.w3.org/Bugs/Public/show_bug.cgi?id=27031 is dealt with and upstream is fixed.  Good enough for this bug's purposes.

(Oh, and unrelatedly, I fixed pdf.js's one reference to ArrayBuffer neutering upstream to be detachment, as the other imported thing needing changing.  Don't know, don't care when the next import will be, it'll be soon enough.)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8713404 [details] [diff] [review]
Last micro-change to detachment terminology

r=me

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8737df4163

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf8737df4163