Soft fail for invalid cache data.

Soft fail for invalid cache data.

API=ScriptCompiler::CachedData::rejected
LOG=Y
R=vogelheim@google.com, vogelheim@chromium.org

Committed: https://chromium.googlesource.com/v8/v8/+/eafce666f49f13011849b6c0c40b271676ec91cf

[vogelheim, 2014-11-13 13:01:15]

https://codereview.chromium.org/724023002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/724023002/diff/1/src/api.cc#newcode1759
src/api.cc:1759: source->cached_data->rejected = (script_data == NULL);
I find this a bit weird...

- I expected ..->rejected to be set at the location where the cached_data was
actually rejected.
- Instead, that code deletes the reference to cached_data, but not the
cached_data object.
- Presence of cached_data is then treated as an error code of sorts.
  - This would also trigger if has_pending_execution.
    (See comment above.)

I'm uneasy about this, but, honestly, given the current code structure I don't
see a super elegant way around this, so I'm a bit unsure on what to advise...

https://codereview.chromium.org/724023002/diff/1/src/api.cc#newcode1761
src/api.cc:1761: delete script_data;
If script_data was nullified along the way, this is a noop. Who deletes it then?

https://codereview.chromium.org/724023002/diff/1/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/724023002/diff/1/src/parser.h#newcode68
src/parser.h:68: *cached_data = NULL;
Memory leak?
Who owns/deletes (*cached_data)?

[vogelheim, 2014-11-13 13:03:01]

https://codereview.chromium.org/724023002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/724023002/diff/1/src/api.cc#newcode1759
src/api.cc:1759: source->cached_data->rejected = (script_data == NULL);
If ScriptData is the "internal" structure (vs CachedData as an "API" structure)
we use to pass around all pertinent information, then maybe ScriptData should
have a rejected flag, too, or maybe just a bool* reference to CachedData's
rejected instance?

[Yang, 2014-11-13 13:04:19]

On 2014/11/13 13:03:01, vogelheim wrote:
> https://codereview.chromium.org/724023002/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/724023002/diff/1/src/api.cc#newcode1759
> src/api.cc:1759: source->cached_data->rejected = (script_data == NULL);
> If ScriptData is the "internal" structure (vs CachedData as an "API"
structure)
> we use to pass around all pertinent information, then maybe ScriptData should
> have a rejected flag, too, or maybe just a bool* reference to CachedData's
> rejected instance?

I guess we can do that as well. Currently I'm just setting it to null. I'll
change that.

[Yang, 2014-11-13 13:14:11]

On 2014/11/13 13:04:19, Yang wrote:
> On 2014/11/13 13:03:01, vogelheim wrote:
> > https://codereview.chromium.org/724023002/diff/1/src/api.cc
> > File src/api.cc (right):
> > 
> > https://codereview.chromium.org/724023002/diff/1/src/api.cc#newcode1759
> > src/api.cc:1759: source->cached_data->rejected = (script_data == NULL);
> > If ScriptData is the "internal" structure (vs CachedData as an "API"
> structure)
> > we use to pass around all pertinent information, then maybe ScriptData
should
> > have a rejected flag, too, or maybe just a bool* reference to CachedData's
> > rejected instance?
> 
> I guess we can do that as well. Currently I'm just setting it to null. I'll
> change that.

Changed as suggested.

[vogelheim1, 2014-11-13 13:50:21]

vogelheim@google.com changed reviewers:
+ vogelheim@google.com

[vogelheim1, 2014-11-13 13:50:22]

lgtm

https://codereview.chromium.org/724023002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/724023002/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:24223: CHECK(cached_data->rejected);
Maybe there should also be a test for CHECK(!rejected) after a successful
ScriptCompiler::Compile. You could add the check to one of the existing tests in
test-serialize.cc or test-parsing.cc

[Yang, 2014-11-13 15:42:57]

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

[vogelheim, 2014-11-13 16:35:58]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/724083003/ by vogelheim@chromium.org.

The reason for reverting is: Build bots break due to memory leak..