776159 - Thread more handles through jsarray

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

Steve, do these changes make sense, or am I being too aggressive?

Comment 1

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

Comment on attachment 644519 [details] [diff] [review]
v0

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

::: js/src/jsarray.cpp
@@ +3742,5 @@
>          }
>      }
>  
>      Rooted<GlobalObject*> parent(cx, parent_);
> +    RootedObject proto(cx, protoArg);

Ooh, that's a good example of a tricky case. You're double-rooting proto, but I think it's probably best this way. The inner proto that you pass to FindProto definitely has to be rooted, but the parameter doesn't need to be -- looking at the callers, you're now explicitly rooting and so you could avoid one root by passing in a bare JSObject* and rooting it at the top. But I like it the way you did it better, just for the simplicity; you don't have to think it through this way.

It's a good example for a rooting document.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 644519 [details] [diff] [review]
v0

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

::: js/src/jsarray.cpp
@@ +3795,5 @@
>  #ifdef JS_METHODJIT
>  JSObject * JS_FASTCALL
>  mjit::stubs::NewDenseUnallocatedArray(VMFrame &f, uint32_t length)
>  {
> +    JSObject *obj = NewArray<false>(f.cx, length, HandleObject::fromMarkedLocation((JSObject **)&f.scratch));

Is this safe? f.scratch is not a marked location, as far as I can tell.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v1

Steve, I've updated this to use RawObject, and I think it's better overall now. I do want a second opinion, however, so I'm re-requesting.

(In reply to Bill McCloskey (:billm) from comment #2)
> HandleObject::fromMarkedLocation((JSObject **)&f.scratch));
> 
> Is this safe? f.scratch is not a marked location, as far as I can tell.

I assumed wrong.  Do we need these to be marked through for moving GC?

Comment 4

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

Comment on attachment 645010 [details] [diff] [review]
v1

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

Sorry about missing the f.scratch thing earlier; I made the wrong assumption too. I see now that it is a void* slot. Unfortunately, it seems to be used to store things like argc at times, so we can't just change it to a gc::Cell* and trace it.

::: js/src/jsarray.cpp
@@ +3683,5 @@
>      }
>  
>      Rooted<GlobalObject*> parent(cx, parent_);
> +    RootedObject proto(cx, protoArg);
> +    PoisonPtr(reinterpret_cast<uintptr_t *>(protoArg));

Ooh, we should fix that. The reinterpret_cast kind of defeats the purpose, since this is mainly for documentation. Would it work to just declare PoisonPtr as taking a void*, or do we need to templafuscate it?

Comment 5

[Terrence Cole [:terrence]]

Making PoisonPtr take a void* worked like a charm:

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

Comment 6

[Ed Morley [:emorley]]

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