Parser & internalization fix: ensure no heap allocs during GetString(Handle<String>).

Parser & internalization fix: ensure no heap allocs during GetString(Handle<String>).

The bug has always been there: when the parser is operating in the "immediately
internalize" mode and calls GetString, we get FlatContent of a string and then
do heap allocation.

The bug was uncovered by https://codereview.chromium.org/693803004/ (which put
the parser to the "immediately internalize" mode more often), but looking at the
code, it's possible that it can happen in other cases too.

This CL makes AstValueFactory handle this situation gracefully: it won't try to
internalize inside GetString(Handle<String>); it's unnecessary anyway since we
have the Handle<String> already.

R=rossberg@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=25155

[marja, 2014-11-05 10:43:45]

rossberg, ptal.

My efficiency fixes yesterday got reverted because they uncovered this bug;
after this fix I can reland them as is.

[rossberg, 2014-11-05 12:39:51]

https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc
File src/ast-value-factory.cc (left):

https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc#old...
src/ast-value-factory.cc:235: return
GetOneByteString(content.ToOneByteVector());
I'm confused, what's the reason to inline these functions? Unless I'm missing
something, the code is identical.

[marja, 2014-11-05 12:41:42]

https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc
File src/ast-value-factory.cc (left):

https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc#old...
src/ast-value-factory.cc:235: return
GetOneByteString(content.ToOneByteVector());
On 2014/11/05 12:39:51, rossberg wrote:
> I'm confused, what's the reason to inline these functions? Unless I'm missing
> something, the code is identical.

constness.

Another option would be to make these API functions return AstRawString* instead
of const AstRawString* but that's also somewhat meh.

(The users are anyway supposed to e.g., internalize the AstRawStrings they get,
so returning const AstRawString* is more correct..)

[marja, 2014-11-05 12:44:30]

On 2014/11/05 12:41:42, marja wrote:
> https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc
> File src/ast-value-factory.cc (left):
> 
>
https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc#old...
> src/ast-value-factory.cc:235: return
> GetOneByteString(content.ToOneByteVector());
> On 2014/11/05 12:39:51, rossberg wrote:
> > I'm confused, what's the reason to inline these functions? Unless I'm
missing
> > something, the code is identical.
> 
> constness.
> 
> Another option would be to make these API functions return AstRawString*
instead
> of const AstRawString* but that's also somewhat meh.
> 
> (The users are anyway supposed to e.g., internalize the AstRawStrings they
get,
> so returning const AstRawString* is more correct..)

... oops, meant to say: the users *aren't* supposed to <call non-const
operations>

[rossberg, 2014-11-05 12:46:21]

https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc
File src/ast-value-factory.cc (left):

https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc#old...
src/ast-value-factory.cc:235: return
GetOneByteString(content.ToOneByteVector());
On 2014/11/05 12:41:41, marja wrote:
> On 2014/11/05 12:39:51, rossberg wrote:
> > I'm confused, what's the reason to inline these functions? Unless I'm
missing
> > something, the code is identical.
> 
> constness.
> 
> Another option would be to make these API functions return AstRawString*
instead
> of const AstRawString* but that's also somewhat meh.
> 
> (The users are anyway supposed to e.g., internalize the AstRawStrings they
get,
> so returning const AstRawString* is more correct..)

Why not make the functions non-const but private, and have the public ones be
thin const wrappers?

[marja, 2014-11-05 12:55:30]

On 2014/11/05 12:46:21, rossberg wrote:
> https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc
> File src/ast-value-factory.cc (left):
> 
>
https://codereview.chromium.org/699343004/diff/1/src/ast-value-factory.cc#old...
> src/ast-value-factory.cc:235: return
> GetOneByteString(content.ToOneByteVector());
> On 2014/11/05 12:41:41, marja wrote:
> > On 2014/11/05 12:39:51, rossberg wrote:
> > > I'm confused, what's the reason to inline these functions? Unless I'm
> missing
> > > something, the code is identical.
> > 
> > constness.
> > 
> > Another option would be to make these API functions return AstRawString*
> instead
> > of const AstRawString* but that's also somewhat meh.
> > 
> > (The users are anyway supposed to e.g., internalize the AstRawStrings they
> get,
> > so returning const AstRawString* is more correct..)
> 
> Why not make the functions non-const but private, and have the public ones be
> thin const wrappers?

That would work too; see the latest patch set.

[rossberg, 2014-11-05 13:19:48]

lgtm

[marja, 2014-11-05 14:00:01]

Committed patchset #2 (id:20001) manually as 25155 (presubmit successful).