797128 - Justify SkipRoots in jstypedarray.cpp's copyFromArray()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The first valid SkipRoot I've found. copyFromArray was inexplicably discarding a handle, and with that fixed there are some unrooted internal pointers stored on the stack. But the only things that can trigger GC will also trigger an early exit, so all of the unrooted pointers are dead if a GC happens anyway.

Comment 1

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

Attachment: Minor rooting fix and SkipRoot justification.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 667159 [details] [diff] [review]
Minor rooting fix and SkipRoot justification.

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

Hurm.  This is annoying: the only reason we need the SkipRoots is the unconditional MaybeCheckStackRoots in ToNumber.

::: js/src/jstypedarray.cpp
@@ +2075,2 @@
>          NativeType *dest = static_cast<NativeType*>(viewData(thisTypedArrayObj)) + offset;
> +        SkipRoot skipDest(cx, &dest);

Lets hoist |src| so that we can have a common comment.  Something like:

<code>
const Value *src = NULL;
NativeType *dest = static_cast<NativeType*>(viewData(thisTypedArrayObj)) + offset;

// The only way the code below can GC is if nativeFromValue fails, but
// in that case we return false immediately, so we do not need to root
// |src| and |dest|.  These SkipRoots are to protect from the unconditional
// MaybeCheckStackRoots done by ToNumber.
SkipRoot skipSrc(cx, &src);
SkipRoot skipDest(cx, &dest);
</code>

I also changed the comment a little bit to say why we need the SkipRoots, because the existing comment strongly implies that they should *not* have SkipRoots.  Feel free to clean this up more if you can express the idea more clearly.

Comment 3

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/40b01e566af0

try push: https://tbpl.mozilla.org/?tree=Try&rev=4762cd73c124

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/40b01e566af0