Prevent extensions from clobbering JSON implementation that extension calls use

Prevent extensions from clobbering JSON implementation that extension calls use

BUG=38857
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48667

[Aaron Boodman, 2010-05-28 21:42:50]

http://codereview.chromium.org/2387002/diff/1/4
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/2387002/diff/1/4#newcode23
chrome/renderer/resources/renderer_extension_bindings.js:23: chromeHidden.JSON =
{};
Since everything relies on event_bindings.js, it would be cleaner to do this
there.

Also, you can centralize all the logic in one place like this:

chromeHidden.JSON = new (function() {
  var originalParse = JSON.parse;
  var originalStringify = JSON.stringify;

  ... anything else that needs to be saved ...

  this.stringify = function(thing) {
    // restore things to their default state
    delete Object.prototype.toJSONString;
    delete Array.prototype.toJSONString;

   // it is important to try/finally so that in case an error gets thrown, we
reset things.
    try {
      do stuff
    } finally {
      put back customizations
    }
  };

  ... any other methods that need to be exposed ...
}

Then clients can just call: chromeHidden.JSON.stringify().

[rafaelw, 2010-05-28 22:20:39]

http://codereview.chromium.org/2387002/diff/1/4
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/2387002/diff/1/4#newcode23
chrome/renderer/resources/renderer_extension_bindings.js:23: chromeHidden.JSON =
{};
On 2010/05/28 21:42:50, Aaron Boodman wrote:
> Since everything relies on event_bindings.js, it would be cleaner to do this
> there.
> 
> Also, you can centralize all the logic in one place like this:
> 
> chromeHidden.JSON = new (function() {
>   var originalParse = JSON.parse;
>   var originalStringify = JSON.stringify;
> 
>   ... anything else that needs to be saved ...
> 
>   this.stringify = function(thing) {
>     // restore things to their default state
>     delete Object.prototype.toJSONString;
>     delete Array.prototype.toJSONString;
> 
>    // it is important to try/finally so that in case an error gets thrown, we
> reset things.
>     try {
>       do stuff
>     } finally {
>       put back customizations
>     }
>   };
> 
>   ... any other methods that need to be exposed ...
> }
> 
> Then clients can just call: chromeHidden.JSON.stringify().

Done.

[Aaron Boodman, 2010-05-28 22:25:18]

Nice. LGTM.

[arv1, 2010-05-29 04:36:01]

I don't have access to a computer but toJSONString is not used by JSON so
you should not need to handle that. toJSON is. Do you mind waiting until
Tuesday so I can look at this?

On May 28, 2010 5:43 PM, <aa@chromium.org> wrote:

http://codereview.chromium.org/2387002/diff/1/4
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/2387002/diff/1/4#newcode23
chrome/renderer/resources/renderer_extension_bindings.js:23:
chromeHidden.JSON = {};
Since everything relies on event_bindings.js, it would be cleaner to do
this there.

Also, you can centralize all the logic in one place like this:

chromeHidden.JSON = new (function() {
 var originalParse = JSON.parse;
 var originalStringify = JSON.stringify;

 ... anything else that needs to be saved ...

 this.stringify = function(thing) {
   // restore things to their default state
   delete Object.prototype.toJSONString;
   delete Array.prototype.toJSONString;

  // it is important to try/finally so that in case an error gets
thrown, we reset things.
   try {
     do stuff
   } finally {
     put back customizations
   }
 };

 ... any other methods that need to be exposed ...
}

Then clients can just call: chromeHidden.JSON.stringify().



http://codereview.chromium.org/2387002/show

[rafaelw1, 2010-05-29 05:20:16]

The patch handles toJSON (not toJSONString). I think Aaron was just
being "short-handy". It also includes tests (that would fail prior to
the change).

I'll wait until Tuesday so you can have a look, though.

R

On Fri, May 28, 2010 at 9:35 PM, Erik Arvidsson <arv@google.com> wrote:
> I don't have access to a computer but toJSONString is not used by JSON so
> you should not need to handle that. toJSON is. Do you mind waiting until
> Tuesday so I can look at this?
>
> On May 28, 2010 5:43 PM, <aa@chromium.org> wrote:
>
> http://codereview.chromium.org/2387002/diff/1/4
> File chrome/renderer/resources/renderer_extension_bindings.js (right):
>
> http://codereview.chromium.org/2387002/diff/1/4#newcode23
> chrome/renderer/resources/renderer_extension_bindings.js:23:
> chromeHidden.JSON = {};
> Since everything relies on event_bindings.js, it would be cleaner to do
> this there.
>
> Also, you can centralize all the logic in one place like this:
>
> chromeHidden.JSON = new (function() {
>  var originalParse = JSON.parse;
>  var originalStringify = JSON.stringify;
>
>  ... anything else that needs to be saved ...
>
>  this.stringify = function(thing) {
>    // restore things to their default state
>    delete Object.prototype.toJSONString;
>    delete Array.prototype.toJSONString;
>
>   // it is important to try/finally so that in case an error gets
> thrown, we reset things.
>    try {
>      do stuff
>    } finally {
>      put back customizations
>    }
>  };
>
>  ... any other methods that need to be exposed ...
> }
>
> Then clients can just call: chromeHidden.JSON.stringify().
>
> http://codereview.chromium.org/2387002/show

[PhistucK, 2010-05-29 12:42:26]

http://codereview.chromium.org/2387002/diff/9001/7
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/2387002/diff/9001/7#newcode28
chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON =
customizedObjectToJSON;
Did you not mean originalStringify here (or something similar)?

It seems as if you basically coded this -
If there is a "toJSON" method on the prototype of Object and Array, delete it
and re-add the same implementation (that was saved in customizedObjectToJSON).
Unless I am missing something, why bother?

http://codereview.chromium.org/2387002/diff/9001/7#newcode31
chrome/renderer/resources/event_bindings.js:31: Array.prototype.toJSON =
customizedArrayToJSON;
Same here?

[rafaelw, 2010-05-29 14:17:09]

http://codereview.chromium.org/2387002/diff/9001/7
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/2387002/diff/9001/7#newcode28
chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON =
customizedObjectToJSON;
The idea here is that if the extension has setup a custom toJSON function for
Array or Object, we want to disable it while we are using JSON.stringify, but
put them back after we are done.

On 2010/05/29 12:42:27, PhistucK wrote:
> Did you not mean originalStringify here (or something similar)?
> 
> It seems as if you basically coded this -
> If there is a "toJSON" method on the prototype of Object and Array, delete it
> and re-add the same implementation (that was saved in customizedObjectToJSON).
> Unless I am missing something, why bother?

[PhistucK, 2010-05-29 14:24:29]

On 2010/05/29 14:17:09, rafaelw wrote:
> http://codereview.chromium.org/2387002/diff/9001/7
> File chrome/renderer/resources/event_bindings.js (right):
> 
> http://codereview.chromium.org/2387002/diff/9001/7#newcode28
> chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON =
> customizedObjectToJSON;
> The idea here is that if the extension has setup a custom toJSON function for
> Array or Object, we want to disable it while we are using JSON.stringify, but
> put them back after we are done.
So they will not be included in the stringifying process?

[Aaron Boodman, 2010-05-29 16:28:55]

On Sat, May 29, 2010 at 7:24 AM,  <phistuck@gmail.com> wrote:
> On 2010/05/29 14:17:09, rafaelw wrote:
>>
>> http://codereview.chromium.org/2387002/diff/9001/7
>> File chrome/renderer/resources/event_bindings.js (right):
>
>> http://codereview.chromium.org/2387002/diff/9001/7#newcode28
>> chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON =
>> customizedObjectToJSON;
>> The idea here is that if the extension has setup a custom toJSON function
>> for
>> Array or Object, we want to disable it while we are using JSON.stringify,
>> but
>> put them back after we are done.
>
> So they will not be included in the stringifying process?

No, they will not. Some developers use libraries that provide
implementations of Array.prototype.toJSON and Object.prototype.toJSON.
These library implementations actually have bugs and we don't want to
use them.

- a

[PhistucK, 2010-05-29 19:24:23]

On 2010/05/29 16:28:55, Aaron Boodman wrote:
> On Sat, May 29, 2010 at 7:24 AM,  <mailto:phistuck@gmail.com> wrote:
> > On 2010/05/29 14:17:09, rafaelw wrote:
> >>
> >> http://codereview.chromium.org/2387002/diff/9001/7
> >> File chrome/renderer/resources/event_bindings.js (right):
> >
> >> http://codereview.chromium.org/2387002/diff/9001/7#newcode28
> >> chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON =
> >> customizedObjectToJSON;
> >> The idea here is that if the extension has setup a custom toJSON function
> >> for
> >> Array or Object, we want to disable it while we are using JSON.stringify,
> >> but
> >> put them back after we are done.
> >
> > So they will not be included in the stringifying process?
> 
> No, they will not. Some developers use libraries that provide
> implementations of Array.prototype.toJSON and Object.prototype.toJSON.
> These library implementations actually have bugs and we don't want to
> use them.
Sorry, I meant to ask - are you doing this in order to make them not included in
the stringifying process?
I do not know how the stringifying process works, does it internally use toJSON
for Object and Array?
If I am totally not following here, you can just ignore. Sorry.
> - a
>

[Aaron Boodman, 2010-05-29 21:42:28]

On Sat, May 29, 2010 at 12:24 PM,  <phistuck@gmail.com> wrote:
> On 2010/05/29 16:28:55, Aaron Boodman wrote:
>>
>> On Sat, May 29, 2010 at 7:24 AM,  <mailto:phistuck@gmail.com> wrote:
>> > On 2010/05/29 14:17:09, rafaelw wrote:
>> >>
>> >> http://codereview.chromium.org/2387002/diff/9001/7
>> >> File chrome/renderer/resources/event_bindings.js (right):
>> >
>> >> http://codereview.chromium.org/2387002/diff/9001/7#newcode28
>> >> chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON
>> >> =
>> >> customizedObjectToJSON;
>> >> The idea here is that if the extension has setup a custom toJSON
>> >> function
>> >> for
>> >> Array or Object, we want to disable it while we are using
>> >> JSON.stringify,
>> >> but
>> >> put them back after we are done.
>> >
>> > So they will not be included in the stringifying process?
>
>> No, they will not. Some developers use libraries that provide
>> implementations of Array.prototype.toJSON and Object.prototype.toJSON.
>> These library implementations actually have bugs and we don't want to
>> use them.
>
> Sorry, I meant to ask - are you doing this in order to make them not
> included in
> the stringifying process?
> I do not know how the stringifying process works, does it internally use
> toJSON
> for Object and Array?

Ah, the way the internal implementation of JSON.stringify works, it
looks for a "toJSON" method on each object and calls that if it is
there. Otherwise, it uses its own internal implementation of toJSON.
It turns out that in this case the expectations of the internal
version of JSON.stringify are not matching the outputs of the custom
toJSON methods (which are normally called from the library's own
top-level stringify() method).

- a

[PhistucK, 2010-05-30 04:06:57]

On 2010/05/29 21:42:28, Aaron Boodman wrote:
> Ah, the way the internal implementation of JSON.stringify works, it
> looks for a "toJSON" method on each object and calls that if it is
> there. Otherwise, it uses its own internal implementation of toJSON.
> It turns out that in this case the expectations of the internal
> version of JSON.stringify are not matching the outputs of the custom
> toJSON methods (which are normally called from the library's own
> top-level stringify() method).
> 
> - a
> 

Everything is clear now, thank you!


PhistucK

[arv (Not doing code reviews), 2010-06-01 17:15:29]

http://codereview.chromium.org/2387002/diff/9001/7
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/2387002/diff/9001/7#newcode22
chrome/renderer/resources/event_bindings.js:22: delete Object.prototype.toJSON;
delete makes things slow in V8. Can you check for presence and set to null
instead?

http://codereview.chromium.org/2387002/diff/9001/7#newcode28
chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON =
customizedObjectToJSON;
Object and Array might have been replaced as well.

I think it might make sense to introduce local variables in the top level
anonymous scope that points to originals.


(function() {
  const $Array = Array;
  const $jsonStringify = JSON.prototype.stringify;
  ...

And then just use the original functions directly.

You will still need to override toJSON before calling stringify.

[rafaelw, 2010-06-01 21:19:42]

arv: PTAL

http://codereview.chromium.org/2387002/diff/9001/7
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/2387002/diff/9001/7#newcode22
chrome/renderer/resources/event_bindings.js:22: delete Object.prototype.toJSON;
On 2010/06/01 17:15:29, arv wrote:
> delete makes things slow in V8. Can you check for presence and set to null
> instead?

Done.

http://codereview.chromium.org/2387002/diff/9001/7#newcode28
chrome/renderer/resources/event_bindings.js:28: Object.prototype.toJSON =
customizedObjectToJSON;
On 2010/06/01 17:15:29, arv wrote:
> Object and Array might have been replaced as well.
> 
> I think it might make sense to introduce local variables in the top level
> anonymous scope that points to originals.
> 
> 
> (function() {
>   const $Array = Array;
>   const $jsonStringify = JSON.prototype.stringify;
>   ...
> 
> And then just use the original functions directly.
> 
> You will still need to override toJSON before calling stringify.

Done. Note that JSON isn't a type so it doesn't have a direct prototype.

[arv (Not doing code reviews), 2010-06-01 21:38:22]

lgtm

http://codereview.chromium.org/2387002/diff/22001/23001
File chrome/renderer/resources/event_bindings.js (right):

http://codereview.chromium.org/2387002/diff/22001/23001#newcode15
chrome/renderer/resources/event_bindings.js:15: chromeHidden.JSON = new
(function() {
I don't really see the point for the "new (function() {" syntax here but if you
find it easier then it is all good.

const $Object = ...;
...

chromeHidden.JSON = {
  stringify: ...,
  parse: ...
};

[arv (Not doing code reviews), 2010-06-01 21:38:55]

On 2010/06/01 21:19:42, rafaelw wrote:

> Done. Note that JSON isn't a type so it doesn't have a direct prototype.

Yeah, that was just a refactoring error.