V8ValueConverter for the activity logger that does not invoke interceptors and

V8ValueConverter for the activity logger that does not invoke interceptors and
accessors when converting V8 values to base values.

BUG=260978, 259093
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217032

[pmarch, 2013-07-18 16:12:30]

HI Adrienne and Ankur

Could you please review this CL.

Thanks

[felt, 2013-07-18 17:28:20]

Hi Petr,

It looks like the end to end test is failing now, you'll need to update it to
work with your patch. As of last week, the API test now checks that the proper
API calls are recorded relative to a given test. Some of the tests had been
generating extraneous additional calls (like document.locaiton in
triggerDOMChangesOnTabsUpdated) so they were added as the "correct" responses.
Now those calls have disappeared. I think what is happening is that they should
not have been there in the first place -- they were artifacts of the value
conversion. So these statements need to be removed from the end-to-end test.

Also, a few nits below.

Adrienne

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/activity_log_value_converter.h (right):

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter.h:18: class
ActivityLogValueConverter {
Can you add a comment to explain why this new class is needed? You could
probably lift some of the text from the long comment in the cc file (which I
think does a fairly good job explaining).

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter.h:33: #endif  //
CHROME_RENDERER_EXTENSIONS_ACTIVITY_LOG_VALUE_CONVERTER_H_
linter is complaining about lack of a new line at the end of the file

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/activity_log_value_converter_unittest.cc
(right):

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter_unittest.cc:55: "list: [
\"monkey\", \"balls\" ],"
hahahaha interesting choice of words for your test

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter_unittest.cc:161: }  //
namespace extensions
linter is complaining about lack of a new line at the end of the file

[pmarch, 2013-07-18 20:42:03]

updated end-to-end tests

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/activity_log_value_converter.h (right):

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter.h:18: class
ActivityLogValueConverter {
On 2013/07/18 17:28:20, felt wrote:
> Can you add a comment to explain why this new class is needed? You could
> probably lift some of the text from the long comment in the cc file (which I
> think does a fairly good job explaining).

Done.

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter.h:33: #endif  //
CHROME_RENDERER_EXTENSIONS_ACTIVITY_LOG_VALUE_CONVERTER_H_
On 2013/07/18 17:28:20, felt wrote:
> linter is complaining about lack of a new line at the end of the file

Done.

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/activity_log_value_converter_unittest.cc
(right):

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter_unittest.cc:55: "list: [
\"monkey\", \"balls\" ],"
On 2013/07/18 17:28:20, felt wrote:
> hahahaha interesting choice of words for your test
hahaha, I copied it over from another test and did not notice this before you
mentioned it.

https://codereview.chromium.org/19730002/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter_unittest.cc:161: }  //
namespace extensions
On 2013/07/18 17:28:20, felt wrote:
> linter is complaining about lack of a new line at the end of the file

Done.

[felt, 2013-07-18 22:04:33]

lgtm

If you don't already have an owner reviewer in mind, Ben Kalman might be a good
person to do it

[pmarch, 2013-07-18 22:31:52]

Hi Ben

Could you please review my CL

Thanks

[not at google - send to devlin, 2013-07-19 01:15:45]

I think i'll need to spend some more time with this patch (I don't fully
understand the problem, but note that we've had issues with putting DOM elements
through v8 value converter before) - it's a bit too complex for the shuttle ride
home - but I would prefer to put whatever solution as a flag within v8 value
converter to make it discoverable.

https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
File chrome/renderer/extensions/activity_log_value_converter.cc (right):

https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_value_converter.cc:30: // DOM object. V8
API does not allow to check if a property has a getter
I believe this will return false for getters:

https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...

[pmarch, 2013-07-19 02:57:35]

On 2013/07/19 01:15:45, kalman wrote:
> I think i'll need to spend some more time with this patch (I don't fully
> understand the problem, but note that we've had issues with putting DOM
elements
> through v8 value converter before) - it's a bit too complex for the shuttle
ride
> home - but I would prefer to put whatever solution as a flag within v8 value
> converter to make it discoverable.
> 
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> File chrome/renderer/extensions/activity_log_value_converter.cc (right):
> 
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> chrome/renderer/extensions/activity_log_value_converter.cc:30: // DOM object.
V8
> API does not allow to check if a property has a getter
> I believe this will return false for getters:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...

Thanks for the pointer on HasRealNamedProperty

We have the following problem when ActivityLog logs JSObjects using
V8ValueConverter. If the object has a getter method installed, the getter method
is invoked by the V8ValueConverter. Then, if the getter method accesses other
JSObjects that happens to be wrappers of DOM elements these accesses are logged
as well thus producing bogus log entrees caused by the V8ValueConverter, but not
an extension. We also observed recursive ActivityLog logging because of the
V8ValueConverter. There is also another problem: an extension developer may not
expect a getter method to be invoked in certain places which may happen because
of the V8ValueConverter. Thus, the converter may violate the extension developer
expectation and clobber extension's execution environment by invoking a getter
method.

[pmarch, 2013-07-19 17:29:24]

On 2013/07/19 02:57:35, pmarch wrote:
> On 2013/07/19 01:15:45, kalman wrote:
> > I think i'll need to spend some more time with this patch (I don't fully
> > understand the problem, but note that we've had issues with putting DOM
> elements
> > through v8 value converter before) - it's a bit too complex for the shuttle
> ride
> > home - but I would prefer to put whatever solution as a flag within v8 value
> > converter to make it discoverable.
> > 
> >
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> > File chrome/renderer/extensions/activity_log_value_converter.cc (right):
> > 
> >
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> > chrome/renderer/extensions/activity_log_value_converter.cc:30: // DOM
object.
> V8
> > API does not allow to check if a property has a getter
> > I believe this will return false for getters:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...
> 
> Thanks for the pointer on HasRealNamedProperty
> 
> We have the following problem when ActivityLog logs JSObjects using
> V8ValueConverter. If the object has a getter method installed, the getter
method
> is invoked by the V8ValueConverter. Then, if the getter method accesses other
> JSObjects that happens to be wrappers of DOM elements these accesses are
logged
> as well thus producing bogus log entrees caused by the V8ValueConverter, but
not
> an extension. We also observed recursive ActivityLog logging because of the
> V8ValueConverter. There is also another problem: an extension developer may
not
> expect a getter method to be invoked in certain places which may happen
because
> of the V8ValueConverter. Thus, the converter may violate the extension
developer
> expectation and clobber extension's execution environment by invoking a getter
> method.

Ben,

Since there is a way to check if there is a getter installed on a JS object
using the V8 API (which I did not know until you reference to the appropriate
API call) the alternative to this patch may be to modify V8ValueConverter and
introduce a flag. The flag will force V8ValueConverter to check if there is a
getter method present and if so and the flag is set, we do not call the method
and return null for this property.

[not at google - send to devlin, 2013-07-19 17:36:21]

On 2013/07/19 17:29:24, pmarch wrote:
> On 2013/07/19 02:57:35, pmarch wrote:
> > On 2013/07/19 01:15:45, kalman wrote:
> > > I think i'll need to spend some more time with this patch (I don't fully
> > > understand the problem, but note that we've had issues with putting DOM
> > elements
> > > through v8 value converter before) - it's a bit too complex for the
shuttle
> > ride
> > > home - but I would prefer to put whatever solution as a flag within v8
value
> > > converter to make it discoverable.
> > > 
> > >
> >
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> > > File chrome/renderer/extensions/activity_log_value_converter.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> > > chrome/renderer/extensions/activity_log_value_converter.cc:30: // DOM
> object.
> > V8
> > > API does not allow to check if a property has a getter
> > > I believe this will return false for getters:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...
> > 
> > Thanks for the pointer on HasRealNamedProperty
> > 
> > We have the following problem when ActivityLog logs JSObjects using
> > V8ValueConverter. If the object has a getter method installed, the getter
> method
> > is invoked by the V8ValueConverter. Then, if the getter method accesses
other
> > JSObjects that happens to be wrappers of DOM elements these accesses are
> logged
> > as well thus producing bogus log entrees caused by the V8ValueConverter, but
> not
> > an extension. We also observed recursive ActivityLog logging because of the
> > V8ValueConverter. There is also another problem: an extension developer may
> not
> > expect a getter method to be invoked in certain places which may happen
> because
> > of the V8ValueConverter. Thus, the converter may violate the extension
> developer
> > expectation and clobber extension's execution environment by invoking a
getter
> > method.
> 
> Ben,
> 
> Since there is a way to check if there is a getter installed on a JS object
> using the V8 API (which I did not know until you reference to the appropriate
> API call) the alternative to this patch may be to modify V8ValueConverter and
> introduce a flag. The flag will force V8ValueConverter to check if there is a
> getter method present and if so and the flag is set, we do not call the method
> and return null for this property.

SGTM.

Incidentally I wrote a patch a while ago which got reverted but which I was
eventually planning to revisit, which slightly pertains to this stuff:
https://codereview.chromium.org/16295013.

[pmarch, 2013-07-19 20:45:20]

On 2013/07/19 17:36:21, kalman wrote:
> On 2013/07/19 17:29:24, pmarch wrote:
> > On 2013/07/19 02:57:35, pmarch wrote:
> > > On 2013/07/19 01:15:45, kalman wrote:
> > > > I think i'll need to spend some more time with this patch (I don't fully
> > > > understand the problem, but note that we've had issues with putting DOM
> > > elements
> > > > through v8 value converter before) - it's a bit too complex for the
> shuttle
> > > ride
> > > > home - but I would prefer to put whatever solution as a flag within v8
> value
> > > > converter to make it discoverable.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> > > > File chrome/renderer/extensions/activity_log_value_converter.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/19730002/diff/19002/chrome/renderer/extension...
> > > > chrome/renderer/extensions/activity_log_value_converter.cc:30: // DOM
> > object.
> > > V8
> > > > API does not allow to check if a property has a getter
> > > > I believe this will return false for getters:
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...
> > > 
> > > Thanks for the pointer on HasRealNamedProperty
> > > 
> > > We have the following problem when ActivityLog logs JSObjects using
> > > V8ValueConverter. If the object has a getter method installed, the getter
> > method
> > > is invoked by the V8ValueConverter. Then, if the getter method accesses
> other
> > > JSObjects that happens to be wrappers of DOM elements these accesses are
> > logged
> > > as well thus producing bogus log entrees caused by the V8ValueConverter,
but
> > not
> > > an extension. We also observed recursive ActivityLog logging because of
the
> > > V8ValueConverter. There is also another problem: an extension developer
may
> > not
> > > expect a getter method to be invoked in certain places which may happen
> > because
> > > of the V8ValueConverter. Thus, the converter may violate the extension
> > developer
> > > expectation and clobber extension's execution environment by invoking a
> getter
> > > method.
> > 
> > Ben,
> > 
> > Since there is a way to check if there is a getter installed on a JS object
> > using the V8 API (which I did not know until you reference to the
appropriate
> > API call) the alternative to this patch may be to modify V8ValueConverter
and
> > introduce a flag. The flag will force V8ValueConverter to check if there is
a
> > getter method present and if so and the flag is set, we do not call the
method
> > and return null for this property.
> 
> SGTM.
> 
> Incidentally I wrote a patch a while ago which got reverted but which I was
> eventually planning to revisit, which slightly pertains to this stuff:
> https://codereview.chromium.org/16295013.

Ben,

I discussed this issue with my manager and we came up to the following solution.

We will be logging certain properties of DOM elements such as if a parameter
passed to a call that is being logged is an anchor element, we would like to log
its href property. The other DOM attributes that we are interested are "ID",
"SRC", "CLASS", and etc. The logic of deciding which attributes should be logged
will go to ActivityLogValueConverter class.

Therefore, implementing a flag for not invoking getters in V8ValueConverter is
not so useful for us now as we do care only about JS Objects that are wrappers
of DOM elements for now.
I added a note in the ActivityLogValueConverter file about the flag so we may
implement it later.

Would you be OK to have ActivityLogValueConverter?

[not at google - send to devlin, 2013-07-19 20:54:05]

> 
> Ben,
> 
> I discussed this issue with my manager and we came up to the following
solution.
> 
> We will be logging certain properties of DOM elements such as if a parameter
> passed to a call that is being logged is an anchor element, we would like to
log
> its href property. The other DOM attributes that we are interested are "ID",
> "SRC", "CLASS", and etc. The logic of deciding which attributes should be
logged
> will go to ActivityLogValueConverter class.
> 
> Therefore, implementing a flag for not invoking getters in V8ValueConverter is
> not so useful for us now as we do care only about JS Objects that are wrappers
> of DOM elements for now.
> I added a note in the ActivityLogValueConverter file about the flag so we may
> implement it later.
> 
> Would you be OK to have ActivityLogValueConverter?

The ID stuff makes sense, but I'd still rather we use V8ValueConverter for this
- strategy pattern would work well. Like

class Strategy {
 public:
  virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
                             v8::Handle<v8::String> key) = 0;
};

then implement your strategy as you want (the ID stuff).

My concern with implementing a separate converter for the activity log is
divergence. It looks like your implementation only handles the top level of an
object. What about if the DOM element is further down in the chain? The Strategy
approach would solve this automatically, and to solve that in a separate class
would require reimplementing the traversal logic.

[pmarch, 2013-07-19 22:08:18]

On 2013/07/19 20:54:05, kalman wrote:
> > 
> > Ben,
> > 
> > I discussed this issue with my manager and we came up to the following
> solution.
> > 
> > We will be logging certain properties of DOM elements such as if a parameter
> > passed to a call that is being logged is an anchor element, we would like to
> log
> > its href property. The other DOM attributes that we are interested are "ID",
> > "SRC", "CLASS", and etc. The logic of deciding which attributes should be
> logged
> > will go to ActivityLogValueConverter class.
> > 
> > Therefore, implementing a flag for not invoking getters in V8ValueConverter
is
> > not so useful for us now as we do care only about JS Objects that are
wrappers
> > of DOM elements for now.
> > I added a note in the ActivityLogValueConverter file about the flag so we
may
> > implement it later.
> > 
> > Would you be OK to have ActivityLogValueConverter?
> 
> The ID stuff makes sense, but I'd still rather we use V8ValueConverter for
this
> - strategy pattern would work well. Like
> 
> class Strategy {
>  public:
>   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
>                              v8::Handle<v8::String> key) = 0;
> };
> 
> then implement your strategy as you want (the ID stuff).
> 
> My concern with implementing a separate converter for the activity log is
> divergence. It looks like your implementation only handles the top level of an
> object. What about if the DOM element is further down in the chain? The
Strategy
> approach would solve this automatically, and to solve that in a separate class
> would require reimplementing the traversal logic.

If I want to log just a constructor name of a JS object, we will waist CPU
cycles by going through every property of the object with ShouldConvert()
returning false. Another example, if you give a DOM element to V8ValueConverter
you will convert the *whole* DOM, through its parent and children properties,
which we do not want to do.

It looks to me that I need to overwrite V8ValueConverterImpl::ToV8Object to
satisfy my needs. Alternatively, I can have a callback function in
V8ValueConverter, and if the callback is set, V8ValueConverter uses the callback
to convert JS objects instead of its default behavior.

What do you think about overwriting V8ValueConverterImpl::ToV8Object or
introducing a callback in V8ValueConverter?

[pmarch, 2013-07-19 22:11:04]

On 2013/07/19 22:08:18, pmarch wrote:
> On 2013/07/19 20:54:05, kalman wrote:
> > > 
> > > Ben,
> > > 
> > > I discussed this issue with my manager and we came up to the following
> > solution.
> > > 
> > > We will be logging certain properties of DOM elements such as if a
parameter
> > > passed to a call that is being logged is an anchor element, we would like
to
> > log
> > > its href property. The other DOM attributes that we are interested are
"ID",
> > > "SRC", "CLASS", and etc. The logic of deciding which attributes should be
> > logged
> > > will go to ActivityLogValueConverter class.
> > > 
> > > Therefore, implementing a flag for not invoking getters in
V8ValueConverter
> is
> > > not so useful for us now as we do care only about JS Objects that are
> wrappers
> > > of DOM elements for now.
> > > I added a note in the ActivityLogValueConverter file about the flag so we
> may
> > > implement it later.
> > > 
> > > Would you be OK to have ActivityLogValueConverter?
> > 
> > The ID stuff makes sense, but I'd still rather we use V8ValueConverter for
> this
> > - strategy pattern would work well. Like
> > 
> > class Strategy {
> >  public:
> >   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
> >                              v8::Handle<v8::String> key) = 0;
> > };
> > 
> > then implement your strategy as you want (the ID stuff).
> > 
> > My concern with implementing a separate converter for the activity log is
> > divergence. It looks like your implementation only handles the top level of
an
> > object. What about if the DOM element is further down in the chain? The
> Strategy
> > approach would solve this automatically, and to solve that in a separate
class
> > would require reimplementing the traversal logic.
> 
> If I want to log just a constructor name of a JS object, we will waist CPU
> cycles by going through every property of the object with ShouldConvert()
> returning false. Another example, if you give a DOM element to
V8ValueConverter
> you will convert the *whole* DOM, through its parent and children properties,
> which we do not want to do.
> 
> It looks to me that I need to overwrite V8ValueConverterImpl::ToV8Object to
> satisfy my needs. Alternatively, I can have a callback function in
> V8ValueConverter, and if the callback is set, V8ValueConverter uses the
callback
> to convert JS objects instead of its default behavior.
> 
> What do you think about overwriting V8ValueConverterImpl::ToV8Object or
> introducing a callback in V8ValueConverter?

V8ValueConverterImpl::ToV8Object ==> V8ValueConverterImpl::FromV8Object

[not at google - send to devlin, 2013-07-19 22:14:04]

On 2013/07/19 22:08:18, pmarch wrote:
> On 2013/07/19 20:54:05, kalman wrote:
> > > 
> > > Ben,
> > > 
> > > I discussed this issue with my manager and we came up to the following
> > solution.
> > > 
> > > We will be logging certain properties of DOM elements such as if a
parameter
> > > passed to a call that is being logged is an anchor element, we would like
to
> > log
> > > its href property. The other DOM attributes that we are interested are
"ID",
> > > "SRC", "CLASS", and etc. The logic of deciding which attributes should be
> > logged
> > > will go to ActivityLogValueConverter class.
> > > 
> > > Therefore, implementing a flag for not invoking getters in
V8ValueConverter
> is
> > > not so useful for us now as we do care only about JS Objects that are
> wrappers
> > > of DOM elements for now.
> > > I added a note in the ActivityLogValueConverter file about the flag so we
> may
> > > implement it later.
> > > 
> > > Would you be OK to have ActivityLogValueConverter?
> > 
> > The ID stuff makes sense, but I'd still rather we use V8ValueConverter for
> this
> > - strategy pattern would work well. Like
> > 
> > class Strategy {
> >  public:
> >   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
> >                              v8::Handle<v8::String> key) = 0;
> > };
> > 
> > then implement your strategy as you want (the ID stuff).
> > 
> > My concern with implementing a separate converter for the activity log is
> > divergence. It looks like your implementation only handles the top level of
an
> > object. What about if the DOM element is further down in the chain? The
> Strategy
> > approach would solve this automatically, and to solve that in a separate
class
> > would require reimplementing the traversal logic.
> 
> If I want to log just a constructor name of a JS object, we will waist CPU
> cycles by going through every property of the object with ShouldConvert()
> returning false. Another example, if you give a DOM element to
V8ValueConverter
> you will convert the *whole* DOM, through its parent and children properties,
> which we do not want to do.
> 
> It looks to me that I need to overwrite V8ValueConverterImpl::ToV8Object to
> satisfy my needs. Alternatively, I can have a callback function in
> V8ValueConverter, and if the callback is set, V8ValueConverter uses the
callback
> to convert JS objects instead of its default behavior.
> 
> What do you think about overwriting V8ValueConverterImpl::ToV8Object or
> introducing a callback in V8ValueConverter?

Sure, but the Strategy thing is basically the same, just with more of the logic
inside the v8valueconverter.

The extra CPU cycles are irrelevant, the pointer indirections slighly less
irrelevant under extremely stressful profiling, but the latter will happen
anyway if ToV8Value is overridable.

[not at google - send to devlin, 2013-07-19 22:14:28]

On 2013/07/19 22:14:04, kalman wrote:
> On 2013/07/19 22:08:18, pmarch wrote:
> > On 2013/07/19 20:54:05, kalman wrote:
> > > > 
> > > > Ben,
> > > > 
> > > > I discussed this issue with my manager and we came up to the following
> > > solution.
> > > > 
> > > > We will be logging certain properties of DOM elements such as if a
> parameter
> > > > passed to a call that is being logged is an anchor element, we would
like
> to
> > > log
> > > > its href property. The other DOM attributes that we are interested are
> "ID",
> > > > "SRC", "CLASS", and etc. The logic of deciding which attributes should
be
> > > logged
> > > > will go to ActivityLogValueConverter class.
> > > > 
> > > > Therefore, implementing a flag for not invoking getters in
> V8ValueConverter
> > is
> > > > not so useful for us now as we do care only about JS Objects that are
> > wrappers
> > > > of DOM elements for now.
> > > > I added a note in the ActivityLogValueConverter file about the flag so
we
> > may
> > > > implement it later.
> > > > 
> > > > Would you be OK to have ActivityLogValueConverter?
> > > 
> > > The ID stuff makes sense, but I'd still rather we use V8ValueConverter for
> > this
> > > - strategy pattern would work well. Like
> > > 
> > > class Strategy {
> > >  public:
> > >   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
> > >                              v8::Handle<v8::String> key) = 0;
> > > };
> > > 
> > > then implement your strategy as you want (the ID stuff).
> > > 
> > > My concern with implementing a separate converter for the activity log is
> > > divergence. It looks like your implementation only handles the top level
of
> an
> > > object. What about if the DOM element is further down in the chain? The
> > Strategy
> > > approach would solve this automatically, and to solve that in a separate
> class
> > > would require reimplementing the traversal logic.
> > 
> > If I want to log just a constructor name of a JS object, we will waist CPU
> > cycles by going through every property of the object with ShouldConvert()
> > returning false. Another example, if you give a DOM element to
> V8ValueConverter
> > you will convert the *whole* DOM, through its parent and children
properties,
> > which we do not want to do.
> > 
> > It looks to me that I need to overwrite V8ValueConverterImpl::ToV8Object to
> > satisfy my needs. Alternatively, I can have a callback function in
> > V8ValueConverter, and if the callback is set, V8ValueConverter uses the
> callback
> > to convert JS objects instead of its default behavior.
> > 
> > What do you think about overwriting V8ValueConverterImpl::ToV8Object or
> > introducing a callback in V8ValueConverter?
> 
> Sure, but the Strategy thing is basically the same, just with more of the
logic
> inside the v8valueconverter.
> 
> The extra CPU cycles are irrelevant, the pointer indirections slighly less
> irrelevant under extremely stressful profiling, but the latter will happen
> anyway if ToV8Value is overridable.

ToV8Value -> FromV8Object :)

[pmarch, 2013-07-19 22:32:36]

On 2013/07/19 22:14:28, kalman wrote:
> On 2013/07/19 22:14:04, kalman wrote:
> > On 2013/07/19 22:08:18, pmarch wrote:
> > > On 2013/07/19 20:54:05, kalman wrote:
> > > > > 
> > > > > Ben,
> > > > > 
> > > > > I discussed this issue with my manager and we came up to the following
> > > > solution.
> > > > > 
> > > > > We will be logging certain properties of DOM elements such as if a
> > parameter
> > > > > passed to a call that is being logged is an anchor element, we would
> like
> > to
> > > > log
> > > > > its href property. The other DOM attributes that we are interested are
> > "ID",
> > > > > "SRC", "CLASS", and etc. The logic of deciding which attributes should
> be
> > > > logged
> > > > > will go to ActivityLogValueConverter class.
> > > > > 
> > > > > Therefore, implementing a flag for not invoking getters in
> > V8ValueConverter
> > > is
> > > > > not so useful for us now as we do care only about JS Objects that are
> > > wrappers
> > > > > of DOM elements for now.
> > > > > I added a note in the ActivityLogValueConverter file about the flag so
> we
> > > may
> > > > > implement it later.
> > > > > 
> > > > > Would you be OK to have ActivityLogValueConverter?
> > > > 
> > > > The ID stuff makes sense, but I'd still rather we use V8ValueConverter
for
> > > this
> > > > - strategy pattern would work well. Like
> > > > 
> > > > class Strategy {
> > > >  public:
> > > >   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
> > > >                              v8::Handle<v8::String> key) = 0;
> > > > };
> > > > 
> > > > then implement your strategy as you want (the ID stuff).
> > > > 
> > > > My concern with implementing a separate converter for the activity log
is
> > > > divergence. It looks like your implementation only handles the top level
> of
> > an
> > > > object. What about if the DOM element is further down in the chain? The
> > > Strategy
> > > > approach would solve this automatically, and to solve that in a separate
> > class
> > > > would require reimplementing the traversal logic.
> > > 
> > > If I want to log just a constructor name of a JS object, we will waist CPU
> > > cycles by going through every property of the object with ShouldConvert()
> > > returning false. Another example, if you give a DOM element to
> > V8ValueConverter
> > > you will convert the *whole* DOM, through its parent and children
> properties,
> > > which we do not want to do.
> > > 
> > > It looks to me that I need to overwrite V8ValueConverterImpl::ToV8Object
to
> > > satisfy my needs. Alternatively, I can have a callback function in
> > > V8ValueConverter, and if the callback is set, V8ValueConverter uses the
> > callback
> > > to convert JS objects instead of its default behavior.
> > > 
> > > What do you think about overwriting V8ValueConverterImpl::ToV8Object or
> > > introducing a callback in V8ValueConverter?
> > 
> > Sure, but the Strategy thing is basically the same, just with more of the
> logic
> > inside the v8valueconverter.
> > 
> > The extra CPU cycles are irrelevant, the pointer indirections slighly less
> > irrelevant under extremely stressful profiling, but the latter will happen
> > anyway if ToV8Value is overridable.
> 
> ToV8Value -> FromV8Object :)

Then we can have something like this:

class Strategy {
  public:
      virtual base::Value* FromV8Object(V8ValueConverter* converter,
                                                           
v8::Handle<v8::Object> obj) = 0;
};

If Strategy::FromV8Object returns non-NULL value in the beginning of
V8ValueConverterImpl::FromV8ValueImpl, V8ValueConverter is done and it does not
perform its conversion.

[not at google - send to devlin, 2013-07-19 22:35:41]

On 2013/07/19 22:32:36, pmarch wrote:
> On 2013/07/19 22:14:28, kalman wrote:
> > On 2013/07/19 22:14:04, kalman wrote:
> > > On 2013/07/19 22:08:18, pmarch wrote:
> > > > On 2013/07/19 20:54:05, kalman wrote:
> > > > > > 
> > > > > > Ben,
> > > > > > 
> > > > > > I discussed this issue with my manager and we came up to the
following
> > > > > solution.
> > > > > > 
> > > > > > We will be logging certain properties of DOM elements such as if a
> > > parameter
> > > > > > passed to a call that is being logged is an anchor element, we would
> > like
> > > to
> > > > > log
> > > > > > its href property. The other DOM attributes that we are interested
are
> > > "ID",
> > > > > > "SRC", "CLASS", and etc. The logic of deciding which attributes
should
> > be
> > > > > logged
> > > > > > will go to ActivityLogValueConverter class.
> > > > > > 
> > > > > > Therefore, implementing a flag for not invoking getters in
> > > V8ValueConverter
> > > > is
> > > > > > not so useful for us now as we do care only about JS Objects that
are
> > > > wrappers
> > > > > > of DOM elements for now.
> > > > > > I added a note in the ActivityLogValueConverter file about the flag
so
> > we
> > > > may
> > > > > > implement it later.
> > > > > > 
> > > > > > Would you be OK to have ActivityLogValueConverter?
> > > > > 
> > > > > The ID stuff makes sense, but I'd still rather we use V8ValueConverter
> for
> > > > this
> > > > > - strategy pattern would work well. Like
> > > > > 
> > > > > class Strategy {
> > > > >  public:
> > > > >   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
> > > > >                              v8::Handle<v8::String> key) = 0;
> > > > > };
> > > > > 
> > > > > then implement your strategy as you want (the ID stuff).
> > > > > 
> > > > > My concern with implementing a separate converter for the activity log
> is
> > > > > divergence. It looks like your implementation only handles the top
level
> > of
> > > an
> > > > > object. What about if the DOM element is further down in the chain?
The
> > > > Strategy
> > > > > approach would solve this automatically, and to solve that in a
separate
> > > class
> > > > > would require reimplementing the traversal logic.
> > > > 
> > > > If I want to log just a constructor name of a JS object, we will waist
CPU
> > > > cycles by going through every property of the object with
ShouldConvert()
> > > > returning false. Another example, if you give a DOM element to
> > > V8ValueConverter
> > > > you will convert the *whole* DOM, through its parent and children
> > properties,
> > > > which we do not want to do.
> > > > 
> > > > It looks to me that I need to overwrite V8ValueConverterImpl::ToV8Object
> to
> > > > satisfy my needs. Alternatively, I can have a callback function in
> > > > V8ValueConverter, and if the callback is set, V8ValueConverter uses the
> > > callback
> > > > to convert JS objects instead of its default behavior.
> > > > 
> > > > What do you think about overwriting V8ValueConverterImpl::ToV8Object or
> > > > introducing a callback in V8ValueConverter?
> > > 
> > > Sure, but the Strategy thing is basically the same, just with more of the
> > logic
> > > inside the v8valueconverter.
> > > 
> > > The extra CPU cycles are irrelevant, the pointer indirections slighly less
> > > irrelevant under extremely stressful profiling, but the latter will happen
> > > anyway if ToV8Value is overridable.
> > 
> > ToV8Value -> FromV8Object :)
> 
> Then we can have something like this:
> 
> class Strategy {
>   public:
>       virtual base::Value* FromV8Object(V8ValueConverter* converter,
>                                                            
> v8::Handle<v8::Object> obj) = 0;
> };
> 
> If Strategy::FromV8Object returns non-NULL value in the beginning of
> V8ValueConverterImpl::FromV8ValueImpl, V8ValueConverter is done and it does
not
> perform its conversion.

how would the activity log version of it be implemented? aren't you just
filtering out properties anyway?

[pmarch, 2013-07-19 22:48:17]

On 2013/07/19 22:35:41, kalman wrote:
> On 2013/07/19 22:32:36, pmarch wrote:
> > On 2013/07/19 22:14:28, kalman wrote:
> > > On 2013/07/19 22:14:04, kalman wrote:
> > > > On 2013/07/19 22:08:18, pmarch wrote:
> > > > > On 2013/07/19 20:54:05, kalman wrote:
> > > > > > > 
> > > > > > > Ben,
> > > > > > > 
> > > > > > > I discussed this issue with my manager and we came up to the
> following
> > > > > > solution.
> > > > > > > 
> > > > > > > We will be logging certain properties of DOM elements such as if a
> > > > parameter
> > > > > > > passed to a call that is being logged is an anchor element, we
would
> > > like
> > > > to
> > > > > > log
> > > > > > > its href property. The other DOM attributes that we are interested
> are
> > > > "ID",
> > > > > > > "SRC", "CLASS", and etc. The logic of deciding which attributes
> should
> > > be
> > > > > > logged
> > > > > > > will go to ActivityLogValueConverter class.
> > > > > > > 
> > > > > > > Therefore, implementing a flag for not invoking getters in
> > > > V8ValueConverter
> > > > > is
> > > > > > > not so useful for us now as we do care only about JS Objects that
> are
> > > > > wrappers
> > > > > > > of DOM elements for now.
> > > > > > > I added a note in the ActivityLogValueConverter file about the
flag
> so
> > > we
> > > > > may
> > > > > > > implement it later.
> > > > > > > 
> > > > > > > Would you be OK to have ActivityLogValueConverter?
> > > > > > 
> > > > > > The ID stuff makes sense, but I'd still rather we use
V8ValueConverter
> > for
> > > > > this
> > > > > > - strategy pattern would work well. Like
> > > > > > 
> > > > > > class Strategy {
> > > > > >  public:
> > > > > >   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
> > > > > >                              v8::Handle<v8::String> key) = 0;
> > > > > > };
> > > > > > 
> > > > > > then implement your strategy as you want (the ID stuff).
> > > > > > 
> > > > > > My concern with implementing a separate converter for the activity
log
> > is
> > > > > > divergence. It looks like your implementation only handles the top
> level
> > > of
> > > > an
> > > > > > object. What about if the DOM element is further down in the chain?
> The
> > > > > Strategy
> > > > > > approach would solve this automatically, and to solve that in a
> separate
> > > > class
> > > > > > would require reimplementing the traversal logic.
> > > > > 
> > > > > If I want to log just a constructor name of a JS object, we will waist
> CPU
> > > > > cycles by going through every property of the object with
> ShouldConvert()
> > > > > returning false. Another example, if you give a DOM element to
> > > > V8ValueConverter
> > > > > you will convert the *whole* DOM, through its parent and children
> > > properties,
> > > > > which we do not want to do.
> > > > > 
> > > > > It looks to me that I need to overwrite
V8ValueConverterImpl::ToV8Object
> > to
> > > > > satisfy my needs. Alternatively, I can have a callback function in
> > > > > V8ValueConverter, and if the callback is set, V8ValueConverter uses
the
> > > > callback
> > > > > to convert JS objects instead of its default behavior.
> > > > > 
> > > > > What do you think about overwriting V8ValueConverterImpl::ToV8Object
or
> > > > > introducing a callback in V8ValueConverter?
> > > > 
> > > > Sure, but the Strategy thing is basically the same, just with more of
the
> > > logic
> > > > inside the v8valueconverter.
> > > > 
> > > > The extra CPU cycles are irrelevant, the pointer indirections slighly
less
> > > > irrelevant under extremely stressful profiling, but the latter will
happen
> > > > anyway if ToV8Value is overridable.
> > > 
> > > ToV8Value -> FromV8Object :)
> > 
> > Then we can have something like this:
> > 
> > class Strategy {
> >   public:
> >       virtual base::Value* FromV8Object(V8ValueConverter* converter,
> >                                                            
> > v8::Handle<v8::Object> obj) = 0;
> > };
> > 
> > If Strategy::FromV8Object returns non-NULL value in the beginning of
> > V8ValueConverterImpl::FromV8ValueImpl, V8ValueConverter is done and it does
> not
> > perform its conversion.
> 
> how would the activity log version of it be implemented? aren't you just
> filtering out properties anyway?

For now, I just read constructor name of a JS object. I do not want to do it via
object['constructor'] as V8ValueConverter would get it, but I want to read it
via V8 API. The concern here is that an extension can set a getter method and
disguise the constructor name. I can implement this logic within
Strategy::FromV8Object().

[not at google - send to devlin, 2013-07-19 23:01:27]

On 2013/07/19 22:48:17, pmarch wrote:
> On 2013/07/19 22:35:41, kalman wrote:
> > On 2013/07/19 22:32:36, pmarch wrote:
> > > On 2013/07/19 22:14:28, kalman wrote:
> > > > On 2013/07/19 22:14:04, kalman wrote:
> > > > > On 2013/07/19 22:08:18, pmarch wrote:
> > > > > > On 2013/07/19 20:54:05, kalman wrote:
> > > > > > > > 
> > > > > > > > Ben,
> > > > > > > > 
> > > > > > > > I discussed this issue with my manager and we came up to the
> > following
> > > > > > > solution.
> > > > > > > > 
> > > > > > > > We will be logging certain properties of DOM elements such as if
a
> > > > > parameter
> > > > > > > > passed to a call that is being logged is an anchor element, we
> would
> > > > like
> > > > > to
> > > > > > > log
> > > > > > > > its href property. The other DOM attributes that we are
interested
> > are
> > > > > "ID",
> > > > > > > > "SRC", "CLASS", and etc. The logic of deciding which attributes
> > should
> > > > be
> > > > > > > logged
> > > > > > > > will go to ActivityLogValueConverter class.
> > > > > > > > 
> > > > > > > > Therefore, implementing a flag for not invoking getters in
> > > > > V8ValueConverter
> > > > > > is
> > > > > > > > not so useful for us now as we do care only about JS Objects
that
> > are
> > > > > > wrappers
> > > > > > > > of DOM elements for now.
> > > > > > > > I added a note in the ActivityLogValueConverter file about the
> flag
> > so
> > > > we
> > > > > > may
> > > > > > > > implement it later.
> > > > > > > > 
> > > > > > > > Would you be OK to have ActivityLogValueConverter?
> > > > > > > 
> > > > > > > The ID stuff makes sense, but I'd still rather we use
> V8ValueConverter
> > > for
> > > > > > this
> > > > > > > - strategy pattern would work well. Like
> > > > > > > 
> > > > > > > class Strategy {
> > > > > > >  public:
> > > > > > >   virtual bool ShouldConvert(v8::Handle<v8::Object> obj,
> > > > > > >                              v8::Handle<v8::String> key) = 0;
> > > > > > > };
> > > > > > > 
> > > > > > > then implement your strategy as you want (the ID stuff).
> > > > > > > 
> > > > > > > My concern with implementing a separate converter for the activity
> log
> > > is
> > > > > > > divergence. It looks like your implementation only handles the top
> > level
> > > > of
> > > > > an
> > > > > > > object. What about if the DOM element is further down in the
chain?
> > The
> > > > > > Strategy
> > > > > > > approach would solve this automatically, and to solve that in a
> > separate
> > > > > class
> > > > > > > would require reimplementing the traversal logic.
> > > > > > 
> > > > > > If I want to log just a constructor name of a JS object, we will
waist
> > CPU
> > > > > > cycles by going through every property of the object with
> > ShouldConvert()
> > > > > > returning false. Another example, if you give a DOM element to
> > > > > V8ValueConverter
> > > > > > you will convert the *whole* DOM, through its parent and children
> > > > properties,
> > > > > > which we do not want to do.
> > > > > > 
> > > > > > It looks to me that I need to overwrite
> V8ValueConverterImpl::ToV8Object
> > > to
> > > > > > satisfy my needs. Alternatively, I can have a callback function in
> > > > > > V8ValueConverter, and if the callback is set, V8ValueConverter uses
> the
> > > > > callback
> > > > > > to convert JS objects instead of its default behavior.
> > > > > > 
> > > > > > What do you think about overwriting V8ValueConverterImpl::ToV8Object
> or
> > > > > > introducing a callback in V8ValueConverter?
> > > > > 
> > > > > Sure, but the Strategy thing is basically the same, just with more of
> the
> > > > logic
> > > > > inside the v8valueconverter.
> > > > > 
> > > > > The extra CPU cycles are irrelevant, the pointer indirections slighly
> less
> > > > > irrelevant under extremely stressful profiling, but the latter will
> happen
> > > > > anyway if ToV8Value is overridable.
> > > > 
> > > > ToV8Value -> FromV8Object :)
> > > 
> > > Then we can have something like this:
> > > 
> > > class Strategy {
> > >   public:
> > >       virtual base::Value* FromV8Object(V8ValueConverter* converter,
> > >                                                            
> > > v8::Handle<v8::Object> obj) = 0;
> > > };
> > > 
> > > If Strategy::FromV8Object returns non-NULL value in the beginning of
> > > V8ValueConverterImpl::FromV8ValueImpl, V8ValueConverter is done and it
does
> > not
> > > perform its conversion.
> > 
> > how would the activity log version of it be implemented? aren't you just
> > filtering out properties anyway?
> 
> For now, I just read constructor name of a JS object. I do not want to do it
via
> object['constructor'] as V8ValueConverter would get it, but I want to read it
> via V8 API. The concern here is that an extension can set a getter method and
> disguise the constructor name. I can implement this logic within
> Strategy::FromV8Object().

Ah I see, the serialization actually has to be different since you want to
serialize them as strings. Makes sense. In that case yeah, allowing FromV8Value
to be overridden (via that strategy-sort-of-thing) sgtm.

you may need to make it an interface more like

bool FromV8Object(v8::Handle<v8::Object> obj, base::Value* out);

so that you can return false to indicate using the default implementation.

[pmarch, 2013-07-22 21:36:36]

Ben,

Can you look at the new version of the CL.

Thank you.

[not at google - send to devlin, 2013-07-22 21:44:57]

not really what I meant, there isn't any difference between this and the code
you had before.

namely, this will still be a problem:

convert({
  nested: {
    object: {
      is: {
        dom: document.body
      }
    }
  }
})

you only need to override the object conversion logic, right? that's what I was
expecting to see overridden.

https://codereview.chromium.org/19730002/diff/56001/chrome/renderer/extension...
File chrome/renderer/extensions/activity_log_converter_strategy.h (right):

https://codereview.chromium.org/19730002/diff/56001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_converter_strategy.h:15: bool
ToV8Value(const base::Value* value, v8::Handle<v8::Value>* out) const;
no need for this?

https://codereview.chromium.org/19730002/diff/56001/content/public/renderer/v...
File content/public/renderer/v8_value_converter.h (right):

https://codereview.chromium.org/19730002/diff/56001/content/public/renderer/v...
content/public/renderer/v8_value_converter.h:25: base::Value** out) const = 0;
make this a member class of Strategy of V8ValueConverter?

[pmarch, 2013-07-22 23:06:54]

I've modified the CL to potentially handle the case of 

convert({
  nested: {
    object: {
      is: {
        dom: document.body
      }
    }
  }
})

However, at the moment we do not want to traverse properties of JSObjects and we
just report their constructer names. Later, we may change this behavior.

https://codereview.chromium.org/19730002/diff/56001/chrome/renderer/extension...
File chrome/renderer/extensions/activity_log_converter_strategy.h (right):

https://codereview.chromium.org/19730002/diff/56001/chrome/renderer/extension...
chrome/renderer/extensions/activity_log_converter_strategy.h:15: bool
ToV8Value(const base::Value* value, v8::Handle<v8::Value>* out) const;
On 2013/07/22 21:44:57, kalman wrote:
> no need for this?

Done.

https://codereview.chromium.org/19730002/diff/56001/content/public/renderer/v...
File content/public/renderer/v8_value_converter.h (right):

https://codereview.chromium.org/19730002/diff/56001/content/public/renderer/v...
content/public/renderer/v8_value_converter.h:25: base::Value** out) const = 0;
On 2013/07/22 21:44:57, kalman wrote:
> make this a member class of Strategy of V8ValueConverter?

Done.

[not at google - send to devlin, 2013-07-23 21:11:13]

I am very sorry to keep going back and forth on this.

But I'm confused again. Why Array? I thought this was for objects only? Do you
not want to log arrays?

[pmarch, 2013-07-23 21:43:39]

On 2013/07/23 21:11:13, kalman wrote:
> I am very sorry to keep going back and forth on this.
We make progress this way.

> 
> But I'm confused again. Why Array? I thought this was for objects only? Do you
> not want to log arrays?

V8ValueConverter uses Get() method when converting Arrays which may in tern
invoke a getter method if it is set on array. If we decide to convert arrays
later, we will test if a getter method installed on a property with
HasRealIndexedProperty() and if so we will drop this object otherwise retrieve
the property.

[not at google - send to devlin, 2013-07-23 21:45:21]

We might as well just not convert array indices with getters on them, save the
bother there.

[not at google - send to devlin, 2013-07-23 21:48:33]

TBH maybe we shouldn't _ever_ be invoking getters for properties (arrays or
objects) except when explicit. This would avoid surprises.

[felt, 2013-07-23 21:54:48]

On 2013/07/23 21:43:39, pmarch wrote:
> On 2013/07/23 21:11:13, kalman wrote:
> > I am very sorry to keep going back and forth on this.
> We make progress this way.
> 
> > 
> > But I'm confused again. Why Array? I thought this was for objects only? Do
you
> > not want to log arrays?
> 
> V8ValueConverter uses Get() method when converting Arrays which may in tern
> invoke a getter method if it is set on array. If we decide to convert arrays
> later, we will test if a getter method installed on a property with
> HasRealIndexedProperty() and if so we will drop this object otherwise retrieve
> the property.

Wait, we should be serializing arrays.

[pmarch, 2013-07-23 21:55:42]

On 2013/07/23 21:48:33, kalman wrote:
> TBH maybe we shouldn't _ever_ be invoking getters for properties (arrays or
> objects) except when explicit. This would avoid surprises.

I need to have better understanding of how the V8ValueConverter is used. For our
logging purposes, we definitely do not want to invoke getters. In other
scenarios, when an extension supplies an object to an extension API call, why
not to allow getters. In fact, some of the extensions may use getters already,
and if we change this behavior, we may break something.

[not at google - send to devlin, 2013-07-23 22:05:04]

On 2013/07/23 21:55:42, pmarch wrote:
> On 2013/07/23 21:48:33, kalman wrote:
> > TBH maybe we shouldn't _ever_ be invoking getters for properties (arrays or
> > objects) except when explicit. This would avoid surprises.
> 
> I need to have better understanding of how the V8ValueConverter is used. For
our
> logging purposes, we definitely do not want to invoke getters. In other
> scenarios, when an extension supplies an object to an extension API call, why
> not to allow getters. In fact, some of the extensions may use getters already,
> and if we change this behavior, we may break something.

Yes, for extension API calls we certainly do want to invoke getters (in fact we
used to not do it, and it led to bugs) - however it's safer if we force it to be
explicit so that callers know to expect it.

Either that, or we'd break things in funny ways.

But I still think it's best to be explicit. Even if we go through and add
SetInvokeGetters(true) all callers that use FromV8Value, it would be better. I
may go through and make the whole thing less crazy by forcing these options to
be constructor arguments, JSONReader style.

[pmarch, 2013-07-23 22:23:38]

On 2013/07/23 22:05:04, kalman wrote:
> On 2013/07/23 21:55:42, pmarch wrote:
> > On 2013/07/23 21:48:33, kalman wrote:
> > > TBH maybe we shouldn't _ever_ be invoking getters for properties (arrays
or
> > > objects) except when explicit. This would avoid surprises.
> > 
> > I need to have better understanding of how the V8ValueConverter is used. For
> our
> > logging purposes, we definitely do not want to invoke getters. In other
> > scenarios, when an extension supplies an object to an extension API call,
why
> > not to allow getters. In fact, some of the extensions may use getters
already,
> > and if we change this behavior, we may break something.
> 
> Yes, for extension API calls we certainly do want to invoke getters (in fact
we
> used to not do it, and it led to bugs) - however it's safer if we force it to
be
> explicit so that callers know to expect it.
> 
> Either that, or we'd break things in funny ways.
> 
> But I still think it's best to be explicit. Even if we go through and add
> SetInvokeGetters(true) all callers that use FromV8Value, it would be better. I
> may go through and make the whole thing less crazy by forcing these options to
> be constructor arguments, JSONReader style.

I agree being explicit with SetInvokeGetters(true) is better. This will also
allow us to reuse the stock FromV8Array method (with SetInvokeGetters(false)) of
V8ValueConverter.

Another consideration is interceptors. They should be safe as they are installed
by trusted code (browser) for DOM elements. However, to my knowledge, NaCl
modules use JS to communicate with JS on a page, thus we need to make sure that
a JS object with an interceptor set by a NaCl module (if it is possible at all)
does not reach V8ValueConverter, or if this object reaches V8ValueConverter,
whether it is safe to invoke such interceptor.

[not at google - send to devlin, 2013-07-23 22:27:45]

> 
> I agree being explicit with SetInvokeGetters(true) is better. This will also
> allow us to reuse the stock FromV8Array method (with SetInvokeGetters(false))
of
> V8ValueConverter.

Sweet.

> 
> Another consideration is interceptors. They should be safe as they are
installed
> by trusted code (browser) for DOM elements. However, to my knowledge, NaCl
> modules use JS to communicate with JS on a page, thus we need to make sure
that
> a JS object with an interceptor set by a NaCl module (if it is possible at
all)
> does not reach V8ValueConverter, or if this object reaches V8ValueConverter,
> whether it is safe to invoke such interceptor.

I'm not familiar with interceptors nor our use of them, but I do wonder whether
V8ValueConverter needs to be involved in any way.

[pmarch, 2013-07-24 01:16:11]

On 2013/07/23 22:27:45, kalman wrote:
> > 
> > I agree being explicit with SetInvokeGetters(true) is better. This will also
> > allow us to reuse the stock FromV8Array method (with
SetInvokeGetters(false))
> of
> > V8ValueConverter.
> 
> Sweet.
> 

Shall I implement this flag in the V8ValueConverter?

> > 
> > Another consideration is interceptors. They should be safe as they are
> installed
> > by trusted code (browser) for DOM elements. However, to my knowledge, NaCl
> > modules use JS to communicate with JS on a page, thus we need to make sure
> that
> > a JS object with an interceptor set by a NaCl module (if it is possible at
> all)
> > does not reach V8ValueConverter, or if this object reaches V8ValueConverter,
> > whether it is safe to invoke such interceptor.
> 
> I'm not familiar with interceptors nor our use of them, but I do wonder
whether
> V8ValueConverter needs to be involved in any way.

[not at google - send to devlin, 2013-07-24 01:24:55]

sounds like it'll help you now - so that'd be great, thanks.

[felt, 2013-07-29 17:13:17]

Hi Petr, I'm back from vacation/conference now. What's the current status of
this CL?

[pmarch, 2013-07-29 17:39:17]

On 2013/07/29 17:13:17, felt wrote:
> Hi Petr, I'm back from vacation/conference now. What's the current status of
> this CL?

I switched to work on something else and will get back to this CL in the end of
this week. Ben and I agreed on implementing an invoke getters flag in
V8ValueConverter.

[felt, 2013-08-02 13:58:14]

On 2013/07/29 17:39:17, pmarch wrote:
> On 2013/07/29 17:13:17, felt wrote:
> > Hi Petr, I'm back from vacation/conference now. What's the current status of
> > this CL?
> 
> I switched to work on something else and will get back to this CL in the end
of
> this week. Ben and I agreed on implementing an invoke getters flag in
> V8ValueConverter.

Thanks Petr. FYI, this is now blocking Michael's database size compression work.

[felt, 2013-08-07 00:01:25]

On 2013/08/02 13:58:14, felt wrote:
> On 2013/07/29 17:39:17, pmarch wrote:
> > On 2013/07/29 17:13:17, felt wrote:
> > > Hi Petr, I'm back from vacation/conference now. What's the current status
of
> > > this CL?
> > 
> > I switched to work on something else and will get back to this CL in the end
> of
> > this week. Ben and I agreed on implementing an invoke getters flag in
> > V8ValueConverter.
> 
> Thanks Petr. FYI, this is now blocking Michael's database size compression
work.

This is now blocking both me and Michael. It would also be nice to land this in
time for M30 (this Friday). Do you think you could take a break from the
pipeline stuff to finish this soon? We'd both appreciate it.

[pmarch, 2013-08-07 15:53:03]

On 2013/08/07 00:01:25, felt wrote:
> On 2013/08/02 13:58:14, felt wrote:
> > On 2013/07/29 17:39:17, pmarch wrote:
> > > On 2013/07/29 17:13:17, felt wrote:
> > > > Hi Petr, I'm back from vacation/conference now. What's the current
status
> of
> > > > this CL?
> > > 
> > > I switched to work on something else and will get back to this CL in the
end
> > of
> > > this week. Ben and I agreed on implementing an invoke getters flag in
> > > V8ValueConverter.
> > 
> > Thanks Petr. FYI, this is now blocking Michael's database size compression
> work.
> 
> This is now blocking both me and Michael. It would also be nice to land this
in
> time for M30 (this Friday). Do you think you could take a break from the
> pipeline stuff to finish this soon? We'd both appreciate it.

I am working on this since yesterday and I think we should go with the current
patch. And this is why.

Recall, we want to convert V8 values to base::Values and make sure that none of
JS code invoked due to conversion.
The JS code may be invoked by C++ code in the following ways:
1) JS installs getter method on a property.
2) An C++ accessor is set on a property via V8 API.
3) A handler (formally known as interceptors) is set on the V8 object via V8
API.
4) A PropertyAccessCheckCallback is set on the object vi V8 API.

Before calling v8_obj->Get() we need to make sure that all above are false. So
there are following problems:
- Using V8 API, there is no way to check if a JS getter method installed on
index-keyed property (in V8 there two types of properties, accessed via index
and via string)
- Using V8 API, there is no way to checck if a PropertyAccessCheckCallback is
set on a V8 object.

Because of these problems, it is not possible to implement complete solution
that will satisfy our needs. So I am suggesting to commit the CL in its current
state and then work with V8 team to implement required calls.

Ben, would you agree?

[not at google - send to devlin, 2013-08-07 16:42:48]

On 2013/08/07 15:53:03, pmarch wrote:
> On 2013/08/07 00:01:25, felt wrote:
> > On 2013/08/02 13:58:14, felt wrote:
> > > On 2013/07/29 17:39:17, pmarch wrote:
> > > > On 2013/07/29 17:13:17, felt wrote:
> > > > > Hi Petr, I'm back from vacation/conference now. What's the current
> status
> > of
> > > > > this CL?
> > > > 
> > > > I switched to work on something else and will get back to this CL in the
> end
> > > of
> > > > this week. Ben and I agreed on implementing an invoke getters flag in
> > > > V8ValueConverter.
> > > 
> > > Thanks Petr. FYI, this is now blocking Michael's database size compression
> > work.
> > 
> > This is now blocking both me and Michael. It would also be nice to land this
> in
> > time for M30 (this Friday). Do you think you could take a break from the
> > pipeline stuff to finish this soon? We'd both appreciate it.
> 
> I am working on this since yesterday and I think we should go with the current
> patch. And this is why.
> 
> Recall, we want to convert V8 values to base::Values and make sure that none
of
> JS code invoked due to conversion.
> The JS code may be invoked by C++ code in the following ways:
> 1) JS installs getter method on a property.
> 2) An C++ accessor is set on a property via V8 API.
> 3) A handler (formally known as interceptors) is set on the V8 object via V8
> API.
> 4) A PropertyAccessCheckCallback is set on the object vi V8 API.
> 
> Before calling v8_obj->Get() we need to make sure that all above are false. So
> there are following problems:
> - Using V8 API, there is no way to check if a JS getter method installed on
> index-keyed property (in V8 there two types of properties, accessed via index
> and via string)
> - Using V8 API, there is no way to checck if a PropertyAccessCheckCallback is
> set on a V8 object.
> 
> Because of these problems, it is not possible to implement complete solution
> that will satisfy our needs. So I am suggesting to commit the CL in its
current
> state and then work with V8 team to implement required calls.
> 
> Ben, would you agree?

The only problem here is the indexed callback, but it's possible that the v8
APIs automatically convert those to named callbacks. I'm not sure. It's
certainly a hole in the v8 APIs if it isn't.

What happened to converting arrays? Unless I misunderstood felt's comment, you
need to convert arrays of objects the same way. This was my understanding from
the very start.

[pmarch, 2013-08-07 16:55:21]

On 2013/08/07 16:42:48, kalman wrote:
> On 2013/08/07 15:53:03, pmarch wrote:
> > On 2013/08/07 00:01:25, felt wrote:
> > > On 2013/08/02 13:58:14, felt wrote:
> > > > On 2013/07/29 17:39:17, pmarch wrote:
> > > > > On 2013/07/29 17:13:17, felt wrote:
> > > > > > Hi Petr, I'm back from vacation/conference now. What's the current
> > status
> > > of
> > > > > > this CL?
> > > > > 
> > > > > I switched to work on something else and will get back to this CL in
the
> > end
> > > > of
> > > > > this week. Ben and I agreed on implementing an invoke getters flag in
> > > > > V8ValueConverter.
> > > > 
> > > > Thanks Petr. FYI, this is now blocking Michael's database size
compression
> > > work.
> > > 
> > > This is now blocking both me and Michael. It would also be nice to land
this
> > in
> > > time for M30 (this Friday). Do you think you could take a break from the
> > > pipeline stuff to finish this soon? We'd both appreciate it.
> > 
> > I am working on this since yesterday and I think we should go with the
current
> > patch. And this is why.
> > 
> > Recall, we want to convert V8 values to base::Values and make sure that none
> of
> > JS code invoked due to conversion.
> > The JS code may be invoked by C++ code in the following ways:
> > 1) JS installs getter method on a property.
> > 2) An C++ accessor is set on a property via V8 API.
> > 3) A handler (formally known as interceptors) is set on the V8 object via V8
> > API.
> > 4) A PropertyAccessCheckCallback is set on the object vi V8 API.
> > 
> > Before calling v8_obj->Get() we need to make sure that all above are false.
So
> > there are following problems:
> > - Using V8 API, there is no way to check if a JS getter method installed on
> > index-keyed property (in V8 there two types of properties, accessed via
index
> > and via string)
> > - Using V8 API, there is no way to checck if a PropertyAccessCheckCallback
is
> > set on a V8 object.
> > 
> > Because of these problems, it is not possible to implement complete solution
> > that will satisfy our needs. So I am suggesting to commit the CL in its
> current
> > state and then work with V8 team to implement required calls.
> > 
> > Ben, would you agree?
> 
> The only problem here is the indexed callback, but it's possible that the v8
> APIs automatically convert those to named callbacks. I'm not sure. It's
> certainly a hole in the v8 APIs if it isn't.
There is a call to check callbacks on named properties
(HasRealNamedCallbackProperty), it does not work for indexed properties, I've
checked this with experiments.

I think PropertyAccessCheckCallbacks are also a problem as they also may invoke
JS. 

> 
> What happened to converting arrays? Unless I misunderstood felt's comment, you
> need to convert arrays of objects the same way. This was my understanding from
> the very start.

Without checking for getters on indexed properties, it is not possible to
guarantee that none of JS is executed when accessing an array.

[not at google - send to devlin, 2013-08-07 17:00:41]

On 2013/08/07 16:55:21, pmarch wrote:
> On 2013/08/07 16:42:48, kalman wrote:
> > On 2013/08/07 15:53:03, pmarch wrote:
> > > On 2013/08/07 00:01:25, felt wrote:
> > > > On 2013/08/02 13:58:14, felt wrote:
> > > > > On 2013/07/29 17:39:17, pmarch wrote:
> > > > > > On 2013/07/29 17:13:17, felt wrote:
> > > > > > > Hi Petr, I'm back from vacation/conference now. What's the current
> > > status
> > > > of
> > > > > > > this CL?
> > > > > > 
> > > > > > I switched to work on something else and will get back to this CL in
> the
> > > end
> > > > > of
> > > > > > this week. Ben and I agreed on implementing an invoke getters flag
in
> > > > > > V8ValueConverter.
> > > > > 
> > > > > Thanks Petr. FYI, this is now blocking Michael's database size
> compression
> > > > work.
> > > > 
> > > > This is now blocking both me and Michael. It would also be nice to land
> this
> > > in
> > > > time for M30 (this Friday). Do you think you could take a break from the
> > > > pipeline stuff to finish this soon? We'd both appreciate it.
> > > 
> > > I am working on this since yesterday and I think we should go with the
> current
> > > patch. And this is why.
> > > 
> > > Recall, we want to convert V8 values to base::Values and make sure that
none
> > of
> > > JS code invoked due to conversion.
> > > The JS code may be invoked by C++ code in the following ways:
> > > 1) JS installs getter method on a property.
> > > 2) An C++ accessor is set on a property via V8 API.
> > > 3) A handler (formally known as interceptors) is set on the V8 object via
V8
> > > API.
> > > 4) A PropertyAccessCheckCallback is set on the object vi V8 API.
> > > 
> > > Before calling v8_obj->Get() we need to make sure that all above are
false.
> So
> > > there are following problems:
> > > - Using V8 API, there is no way to check if a JS getter method installed
on
> > > index-keyed property (in V8 there two types of properties, accessed via
> index
> > > and via string)
> > > - Using V8 API, there is no way to checck if a PropertyAccessCheckCallback
> is
> > > set on a V8 object.
> > > 
> > > Because of these problems, it is not possible to implement complete
solution
> > > that will satisfy our needs. So I am suggesting to commit the CL in its
> > current
> > > state and then work with V8 team to implement required calls.
> > > 
> > > Ben, would you agree?
> > 
> > The only problem here is the indexed callback, but it's possible that the v8
> > APIs automatically convert those to named callbacks. I'm not sure. It's
> > certainly a hole in the v8 APIs if it isn't.
> There is a call to check callbacks on named properties
> (HasRealNamedCallbackProperty), it does not work for indexed properties, I've
> checked this with experiments.
> 
> I think PropertyAccessCheckCallbacks are also a problem as they also may
invoke
> JS. 
> 
> > 
> > What happened to converting arrays? Unless I misunderstood felt's comment,
you
> > need to convert arrays of objects the same way. This was my understanding
from
> > the very start.
> 
> Without checking for getters on indexed properties, it is not possible to
> guarantee that none of JS is executed when accessing an array.

My point is that if it's a requirement that arrays are logged, you need to log
them, regardless of implementation details. We can enlist help to fix the
indexed property problem from the v8 team.

[felt, 2013-08-07 17:07:38]

On 2013/08/07 17:00:41, kalman wrote:
> On 2013/08/07 16:55:21, pmarch wrote:
> > On 2013/08/07 16:42:48, kalman wrote:
> > > On 2013/08/07 15:53:03, pmarch wrote:
> > > > On 2013/08/07 00:01:25, felt wrote:
> > > > > On 2013/08/02 13:58:14, felt wrote:
> > > > > > On 2013/07/29 17:39:17, pmarch wrote:
> > > > > > > On 2013/07/29 17:13:17, felt wrote:
> > > > > > > > Hi Petr, I'm back from vacation/conference now. What's the
current
> > > > status
> > > > > of
> > > > > > > > this CL?
> > > > > > > 
> > > > > > > I switched to work on something else and will get back to this CL
in
> > the
> > > > end
> > > > > > of
> > > > > > > this week. Ben and I agreed on implementing an invoke getters flag
> in
> > > > > > > V8ValueConverter.
> > > > > > 
> > > > > > Thanks Petr. FYI, this is now blocking Michael's database size
> > compression
> > > > > work.
> > > > > 
> > > > > This is now blocking both me and Michael. It would also be nice to
land
> > this
> > > > in
> > > > > time for M30 (this Friday). Do you think you could take a break from
the
> > > > > pipeline stuff to finish this soon? We'd both appreciate it.
> > > > 
> > > > I am working on this since yesterday and I think we should go with the
> > current
> > > > patch. And this is why.
> > > > 
> > > > Recall, we want to convert V8 values to base::Values and make sure that
> none
> > > of
> > > > JS code invoked due to conversion.
> > > > The JS code may be invoked by C++ code in the following ways:
> > > > 1) JS installs getter method on a property.
> > > > 2) An C++ accessor is set on a property via V8 API.
> > > > 3) A handler (formally known as interceptors) is set on the V8 object
via
> V8
> > > > API.
> > > > 4) A PropertyAccessCheckCallback is set on the object vi V8 API.
> > > > 
> > > > Before calling v8_obj->Get() we need to make sure that all above are
> false.
> > So
> > > > there are following problems:
> > > > - Using V8 API, there is no way to check if a JS getter method installed
> on
> > > > index-keyed property (in V8 there two types of properties, accessed via
> > index
> > > > and via string)
> > > > - Using V8 API, there is no way to checck if a
PropertyAccessCheckCallback
> > is
> > > > set on a V8 object.
> > > > 
> > > > Because of these problems, it is not possible to implement complete
> solution
> > > > that will satisfy our needs. So I am suggesting to commit the CL in its
> > > current
> > > > state and then work with V8 team to implement required calls.
> > > > 
> > > > Ben, would you agree?
> > > 
> > > The only problem here is the indexed callback, but it's possible that the
v8
> > > APIs automatically convert those to named callbacks. I'm not sure. It's
> > > certainly a hole in the v8 APIs if it isn't.
> > There is a call to check callbacks on named properties
> > (HasRealNamedCallbackProperty), it does not work for indexed properties,
I've
> > checked this with experiments.
> > 
> > I think PropertyAccessCheckCallbacks are also a problem as they also may
> invoke
> > JS. 
> > 
> > > 
> > > What happened to converting arrays? Unless I misunderstood felt's comment,
> you
> > > need to convert arrays of objects the same way. This was my understanding
> from
> > > the very start.
> > 
> > Without checking for getters on indexed properties, it is not possible to
> > guarantee that none of JS is executed when accessing an array.
> 
> My point is that if it's a requirement that arrays are logged, you need to log
> them, regardless of implementation details. We can enlist help to fix the
> indexed property problem from the v8 team.

After talking off-thread with Petr: my preference is to see at least some fix go
in for objects right now, then we can tackle arrays at a later date.

[not at google - send to devlin, 2013-08-07 17:14:40]

> After talking off-thread with Petr: my preference is to see at least some fix
go
> in for objects right now, then we can tackle arrays at a later date.

Do you mean submit a version that has the array vulnerability and fix that
later, or just serialize arrays for now and go back to not-serializing them
later?

My half-solution preference is the former since it involves backing out less
code later on.

[pmarch, 2013-08-07 17:18:49]

On 2013/08/07 17:14:40, kalman wrote:
> > After talking off-thread with Petr: my preference is to see at least some
fix
> go
> > in for objects right now, then we can tackle arrays at a later date.
> 
> Do you mean submit a version that has the array vulnerability and fix that
> later, or just serialize arrays for now and go back to not-serializing them
> later?

There is miss-understanding here we have the following two solutions:
1) Do not log arrays, no vulnerabilities (getter invocations)
2) Log array as in vanilla V8 value converter with having the vulnerability
(getter invocations)

This CL currently implements 1).

> 
> My half-solution preference is the former since it involves backing out less
> code later on.

[not at google - send to devlin, 2013-08-07 17:21:01]

On 2013/08/07 17:18:49, pmarch wrote:
> On 2013/08/07 17:14:40, kalman wrote:
> > > After talking off-thread with Petr: my preference is to see at least some
> fix
> > go
> > > in for objects right now, then we can tackle arrays at a later date.
> > 
> > Do you mean submit a version that has the array vulnerability and fix that
> > later, or just serialize arrays for now and go back to not-serializing them
> > later?
> 
> There is miss-understanding here we have the following two solutions:
> 1) Do not log arrays, no vulnerabilities (getter invocations)
> 2) Log array as in vanilla V8 value converter with having the vulnerability
> (getter invocations)
> 
> This CL currently implements 1).
> 
> > 
> > My half-solution preference is the former since it involves backing out less
> > code later on.

That's my understanding, I don't think there's a misunderstanding. I would
prefer we do (2) because it's less code and will look more like the eventual
solution here. With (2) all that's needed is a v8 fix + some simple coding here.
With (1) you need a v8 fix and some more severe coding.

[pmarch, 2013-08-07 17:26:03]

On 2013/08/07 17:21:01, kalman wrote:
> On 2013/08/07 17:18:49, pmarch wrote:
> > On 2013/08/07 17:14:40, kalman wrote:
> > > > After talking off-thread with Petr: my preference is to see at least
some
> > fix
> > > go
> > > > in for objects right now, then we can tackle arrays at a later date.
> > > 
> > > Do you mean submit a version that has the array vulnerability and fix that
> > > later, or just serialize arrays for now and go back to not-serializing
them
> > > later?
> > 
> > There is miss-understanding here we have the following two solutions:
> > 1) Do not log arrays, no vulnerabilities (getter invocations)
> > 2) Log array as in vanilla V8 value converter with having the vulnerability
> > (getter invocations)
> > 
> > This CL currently implements 1).
> > 
> > > 
> > > My half-solution preference is the former since it involves backing out
less
> > > code later on.
> 
> That's my understanding, I don't think there's a misunderstanding. I would
> prefer we do (2) because it's less code and will look more like the eventual
> solution here. With (2) all that's needed is a v8 fix + some simple coding
here.
> With (1) you need a v8 fix and some more severe coding.

As for me, (1) and (2) are almost the same in terms of coding effort. If we
agree on (1) this CL will go in, otherwise I will kill FromV8Array() in
ActivityLogConverterStrategy and thus enable V8ValueConverter to convert arrays
(but not other JSObjects).

[not at google - send to devlin, 2013-08-07 17:30:24]

On 2013/08/07 17:26:03, pmarch wrote:
> On 2013/08/07 17:21:01, kalman wrote:
> > On 2013/08/07 17:18:49, pmarch wrote:
> > > On 2013/08/07 17:14:40, kalman wrote:
> > > > > After talking off-thread with Petr: my preference is to see at least
> some
> > > fix
> > > > go
> > > > > in for objects right now, then we can tackle arrays at a later date.
> > > > 
> > > > Do you mean submit a version that has the array vulnerability and fix
that
> > > > later, or just serialize arrays for now and go back to not-serializing
> them
> > > > later?
> > > 
> > > There is miss-understanding here we have the following two solutions:
> > > 1) Do not log arrays, no vulnerabilities (getter invocations)
> > > 2) Log array as in vanilla V8 value converter with having the
vulnerability
> > > (getter invocations)
> > > 
> > > This CL currently implements 1).
> > > 
> > > > 
> > > > My half-solution preference is the former since it involves backing out
> less
> > > > code later on.
> > 
> > That's my understanding, I don't think there's a misunderstanding. I would
> > prefer we do (2) because it's less code and will look more like the eventual
> > solution here. With (2) all that's needed is a v8 fix + some simple coding
> here.
> > With (1) you need a v8 fix and some more severe coding.
> 
> As for me, (1) and (2) are almost the same in terms of coding effort. If we
> agree on (1) this CL will go in, otherwise I will kill FromV8Array() in
> ActivityLogConverterStrategy and thus enable V8ValueConverter to convert
arrays
> (but not other JSObjects).

(2) doesn't involve changing the interface which is why I think it's less
effort. The changes would be confined to the value converter.

But I think this change has been rather over-analysed. As it stands I prefer the
version before I even made any comments because what I expected would generalise
this has actually made it more complicated without much gain. (2) would be
worthwhile for me, though, along with an option to the converter to only look at
named properties. That's half the reason for this patch, right?

[pmarch, 2013-08-07 17:38:21]

On 2013/08/07 17:30:24, kalman wrote:
> On 2013/08/07 17:26:03, pmarch wrote:
> > On 2013/08/07 17:21:01, kalman wrote:
> > > On 2013/08/07 17:18:49, pmarch wrote:
> > > > On 2013/08/07 17:14:40, kalman wrote:
> > > > > > After talking off-thread with Petr: my preference is to see at least
> > some
> > > > fix
> > > > > go
> > > > > > in for objects right now, then we can tackle arrays at a later date.
> > > > > 
> > > > > Do you mean submit a version that has the array vulnerability and fix
> that
> > > > > later, or just serialize arrays for now and go back to not-serializing
> > them
> > > > > later?
> > > > 
> > > > There is miss-understanding here we have the following two solutions:
> > > > 1) Do not log arrays, no vulnerabilities (getter invocations)
> > > > 2) Log array as in vanilla V8 value converter with having the
> vulnerability
> > > > (getter invocations)
> > > > 
> > > > This CL currently implements 1).
> > > > 
> > > > > 
> > > > > My half-solution preference is the former since it involves backing
out
> > less
> > > > > code later on.
> > > 
> > > That's my understanding, I don't think there's a misunderstanding. I would
> > > prefer we do (2) because it's less code and will look more like the
eventual
> > > solution here. With (2) all that's needed is a v8 fix + some simple coding
> > here.
> > > With (1) you need a v8 fix and some more severe coding.
> > 
> > As for me, (1) and (2) are almost the same in terms of coding effort. If we
> > agree on (1) this CL will go in, otherwise I will kill FromV8Array() in
> > ActivityLogConverterStrategy and thus enable V8ValueConverter to convert
> arrays
> > (but not other JSObjects).
> 
> (2) doesn't involve changing the interface which is why I think it's less
> effort. The changes would be confined to the value converter.
> 
regarding (2), we still want to change interface for JSObject conversion, we do
not want invoke any getters on them.

> But I think this change has been rather over-analysed. As it stands I prefer
the
> version before I even made any comments because what I expected would
generalise
> this has actually made it more complicated without much gain. (2) would be
> worthwhile for me, though, along with an option to the converter to only look
at
> named properties. That's half the reason for this patch, right?

At this point our main priority is not invoke any JS when using
V8ValueConverter. This CL targets this problem. We will later extend our
strategy object to safely look into properties but only those that we are
interested in without invoking JS along the way.

[not at google - send to devlin, 2013-08-07 17:44:28]

> > 
> regarding (2), we still want to change interface for JSObject conversion, we
do
> not want invoke any getters on them.

I don't follow, why does this involve changing the interface for
V8ValueConverter later?

> 
> > But I think this change has been rather over-analysed. As it stands I prefer
> the
> > version before I even made any comments because what I expected would
> generalise
> > this has actually made it more complicated without much gain. (2) would be
> > worthwhile for me, though, along with an option to the converter to only
look
> at
> > named properties. That's half the reason for this patch, right?
> 
> At this point our main priority is not invoke any JS when using
> V8ValueConverter. This CL targets this problem. We will later extend our
> strategy object to safely look into properties but only those that we are
> interested in without invoking JS along the way.

Let me check with Adam Klein who sits in this area when I'm out of this meeting.
I can't really review code anyway.

[pmarch, 2013-08-07 17:52:51]

On 2013/08/07 17:44:28, kalman wrote:
> > > 
> > regarding (2), we still want to change interface for JSObject conversion, we
> do
> > not want invoke any getters on them.
> 
> I don't follow, why does this involve changing the interface for
> V8ValueConverter later?

Solution (2) changes interface for V8ValueConverter with strategy object which
essentially overwrites the behavior of FromV8Object().


> > 
> > > But I think this change has been rather over-analysed. As it stands I
prefer
> > the
> > > version before I even made any comments because what I expected would
> > generalise
> > > this has actually made it more complicated without much gain. (2) would be
> > > worthwhile for me, though, along with an option to the converter to only
> look
> > at
> > > named properties. That's half the reason for this patch, right?
> > 
> > At this point our main priority is not invoke any JS when using
> > V8ValueConverter. This CL targets this problem. We will later extend our
> > strategy object to safely look into properties but only those that we are
> > interested in without invoking JS along the way.
> 
> Let me check with Adam Klein who sits in this area when I'm out of this
meeting.
> I can't really review code anyway.

Let me know if you want to include me into this discussion or chat latter at any
point.

[not at google - send to devlin, 2013-08-07 18:43:17]

On 2013/08/07 17:52:51, pmarch wrote:
> On 2013/08/07 17:44:28, kalman wrote:
> > > > 
> > > regarding (2), we still want to change interface for JSObject conversion,
we
> > do
> > > not want invoke any getters on them.
> > 
> > I don't follow, why does this involve changing the interface for
> > V8ValueConverter later?
> 
> Solution (2) changes interface for V8ValueConverter with strategy object which
> essentially overwrites the behavior of FromV8Object().
> 
> 
> > > 
> > > > But I think this change has been rather over-analysed. As it stands I
> prefer
> > > the
> > > > version before I even made any comments because what I expected would
> > > generalise
> > > > this has actually made it more complicated without much gain. (2) would
be
> > > > worthwhile for me, though, along with an option to the converter to only
> > look
> > > at
> > > > named properties. That's half the reason for this patch, right?
> > > 
> > > At this point our main priority is not invoke any JS when using
> > > V8ValueConverter. This CL targets this problem. We will later extend our
> > > strategy object to safely look into properties but only those that we are
> > > interested in without invoking JS along the way.
> > 
> > Let me check with Adam Klein who sits in this area when I'm out of this
> meeting.
> > I can't really review code anyway.
> 
> Let me know if you want to include me into this discussion or chat latter at
any
> point.

After chatting to Adam yes we'd need to add another method for the indexed
property thing.

However, we can also do it by calling into JS and using
GetOwnPropertyDescriptor. That would work for arrays as well - and we can have a
safe version of that function by adding it here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/ex...

[not at google - send to devlin, 2013-08-07 18:45:08]

On 2013/08/07 17:52:51, pmarch wrote:
> On 2013/08/07 17:44:28, kalman wrote:
> > > > 
> > > regarding (2), we still want to change interface for JSObject conversion,
we
> > do
> > > not want invoke any getters on them.
> > 
> > I don't follow, why does this involve changing the interface for
> > V8ValueConverter later?
> 
> Solution (2) changes interface for V8ValueConverter with strategy object which
> essentially overwrites the behavior of FromV8Object().
> 

I can't follow this anymore, just getting more confused.

[felt, 2013-08-07 19:33:39]

On 2013/08/07 18:45:08, kalman wrote:
> On 2013/08/07 17:52:51, pmarch wrote:
> > On 2013/08/07 17:44:28, kalman wrote:
> > > > > 
> > > > regarding (2), we still want to change interface for JSObject
conversion,
> we
> > > do
> > > > not want invoke any getters on them.
> > > 
> > > I don't follow, why does this involve changing the interface for
> > > V8ValueConverter later?
> > 
> > Solution (2) changes interface for V8ValueConverter with strategy object
which
> > essentially overwrites the behavior of FromV8Object().
> > 
> 
> I can't follow this anymore, just getting more confused.

Could you guys schedule a 10min GVC? Maybe talking would help. I would offer an
opinion but I think I know less about the exact implementation details than
either of you, and we really do need some solution for objects.

[pmarch, 2013-08-07 21:50:40]

On 2013/08/07 19:33:39, felt wrote:
> On 2013/08/07 18:45:08, kalman wrote:
> > On 2013/08/07 17:52:51, pmarch wrote:
> > > On 2013/08/07 17:44:28, kalman wrote:
> > > > > > 
> > > > > regarding (2), we still want to change interface for JSObject
> conversion,
> > we
> > > > do
> > > > > not want invoke any getters on them.
> > > > 
> > > > I don't follow, why does this involve changing the interface for
> > > > V8ValueConverter later?
> > > 
> > > Solution (2) changes interface for V8ValueConverter with strategy object
> which
> > > essentially overwrites the behavior of FromV8Object().
> > > 
> > 
> > I can't follow this anymore, just getting more confused.
> 
> Could you guys schedule a 10min GVC? Maybe talking would help. I would offer
an
> opinion but I think I know less about the exact implementation details than
> either of you, and we really do need some solution for objects.

Rebased and updated a comment. Please, have a final look.

[not at google - send to devlin, 2013-08-07 22:07:55]

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.cc (right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:30: 
add a comment about the array index stuff we've been talking about.

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:46: *out = new
base::StringValue(std::string(*utf8, utf8.length()));
you should be able to do just

name = v8::String::Concat(...);
*out = new base::StringValue(*v8::String::AsciiValue(name))

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:47: return true;
// Prevent V8ValueConverter from further processing this object.
comment would look better on its own line

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
(right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:164: &&
out_string == std::string("[Function foo()]"));
These can all be done more concisely if you pull them into functions that return
testing::AssertionResults. 

Also most of them sholud be EXPECT_TRUE not ASSERT_TRUE.

Something like

EXPECT_TRUE(IsString("[Function]", v8_object, "function"));

where

v8::AssertionResult IsString(const std::string& string, v8::Local<v8::Object>
object, const std::string& key) {
  if (...)
    return testing::AssertionFailure() << "failed cos..";
  rturn testing::AssertionSucces();
}

it's a nice pattern.

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/api_activity_logger.cc:62: // will call JS toString()
method which can be overwritten.
RegExp seems a bit like an implementation detail. We could equally change
V8ValueConverter not to call ToString() on the regexp.

In fact you could change it to use GetSource().

All that aside - you're not disabling RegExp here anyway.

https://codereview.chromium.org/19730002/diff/102002/content/renderer/v8_valu...
File content/renderer/v8_value_converter_impl.cc (right):

https://codereview.chromium.org/19730002/diff/102002/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:306: }
I think the context scope stuff below might actually be pointless, but you
should probably honor it anyway and move this block below it.

https://codereview.chromium.org/19730002/diff/102002/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:374: }
likewise

[not at google - send to devlin, 2013-08-07 22:08:40]

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.cc (right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:46: *out = new
base::StringValue(std::string(*utf8, utf8.length()));
On 2013/08/07 22:07:56, kalman wrote:
> you should be able to do just
> 
> name = v8::String::Concat(...);
> *out = new base::StringValue(*v8::String::AsciiValue(name))

where by AsciiValue I mean Utf8Value. Or is the length() part really
necessary..?

[pmarch, 2013-08-08 00:53:07]

addressed Ben's comments

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.cc (right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:30: 
On 2013/08/07 22:07:56, kalman wrote:
> add a comment about the array index stuff we've been talking about.

Done.

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:46: *out = new
base::StringValue(std::string(*utf8, utf8.length()));
On 2013/08/07 22:08:40, kalman wrote:
> On 2013/08/07 22:07:56, kalman wrote:
> > you should be able to do just
> > 
> > name = v8::String::Concat(...);
> > *out = new base::StringValue(*v8::String::AsciiValue(name))
> 
> where by AsciiValue I mean Utf8Value. Or is the length() part really
> necessary..?

Done.

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:46: *out = new
base::StringValue(std::string(*utf8, utf8.length()));
On 2013/08/07 22:07:56, kalman wrote:
> you should be able to do just
> 
> name = v8::String::Concat(...);
> *out = new base::StringValue(*v8::String::AsciiValue(name))

Done.

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:47: return true;
// Prevent V8ValueConverter from further processing this object.
On 2013/08/07 22:07:56, kalman wrote:
> comment would look better on its own line

Done.

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
(right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:164: &&
out_string == std::string("[Function foo()]"));
On 2013/08/07 22:07:56, kalman wrote:
> These can all be done more concisely if you pull them into functions that
return
> testing::AssertionResults. 
> 
> Also most of them sholud be EXPECT_TRUE not ASSERT_TRUE.
> 
> Something like
> 
> EXPECT_TRUE(IsString("[Function]", v8_object, "function"));
> 
> where
> 
> v8::AssertionResult IsString(const std::string& string, v8::Local<v8::Object>
> object, const std::string& key) {
>   if (...)
>     return testing::AssertionFailure() << "failed cos..";
>   rturn testing::AssertionSucces();
> }
> 
> it's a nice pattern.

Done.

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/api_activity_logger.cc:62: // will call JS toString()
method which can be overwritten.
On 2013/08/07 22:07:56, kalman wrote:
> RegExp seems a bit like an implementation detail. We could equally change
> V8ValueConverter not to call ToString() on the regexp.
> 
> In fact you could change it to use GetSource().
> 
> All that aside - you're not disabling RegExp here anyway.

I've decided to kill this comment

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
File chrome/renderer/extensions/dom_activity_logger.cc (right):

https://codereview.chromium.org/19730002/diff/102002/chrome/renderer/extensio...
chrome/renderer/extensions/dom_activity_logger.cc:35: // Do not set the
converter's RegExpr to true via SetRegExpAllowed() as this
killed comment as well

https://codereview.chromium.org/19730002/diff/102002/content/renderer/v8_valu...
File content/renderer/v8_value_converter_impl.cc (right):

https://codereview.chromium.org/19730002/diff/102002/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:306: }
On 2013/08/07 22:07:56, kalman wrote:
> I think the context scope stuff below might actually be pointless, but you
> should probably honor it anyway and move this block below it.

Done.

https://codereview.chromium.org/19730002/diff/102002/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:374: }
On 2013/08/07 22:07:56, kalman wrote:
> likewise

Done.

[not at google - send to devlin, 2013-08-08 01:02:20]

lgtm

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.cc (right):

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:33: // callback.
it would be even nicer to file a bug on the missing v8 feature and mention it
here, so that readers know you're tracking it.

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.h (right):

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.h:15: : public
content::V8ValueConverter::Strategy {
class-level comment? should have asked for this before.

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
(right):

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:25:
context_.Reset(isolate_, v8::Context::New(isolate_, NULL, global));
global not needed

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:39:
scoped_ptr<base::Value> value(converter_->FromV8Value(v8_value, context));
you should be able to pass context_ into these directly without needing to
create another handle (context)

would reduce line count in these tests significantly

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:99:
v8::Persistent<v8::Context> context_;
use ScopedPersistent, don't need the Dispose

https://codereview.chromium.org/19730002/diff/120001/content/renderer/v8_valu...
File content/renderer/v8_value_converter_impl.cc (right):

https://codereview.chromium.org/19730002/diff/120001/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:309: }
it needs to be after scope is reset() below.

https://codereview.chromium.org/19730002/diff/120001/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:379: }
ditto

[pmarch, 2013-08-08 03:20:18]

Thanks Ben

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.cc (right):

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:33: // callback.
On 2013/08/08 01:02:21, kalman wrote:
> it would be even nicer to file a bug on the missing v8 feature and mention it
> here, so that readers know you're tracking it.

Issue 269848
done.

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.h (right):

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.h:15: : public
content::V8ValueConverter::Strategy {
On 2013/08/08 01:02:21, kalman wrote:
> class-level comment? should have asked for this before.

Done.

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc
(right):

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:25:
context_.Reset(isolate_, v8::Context::New(isolate_, NULL, global));
On 2013/08/08 01:02:21, kalman wrote:
> global not needed

Done.

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:39:
scoped_ptr<base::Value> value(converter_->FromV8Value(v8_value, context));
On 2013/08/08 01:02:21, kalman wrote:
> you should be able to pass context_ into these directly without needing to
> create another handle (context)
> 
> would reduce line count in these tests significantly

Done.

https://codereview.chromium.org/19730002/diff/120001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy_unittest.cc:99:
v8::Persistent<v8::Context> context_;
On 2013/08/08 01:02:21, kalman wrote:
> use ScopedPersistent, don't need the Dispose

Done.

https://codereview.chromium.org/19730002/diff/120001/content/renderer/v8_valu...
File content/renderer/v8_value_converter_impl.cc (right):

https://codereview.chromium.org/19730002/diff/120001/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:309: }
On 2013/08/08 01:02:21, kalman wrote:
> it needs to be after scope is reset() below.

gotcha

https://codereview.chromium.org/19730002/diff/120001/content/renderer/v8_valu...
content/renderer/v8_value_converter_impl.cc:379: }
On 2013/08/08 01:02:21, kalman wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-08-08 16:45:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/19730002/132001

[commit-bot: I haz the power, 2013-08-08 16:48:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/19730002/132001

[pmarch, 2013-08-08 18:30:24]

Hi, could you please review owned files

jamesr: 
  content/renderer/v8_value_converter_impl.cc
  content/renderer/v8_value_converter_impl.h

joi:
  content/public/renderer/v8_value_converter.h

Thanks

[jamesr, 2013-08-09 21:10:24]

I don't think I understand V8 internals well enough to understand the
implications of this patch.  Adding jochen@ as a reviewer instead.

[dcarney, 2013-08-12 08:47:03]

lgtm (wrt the v8 api calls)

see comment though

https://codereview.chromium.org/19730002/diff/132001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.cc (right):

https://codereview.chromium.org/19730002/diff/132001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:46: name =
v8::String::Concat(name, value->ToObject()->GetConstructorName());
Do you want ToObject?  This is relatively expensive compared to the other things
happening here.  An IsObject check and a Cast would be more inline with the code
for function above in terms of cost, but I have no idea what the goal here is.

[Jói, 2013-08-12 10:23:46]

The interface change in //content/public LGTM, but I don't know much about V8 so
please defer to the review by jochen@ or another reviewer familiar with V8.

Cheers,
Jói

[jochen (gone - plz use gerrit), 2013-08-12 12:08:12]

I asked Dan to do the V8 specific code review, so please don't wait for me

[pmarch, 2013-08-12 13:39:25]

https://codereview.chromium.org/19730002/diff/132001/chrome/renderer/extensio...
File chrome/renderer/extensions/activity_log_converter_strategy.cc (right):

https://codereview.chromium.org/19730002/diff/132001/chrome/renderer/extensio...
chrome/renderer/extensions/activity_log_converter_strategy.cc:46: name =
v8::String::Concat(name, value->ToObject()->GetConstructorName());
On 2013/08/12 08:47:04, dcarney wrote:
> Do you want ToObject?  This is relatively expensive compared to the other
things
> happening here.  An IsObject check and a Cast would be more inline with the
code
> for function above in terms of cost, but I have no idea what the goal here is.

Thanks Dan, nice catch. TOObject is absolutely redundant here.

[pmarch, 2013-08-12 13:41:13]

On 2013/08/12 12:08:12, jochen wrote:
> I asked Dan to do the V8 specific code review, so please don't wait for me

Jochen, I need LGTM from either you or James as you are the owners of
 content/renderer/v8_value_converter_impl.cc
 content/renderer/v8_value_converter_impl.h

Thanks

[jochen (gone - plz use gerrit), 2013-08-12 13:44:28]

lgtm

[commit-bot: I haz the power, 2013-08-12 13:46:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/19730002/153001

[commit-bot: I haz the power, 2013-08-12 17:51:54]

Change committed as 217032