798624 - GC: specialize low-level character access for JSStableString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

If we specialize JSStableString::chars() and push StableCharPtr into the interface at several key choke-points we can use the type system to enforce the requirement that we have uninlined jschar*s in places that we may GC with a jschar* on the stack.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0

This handles the simple cases.  There is still RegExpObject, but it requires an atom, so is going to need a more involved change.  My plan is to implement JSStableAtom and refactor AtomizeInline [/me shudders], but that can come later.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 668658 [details] [diff] [review]
v0

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

This is great: typier and assertier!  My nits are basically just to push StableCharPtr into a few more interfaces that currently take 'const jschar *':

(I assume UnstableCharPtr use is coming later?)

::: js/src/frontend/BytecodeCompiler.cpp
@@ +94,5 @@
>        case CompileOptions::NO_SOURCE:
>          break;
>      }
>  
> +    Parser parser(cx, options, chars.get(), length, /* foldConstants = */ true);

Parser may want to store a const jschar * internally, but it makes sense for it to take a StableCharPtr in the constructor interface.

::: js/src/jsclone.cpp
@@ +1004,5 @@
>              return false;
>  
>          size_t length = stable->length();
> +        const StableCharPtr chars = stable->chars();
> +        RegExpObject *reobj = RegExpObject::createNoStatics(context(), chars.get(), length, flags, NULL);

For the same reason about, it seems like RegExpObject::createNoStatics would take a stable pointer (as a semantic interface requirement) and use .get() internally.

::: js/src/jscompartment.cpp
@@ +279,5 @@
>          RootedValue orig(cx, *vp);
>          JSStableString *str = vp->toString()->ensureStable(cx);
>          if (!str)
>              return false;
> +        JSString *wrapped = js_NewStringCopyN(cx, str->chars().get(), str->length());

ditto

Comment 3

[Terrence Cole [:terrence]]

(In reply to Luke Wagner [:luke] from comment #2)
> (I assume UnstableCharPtr use is coming later?)

That is the hope, but I'm not sure how likely it is anymore.  My previous effort to convert all the jschar* interfaces at once was starting to get seriously overweight.  When I realized we could get most of the benefits using, well, this patch, I decided to split this off and get it in first.  Converting all of our internal jschar* use to RangedPtr is a noble goal, but it isn't on the shortest path to exact rooting anymore.  I could include it to make things easier to future or remove it since it's unused in the interim - which would be better?
 
> ::: js/src/jscompartment.cpp
> @@ +279,5 @@
> >          RootedValue orig(cx, *vp);
> >          JSStableString *str = vp->toString()->ensureStable(cx);
> >          if (!str)
> >              return false;
> > +        JSString *wrapped = js_NewStringCopyN(cx, str->chars().get(), str->length());
> 
> ditto

Ah, good catch!  This one hadn't occurred to me because most of the users are for buffers.

Comment 4

[Luke Wagner [:luke]]

(In reply to Terrence Cole [:terrence] from comment #3)
> That is the hope, but I'm not sure how likely it is anymore.  My previous
> effort to convert all the jschar* interfaces at once was starting to get
> seriously overweight.

I bet.  I wasn't asking for internal use; more for JSAPI public use.  If we don't do the latter, then perhaps we could leave UnstableCharPtr out.

Comment 5

[Terrence Cole [:terrence]]

Attachment: v0: Fix for a busted assertion in RangedPtr.h.

In RegExpExecute we have:
size_t displacement = 0;
.... stuff that may or may not modify displacement ....
chars += displacement;

RangedPtr.h is asserting on this code in operator+/- that the offset is strictly > 0, which is just wrong.

Setting rs=Waldo from IRC.

Comment 6

[Terrence Cole [:terrence]]

Attachment: nits

The nit corrections turned into a 40k patch.  There is nothing controversial in it, I'm just putting it here for completeness' sake.

I also opened new bug 799248.  Making NewString GC safe is going to need another review pass and I don't want to make you have to wade through all of these mechanical changes a second time.

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/754a1efb5b37

Comment 8

[Terrence Cole [:terrence]]

And backed out for CTypes build failure in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14684be81166

Comment 9

[Terrence Cole [:terrence]]

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=68b7150624f1

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c08d52e521d

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2c08d52e521d

Comment 11

[Scoobidiver (away)]

Should this bug be reopened because of the backout landed in bug 837845?

Comment 12

[Terrence Cole [:terrence]]

Don't bother. The real solution to this problem is considerably more labor intensive, which is why we took this path in the first place. Until then we will simply not store strings in the nursery. I'll open a new blocking bug later when we have the time to work on that.