The interpreter engine for the core JavaScript language, independent of the browser's object model. File ONLY core JavaScript language bugs in this category. For bugs involving browser objects such as "window" and "document", use the "DOM" component. For bugs involving calls between JavaScript and C++, use the "XPConnect" component.

Created attachment 8438316[details][diff][review]
Part 1 - Add JSONParserBase class
This class adds JSONParserBase as we discussed. This class holds all state and methods that can be shared between the two encodings.

Created attachment 8438333[details][diff][review]
Part 2 - Parameterize JSONParser
With this patch everything compiles, because we only instantiate JSONParser<jschar>, but we need more work to handle other char types.

Created attachment 8438473[details][diff][review]
Part 5 - Add AutoStableStringChars
AutoStableStringChars gives us access to a string's chars across potential GCs.
Once we allocate strings and chars in the nursery, AutoStableStringChars will have to make a copy of the chars. I think/hope this will be fine; only relatively small strings will have their chars allocated in the nursery, and I think there will be very few places where we need this class.

Comment on attachment 8438473[details][diff][review]
Part 5 - Add AutoStableStringChars
Review of attachment 8438473[details][diff][review]:
-----------------------------------------------------------------
r=me
I think once we store string data in the nursery, we'll also need to make this a CustomRooted and make ::trace update s_. That will also require updating the analysis, so we should wait on that -- for now this is a perfect placeholder.
::: js/src/vm/String.h
@@ +1072,5 @@
> + State state_;
> + bool ownsChars_;
> +
> + public:
> + explicit AutoStableStringChars(JSContext *cx, JSLinearString *s)
|explicit| is only required on one-arg constructors. (Although it's fine if we've decided to put it unconditionally on all constructors and I just missed the memo.)

(In reply to Terrence Cole [:terrence] from comment #9)
> I think once we store string data in the nursery, we'll also need to make
> this a CustomRooted and make ::trace update s_. That will also require
> updating the analysis, so we should wait on that -- for now this is a
> perfect placeholder.
My plan was to have AutoStableStringChars::init* copy the chars to some malloc'ed buffer for nursery-allocated chars, and then never touch the string again (the RootedString is only there to keep it alive). Why would we need the CustomRooted/trace?
> |explicit| is only required on one-arg constructors. (Although it's fine if
> we've decided to put it unconditionally on all constructors and I just
> missed the memo.)
Initially the constructor had one argument and the string was passed to init. Then I moved the argument to the constructor and forgot to remove the explicit, will fix.
Thanks for the quick review!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> Why won't JSONParser::NoError still work? Several places in JSONParser.cpp
> have the same sort of thing in play.
It'd have to be JSONParser<CharT>::NoError or JSONParser<jschar>::NoError with the other patches.
> It is incredibly strange that TwoByteChars is a Range, yet ConstTwoByteChars
> is a RangedPtr. Both should be ranges. Please fix this in a separate
> patch. For now, I'll let this slide.
Yes I noticed it too; pretty weird. The other patches rewrite all this code to use Range/RangedPtr directly though.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> Are we to the point of not scanning the stack these days, I think? If so,
> preserving the Rooted above and getting rid of this Anchor seems good.
>
> Or, no, ASSC contains a Rooted. Is there a way we can have the ensureFlat
> flow directly into that Rooted or so, somehow? Musing, mostly.
I think it's good to keep the Anchor a little longer until the conservative scanner dies, hopefully soon. I don't think there's a nice way to make the result of ensureFlat flow into that Rooted, because we also need the OOM check...
> Could latin1Chars() return a Range, maybe?
>
> Would also be nice for twoByteChars to return a Range.
Yes, nice idea.

Created attachment 8439101[details][diff][review]
Part 8 - Address comments
The JSONParser constructor now takes a Range instead of RangedPtr + length. I also added latin1Range and twoByteRange methods to AutoStableStringChars (I want to keep the jschar*/Latin1Char* methods to avoid .start().get() etc when we don't want a Range).
I also tried to make ConstTwoByteChars a Range instead of RangedPtr, but it requires a bunch of changes everywhere and as it's a pre-existing issue I think this is better done as a follow-up bug.

(In reply to Jan de Mooij [:jandem] from comment #15)
> (I want to keep the jschar*/Latin1Char* methods to avoid .start().get() etc
> when we don't want a Range).
Actually it feels to me like exposing the data *only* as a Range is nice, in that users will always have both at the ready when needed. The raw pointer has no real use on its own, only in connection with its extent of validity.
As for .start().get(), I think we need to push RangedPtr down into users a bit harder, so this extra typing isn't needed. Worth keeping in mind for patches and reviews, I think.
> I also tried to make ConstTwoByteChars a Range instead of RangedPtr, but it
> requires a bunch of changes everywhere and as it's a pre-existing issue I
> think this is better done as a follow-up bug.
Fine by me.