Closed Bug 1079844 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: till, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

Attachments

(16 files)

3.93 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.72 KB, patch
mrrrgn
: review+
Details | Diff | Splinter Review
2.89 KB, patch
sfink
: review+
Details | Diff | Splinter Review
9.25 KB, patch
jandem
: review+
Details | Diff | Splinter Review
11.04 KB, patch
jandem
: review+
Details | Diff | Splinter Review
22.51 KB, patch
sfink
: review+
Details | Diff | Splinter Review
6.43 KB, patch
sfink
: review+
Details | Diff | Splinter Review
7.30 KB, patch
sfink
: review+
Details | Diff | Splinter Review
3.51 KB, patch
sfink
: review+
Details | Diff | Splinter Review
6.57 KB, patch
till
: review+
Details | Diff | Splinter Review
9.30 KB, patch
till
: review+
Details | Diff | Splinter Review
16.83 KB, patch
jandem
: review+
Details | Diff | Splinter Review
50.82 KB, patch
till
: review+
Details | Diff | Splinter Review
7.97 KB, patch
till
: review+
Details | Diff | Splinter Review
22.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #8711934 - Flags: review?(sphink)
Detachment applies only to ArrayBuffers, and the methods don't do any detaching/neutering, so "notify" makes more sense as the verb here.
Attachment #8711935 - Flags: review?(sphink)
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.
Attachment #8711943 - Flags: review?(bzbarsky)
Attachment #8711929 - Flags: review?(winter2718) → review+
Keywords: leave-open
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 :)
Attachment #8711928 - Flags: review?(jdemooij) → review+
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/
Attachment #8711931 - Flags: review?(jdemooij) → review+
Attachment #8711932 - Flags: review?(jdemooij) → review+
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
Attachment #8711939 - Flags: review?(jdemooij) → review+
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`?
Attachment #8711937 - Flags: review?(till) → review+
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?
Attachment #8711938 - Flags: review?(till) → review+
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!
Attachment #8711941 - Flags: review?(till) → review+
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.
Attachment #8711940 - Flags: review?(till) → review+
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?
Attachment #8711930 - Flags: review?(sphink) → review+
Attachment #8711933 - Flags: review?(sphink) → review+
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.
Attachment #8711934 - Flags: review?(sphink) → review+
Attachment #8711935 - Flags: review?(sphink) → review+
Attachment #8711936 - Flags: review?(sphink) → review+
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 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
Attachment #8711943 - Flags: review?(bzbarsky) → review+
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.)
Attachment #8713404 - Flags: review?(bzbarsky)
Comment on attachment 8713404 [details] [diff] [review]
Last micro-change to detachment terminology

r=me
Attachment #8713404 - Flags: review?(bzbarsky) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/bf8737df4163
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.