Fixing crash in V8ValueConverter.

Fixing crash in V8ValueConverter.

It appears that the crash occurs when the V8ValueConverter is passed an object
that has a named callback (v8::Object::HasRealNamedCallbackProperty()) that
is an internal property (something that is not defined in JS, but intercepted by
the c++ side).

Something like a DOM input element is a good example of this.
The crash doesn't happen with every object that has internal properties and
named callbacks, but so far I am unable to figure out how to programatically
differentiate between the ones that will crash and the ones that won't.

My current solution is to not convert any named callback.
BUG=139933
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150035

[Aaron Boodman, 2012-08-02 18:13:44]

https://chromiumcodereview.appspot.com/10837066/diff/1/content/renderer/v8_va...
File content/renderer/v8_value_converter_impl.cc (right):

https://chromiumcodereview.appspot.com/10837066/diff/1/content/renderer/v8_va...
content/renderer/v8_value_converter_impl.cc:323: if (!key->IsString() ||
Would be more clear like:

// base::DictionaryValue can only have string properties.
// Anyway, this is probably an Array.
if (!key->IsString())
  continue;

// Whatever the reason for this is here...
if (!val->HasRealNamedProperty(key->ToString())
  continue;

... etc ...

https://chromiumcodereview.appspot.com/10837066/diff/1/content/renderer/v8_va...
content/renderer/v8_value_converter_impl.cc:325:
(val->HasRealNamedCallbackProperty(key->ToString()) &&
I think it is fine to just skip getters all together.

[not at google - send to devlin, 2012-08-02 18:59:41]

https://chromiumcodereview.appspot.com/10837066/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/callback/test.js
(right):

https://chromiumcodereview.appspot.com/10837066/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/callback/test.js:160:
chrome.test.assertTrue(scriptVal[0] != null);
drive-by: should this be a V8UnitTest?

[eaugusti, 2012-08-02 19:44:53]

https://chromiumcodereview.appspot.com/10837066/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/executescript/callback/test.js
(right):

https://chromiumcodereview.appspot.com/10837066/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/api_test/executescript/callback/test.js:160:
chrome.test.assertTrue(scriptVal[0] != null);
On 2012/08/02 18:59:41, kalman wrote:
> drive-by: should this be a V8UnitTest?

Yes, it should be. It is just hard because it is actually WebKit that is
registering the named callbacks that are causing the crash. So right now all I
have is a unittest that says "Don't try and convert named callbacks".

https://chromiumcodereview.appspot.com/10837066/diff/1/content/renderer/v8_va...
File content/renderer/v8_value_converter_impl.cc (right):

https://chromiumcodereview.appspot.com/10837066/diff/1/content/renderer/v8_va...
content/renderer/v8_value_converter_impl.cc:323: if (!key->IsString() ||
On 2012/08/02 18:13:44, Aaron Boodman wrote:
> Would be more clear like:
> 
> // base::DictionaryValue can only have string properties.
> // Anyway, this is probably an Array.
> if (!key->IsString())
>   continue;
> 
> // Whatever the reason for this is here...
> if (!val->HasRealNamedProperty(key->ToString())
>   continue;
> 
> ... etc ...

Done.

https://chromiumcodereview.appspot.com/10837066/diff/1/content/renderer/v8_va...
content/renderer/v8_value_converter_impl.cc:325:
(val->HasRealNamedCallbackProperty(key->ToString()) &&
On 2012/08/02 18:13:44, Aaron Boodman wrote:
> I think it is fine to just skip getters all together.

Done.

[asargent_no_longer_on_chrome, 2012-08-03 19:35:22]

LGTM

[not at google - send to devlin, 2012-08-03 19:38:55]

(responding to bug)

I'm not comfortable lging this because it's Aaron's review, and I think he'll be
back online before getting on the plane. I am about to be offline for 24 hours,
though.


However, now that I'm here... my 2c is that I don't think we should be disabling
named getters in V8ValueConverter, since it's how we serialise all things from
v8 to C++.

One example where it may be detrimental: the storage API converts properties to
JSON when it stores them. I think it's legitimate to want to store something
like

{get width() { return 10; }}

because there's nothing wrong with it, because it might happen and it would be a
surprising not to work, and because JSON.stringify works fine with getters.

And I only mention the storage API because that's what I'm familiar with, there
are possibly other things (even beyond extensions?) that may be affected.

So yeah, would loop detection in the conversion be a more general solution?

[not at google - send to devlin, 2012-08-03 19:40:11]

Sorry, I note that you've said in your CL description that it *does* still allow
for normal named callbacks. Ignore me.

[eaugusti, 2012-08-03 19:42:59]

On 2012/08/03 19:40:11, kalman wrote:
> Sorry, I note that you've said in your CL description that it *does* still
allow
> for normal named callbacks. Ignore me.

No, you are correct. As per Aaron's comment, I disabled all named callbacks (but
forgot to update the description). If you need them, let me re-enable them.

[eaugusti, 2012-08-03 20:42:55]

Ok, now we have getters again for objects that do not have internal fields.

[Aaron Boodman, 2012-08-04 11:34:18]

LGTM, but before landing, please put it back to just skipping all getters. I'd
rather be consistent and not support getters at all for this first patch. And
kalman doesn't need this feature now, he's just saying that it would be nice to
have.

After that lands and fixes the bug, if you want to support getters, then you
should add a max_recursion_depth feature to V8ValueConverter which guards
against infinitely recursive getters in the general case.

https://chromiumcodereview.appspot.com/10837066/diff/2003/content/renderer/v8...
File content/renderer/v8_value_converter_impl.cc (right):

https://chromiumcodereview.appspot.com/10837066/diff/2003/content/renderer/v8...
content/renderer/v8_value_converter_impl.cc:325:
val->HasRealNamedCallbackProperty(key->ToString()))
Please add braces since the if statement is more than two lines total.

[Aaron Boodman, 2012-08-04 11:36:12]

On 2012/08/03 19:38:55, kalman wrote:
> So yeah, would loop detection in the conversion be a more general solution?

We already have loop detection. It's either not detecting this because there is
no loop (it's just infinitely recursive), or because it's buggy. In any case,
the first priority is to fix the crash.

[eaugusti, 2012-08-04 19:10:37]

https://chromiumcodereview.appspot.com/10837066/diff/2003/content/renderer/v8...
File content/renderer/v8_value_converter_impl.cc (right):

https://chromiumcodereview.appspot.com/10837066/diff/2003/content/renderer/v8...
content/renderer/v8_value_converter_impl.cc:325:
val->HasRealNamedCallbackProperty(key->ToString()))
On 2012/08/04 11:34:19, Aaron Boodman wrote:
> Please add braces since the if statement is more than two lines total.

Done.

[jamesr, 2012-08-04 19:26:13]

lgtm

[commit-bot: I haz the power, 2012-08-04 19:31:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eaugusti@chromium.org/10837066/11002

[commit-bot: I haz the power, 2012-08-05 00:30:13]

Change committed as 150035