829898 - Various rooting in jsstr.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Pass MutableHandleValue to Encode, Decode & TransferBufferToString

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Pass a HandleLinearString to Encode & Decode and remove pointless OOM check

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Root various strings around the file

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 701496 [details] [diff] [review]
Part a: Pass MutableHandleValue to Encode, Decode & TransferBufferToString

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

Nice.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 701497 [details] [diff] [review]
Part b: Pass a HandleLinearString to Encode & Decode and remove pointless OOM check

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

Oh, very nice indeed.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 701498 [details] [diff] [review]
Part c: Root various strings around the file

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

::: js/src/jsstr.cpp
@@ +377,3 @@
>          if (!str1)
>              return false;
>          value.setString(str1);

Make this |value.setString(DropUnrooted(str1));| to end the Unrooted's assertion region before we get to defineElement.

@@ +2692,5 @@
>                  if (!matches[i + 1].isUndefined()) {
>                      JSSubString parsub;
>                      res->getParen(i + 1, &parsub);
> +                    UnrootedString sub2 = js_NewStringCopyN(cx, parsub.chars, parsub.length);
> +                    if (!sub2 || !splits.append(StringValue(DropUnrooted(sub2))))

No need for DropUnrooted here: sub2 is going to go out of scope before we can GC.

@@ -2917,5 @@
>          if (!str)
>              return false;
>      }
>  
> -out:

\o/

@@ +2952,5 @@
>  str_slice(JSContext *cx, unsigned argc, Value *vp)
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
> +    // Common case.

That doesn't look like a spec step to me ;-). Lets leave this out.

@@ +2957,2 @@
>      if (args.length() == 1 && args.thisv().isString() && args[0].isInt32()) {
> +        RootedString str(cx, args.thisv().toString());

This is a fast path with an obvious rooting story, so just make this RawString and we'll let the rooting analysis protect it.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/11192d6d324d
https://hg.mozilla.org/mozilla-central/rev/0bf4b7cd6f82
https://hg.mozilla.org/mozilla-central/rev/6d2b3fff3628

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2b3fff3628