832363 - GC: Rooting updates to jsdate.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Update the rooting in jsdate.cpp according to the static rooting analysis.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 703945 [details] [diff] [review]
Proposed fix

Steve can you check that this is ok?  Do we want to be fixing up the unnecessary rooting reported, or shall we leave that for now?

Comment 3

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

Comment on attachment 703945 [details] [diff] [review]
Proposed fix

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

r=me with those 2 Unrooteds changed to Raws.

::: js/src/jsdate.cpp
@@ +767,5 @@
>   *   TZD  = time zone designator (Z or +hh:mm or -hh:mm or missing for local)
>   */
>  
>  static JSBool
> +date_parseISOString(UnrootedLinearString str, double *result, DateTimeInfo *dtInfo)

When there's no cx being passed in and no implicit cx being used, I'd definitely rather use Raw for these instead of Unrooted. Unrooted implies to me that there *is* some GCing possible, it just isn't in the region covered by the Unrooted.

More generally, it sounds like we're now saying that maybe it's not worth using Unrooted (vs Raw) anywhere. But I haven't gotten that far yet; I'll probably continue to use Unrooted in places where it's simple and doesn't cause any trouble.

@@ +906,5 @@
>  #undef NEED_NDIGITS
>  }
>  
>  static JSBool
> +date_parseString(UnrootedLinearString str, double *result, DateTimeInfo *dtInfo)

same here

@@ +3060,5 @@
>              UnrootedString str = args[0].toString();
>              if (!str)
>                  return false;
>  
> +            UnrootedLinearString linearStr = DropUnrooted(str)->ensureLinear(cx);

I would r+ either way, but |str| is an example of something that I'd probably switch to RawString now. The DropUnrooted is going to confuse people who aren't familiar with it, and it's not really doing any good here. (In fact, it's somewhat implying that ensureLinear could GC, which is not the case.)

Comment 4

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

(In reply to Jon Coppeard (:jonco) from comment #2)
> Comment on attachment 703945 [details] [diff] [review]
> Proposed fix
> 
> Steve can you check that this is ok?  Do we want to be fixing up the
> unnecessary rooting reported, or shall we leave that for now?

I agree with you. I'm not planning on fixing over-rooting yet. I'll worry about that when we have proper rooting out of the way, so it unblocks other things.

Comment 5

[Jon Coppeard (:jonco)]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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