[Extensions] Don't allow built-in extensions code to be overridden

[Extensions] Don't allow built-in extensions code to be overridden
BUG=546677
Committed: https://crrev.com/83a4b3aa72d98fe4176b4a54c8cea227ed966570
Cr-Commit-Position: refs/heads/master@{#356654}

[Devlin, 2015-10-23 22:08:27]

rdevlin.cronin@chromium.org changed reviewers:
+ jochen@chromium.org

[Devlin, 2015-10-23 22:08:28]

Jochen, mind taking a look?

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/module_...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:106: obj->ForceSet(args[0], args[1],
v8::ReadOnly);
This works, but it looks like ForceSet() is deprecated.  What's the alternative?
 (Set() will just hit the custom setter, so that doesn't work.)

Another approach would be using the safe-builtin $Object.defineProperty() in the
.js files - but I was struggling to find a way to put that into a function that
could be passed in in WrapSource().

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/resourc...
File extensions/renderer/resources/last_error.js (right):

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/resourc...
extensions/renderer/resources/last_error.js:137: exports.$set('clear', clear);
Once we decide what's best here, this'll need to be done on all exports.

[jochen (gone - plz use gerrit), 2015-10-26 15:46:54]

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/module_...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:106: obj->ForceSet(args[0], args[1],
v8::ReadOnly);
On 2015/10/23 at 22:08:28, Devlin wrote:
> This works, but it looks like ForceSet() is deprecated.  What's the
alternative?  (Set() will just hit the custom setter, so that doesn't work.)
> 
> Another approach would be using the safe-builtin $Object.defineProperty() in
the .js files - but I was struggling to find a way to put that into a function
that could be passed in in WrapSource().

You'd use v8::Object::DefineOwnProperty()

[Devlin, 2015-10-26 19:18:49]

Patchset #2 (id:20001) has been deleted

[Devlin, 2015-10-26 21:18:17]

Patchset #2 (id:40001) has been deleted

[Devlin, 2015-10-27 23:08:54]

Okay, I think this is ready.

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/module_...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1417513003/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:106: obj->ForceSet(args[0], args[1],
v8::ReadOnly);
On 2015/10/26 15:46:54, jochen wrote:
> On 2015/10/23 at 22:08:28, Devlin wrote:
> > This works, but it looks like ForceSet() is deprecated.  What's the
> alternative?  (Set() will just hit the custom setter, so that doesn't work.)
> > 
> > Another approach would be using the safe-builtin $Object.defineProperty() in
> the .js files - but I was struggling to find a way to put that into a function
> that could be passed in in WrapSource().
> 
> You'd use v8::Object::DefineOwnProperty()

Ah, for some reason I thought that would hit the accessor, but looks like it
works.

[jochen (gone - plz use gerrit), 2015-10-28 14:22:19]

lgtm

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
extensions/renderer/module_system.cc:107:
obj->DefineOwnProperty(args.GetIsolate()->GetCallingContext(),
I know this is copy/paste, but this should really be GetCurrentContext().

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
extensions/renderer/module_system.cc:109: if (!result.IsJust() ||
!result.FromJust())
if (!result.FromMaybe(false)) is shorter

[Devlin, 2015-10-28 20:38:18]

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
extensions/renderer/module_system.cc:107:
obj->DefineOwnProperty(args.GetIsolate()->GetCallingContext(),
On 2015/10/28 14:22:18, jochen wrote:
> I know this is copy/paste, but this should really be GetCurrentContext().

Done.  For my own edification, can you explain why?  (Naively, I would think
using the context that called the JS code would be preferred over whichever
context is on the top of the stack.)

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
extensions/renderer/module_system.cc:109: if (!result.IsJust() ||
!result.FromJust())
On 2015/10/28 14:22:18, jochen wrote:
> if (!result.FromMaybe(false)) is shorter

Nifty, done.

[Devlin, 2015-10-28 20:38:24]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-10-28 20:38:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1417513003/#ps100001
(title: "Jochen's")

[commit-bot: I haz the power, 2015-10-28 20:39:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417513003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417513003/100001

[commit-bot: I haz the power, 2015-10-28 21:44:06]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-10-28 21:45:04]

Patchset 4 (id:??) landed as
https://crrev.com/83a4b3aa72d98fe4176b4a54c8cea227ed966570
Cr-Commit-Position: refs/heads/master@{#356654}

[dcheng, 2015-10-30 21:32:58]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-10-30 21:32:59]

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/1417513003/diff/80001/extensions/renderer/mod...
extensions/renderer/module_system.cc:107:
obj->DefineOwnProperty(args.GetIsolate()->GetCallingContext(),
On 2015/10/28 at 20:38:17, Devlin wrote:
> On 2015/10/28 14:22:18, jochen wrote:
> > I know this is copy/paste, but this should really be GetCurrentContext().
> 
> Done.  For my own edification, can you explain why?  (Naively, I would think
using the context that called the JS code would be preferred over whichever
context is on the top of the stack.)

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/aEkGnu1mkBc has
some context which might be helpful.

Disclaimer: I myself find the current vs calling context (and entered vs current
as well:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/SJZVnlbxFOY) to
be quite confusing.