969812 - Make JS_NewArrayObject take HandleValueArray argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Currently JS_NewArrayObject takes a length argument and a Value pointer which it roots.  We should convert this to take a HandleValueArray and remove the rooting.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: 1 - convert-new-array-object

Initial patch to do this in the JS engine.  Splits JS_NewArrayObject in to a version which takes a length and doesn't initialize, and a version which takes the contents of the new array.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 2 - convert-new-array-object-browser

Convert occurrences in the browser.

Comment 3

[Jon Coppeard (:jonco)]

So there a few places where we have an Sequence<JS::Value> which we use as the contents for creating an array, in: 

- MessagePort::PostMessageMoz()
- nsGlobalWindow::PostMessageMoz()
- WorkerPrivateParent<Derived>::PostMessageInternal()
- WorkerPrivate::PostMessageToParentInternal()

I don't understand the control flow here, but it seems these are ultimately coming from generated bindings code which ensures they are rooted (hence I used fromMarkedLocation to convert them to a HandleValueArray).

Do you think this is ok, or is there some other source for these where they can be unrooted?

I was wondering about the possibility of changing these APIs to express the rooted-ness of the arguments but it seems like that would require special casing the bindings generation to sometimes generate flat arrays of Values in a different way.

Comment 4

[Tom S [:evilpie]]

Comment on attachment 8372858 [details] [diff] [review]
1 - convert-new-array-object

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

::: js/src/jsapi.cpp
@@ +3790,5 @@
> +    JS_ASSERT(!cx->runtime()->isAtomsCompartment(cx->compartment()));
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);
> +
> +    return NewDenseCopiedArray(cx, length, nullptr);

I don't really understand all the different NewArray functions anymore, but NewDenseAllocatedArray seems like a better fit here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Do you think this is ok, or is there some other source for these where they can be
> unrooted?

No, they should always be coming from bindings code, and that should always root them.

> it seems like that would require special casing the bindings generation to sometimes
> generate flat arrays of Values in a different way

We can do that, if that would make things clearer.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky [:bz] from comment #5)

Thanks.  I will leave it using fromMarkedLocation() for now then, but if these methods could take HandleValueArray then that would be clearer, and safer in case any anything else tries to call them.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: 1 - convert-new-array-object

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 2 - convert-new-array-object-browser

Comment 9

[Boris Zbarsky [:bzbarsky]]

> but if these methods could take HandleValueArray

Please file a followup?  I'll look into how sane that is to do.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8374040 [details] [diff] [review]
1 - convert-new-array-object

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

Very nice! r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8374041 [details] [diff] [review]
2 - convert-new-array-object-browser

>+++ b/dom/base/MessagePort.cpp
> MessagePort::PostMessageMoz(JSContext* aCx, JS::Handle<JS::Value> aMessage,

Please don't add the stray blank line at the start of this function.

r=me with that

Comment 12

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33f464080abc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33f464080abc