Closed Bug 829898 Opened 7 years ago Closed 7 years ago

Various rooting in jsstr.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(3 files)

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

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

Nice.
Attachment #701496 - Flags: review?(terrence) → review+
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.
Attachment #701497 - Flags: review?(terrence) → review+
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.
Attachment #701498 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/11192d6d324d
https://hg.mozilla.org/mozilla-central/rev/0bf4b7cd6f82
https://hg.mozilla.org/mozilla-central/rev/6d2b3fff3628
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.