[Extensions] Expand bindings access checks

[Extensions] Expand bindings access checks

BUG=601149
BUG=601073
Committed: https://crrev.com/c0569cc04741cccf6548c2169fcc1609d958523f
Cr-Commit-Position: refs/heads/master@{#387710}

[Devlin, 2016-04-07 19:56:50]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-04-07 19:57:05]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-04-07 19:57:12]

Patchset #1 (id:40001) has been deleted

[Devlin, 2016-04-07 19:57:18]

Patchset #1 (id:60001) has been deleted

[Devlin, 2016-04-07 19:57:24]

Patchset #1 (id:80001) has been deleted

[Devlin, 2016-04-07 19:57:30]

Patchset #1 (id:100001) has been deleted

[Devlin, 2016-04-07 20:57:28]

Description was changed from

==========
Foo
BUG=
==========

to

==========
[Extensions] Expand bindings access checks

BUG=601149
BUG=601073
==========

[Devlin, 2016-04-07 20:57:55]

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

[Devlin, 2016-04-07 20:57:56]

Antony, mind taking a look?

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/pr...
File extensions/renderer/print_native_handler.h (left):

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/pr...
extensions/renderer/print_native_handler.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
This appears to be totally unused

[asargent_no_longer_on_chrome, 2016-04-11 17:30:14]

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:133: // in
//extensions.
Can you explain why you moved these here from //extensions ? It's not
immediately obvious to me, since the code looks the same. Are there different
security mechanisms in place for the native handlers registered here vs the ones
in dispatcher.cc's Dispatcher::RegisterNativeHandlers ?

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/ob...
File extensions/renderer/object_backed_native_handler.cc (right):

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/ob...
extensions/renderer/object_backed_native_handler.cc:83: }
Did you intend for this to get checked in or were you just using this for local
testing/development?

[Devlin, 2016-04-13 19:51:26]

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:133: // in
//extensions.
On 2016/04/11 17:30:14, Antony Sargent wrote:
> Can you explain why you moved these here from //extensions ? It's not
> immediately obvious to me, since the code looks the same. Are there different
> security mechanisms in place for the native handlers registered here vs the
ones
> in dispatcher.cc's Dispatcher::RegisterNativeHandlers ?

These are for features that are only available in chrome, so we hit a DCHECK
when we try to restrict them to that feature (i18n, lazy_background_page) in
//extensions.

More fundamentally, we really shouldn't be including them in //extensions at
all, because they're unused and increase the size of the extensions target
(though that part isn't addressed here - that's the TODO).

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/ob...
File extensions/renderer/object_backed_native_handler.cc (right):

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/ob...
extensions/renderer/object_backed_native_handler.cc:83: }
On 2016/04/11 17:30:14, Antony Sargent wrote:
> Did you intend for this to get checked in or were you just using this for
local
> testing/development?

I wanted to check this one in (I think DVLOG is okay - locally I go straight to
LOG(WARNING) ;)).  It's useful information for debugging, and shouldn't clutter
the console, since it shouldn't likely be hit.

[asargent_no_longer_on_chrome, 2016-04-13 23:02:41]

lgtm

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:133: // in
//extensions.
On 2016/04/13 19:51:26, Devlin wrote:
> On 2016/04/11 17:30:14, Antony Sargent wrote:
> > Can you explain why you moved these here from //extensions ? It's not
> > immediately obvious to me, since the code looks the same. Are there
different
> > security mechanisms in place for the native handlers registered here vs the
> ones
> > in dispatcher.cc's Dispatcher::RegisterNativeHandlers ?
> 
> These are for features that are only available in chrome, so we hit a DCHECK
> when we try to restrict them to that feature (i18n, lazy_background_page) in
> //extensions.
> 
> More fundamentally, we really shouldn't be including them in //extensions at
> all, because they're unused and increase the size of the extensions target
> (though that part isn't addressed here - that's the TODO).

Ok, the part about i18n makes sense since its implementation is currently in
chrome/browser/extensions/api/i18n/ and the API feature is declared in
chrome/common/extensions/api/_api_features.json. 


Out of curiousity, what is it about LazyBackgroundPageNativeHandler that is
chrome specific? It looks like the methods there just send IPCs that get handled
in extensions/browser/process_manager.cc.


BTW, I imagine that both the i18n API and event pages are things that non-chrome
consumers of //extensions probably would want, so I guess we eventually will end
up needing to move these back.

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/ob...
File extensions/renderer/object_backed_native_handler.cc (right):

https://codereview.chromium.org/1866103002/diff/120001/extensions/renderer/ob...
extensions/renderer/object_backed_native_handler.cc:83: }
On 2016/04/13 19:51:26, Devlin wrote:
> On 2016/04/11 17:30:14, Antony Sargent wrote:
> > Did you intend for this to get checked in or were you just using this for
> local
> > testing/development?
> 
> I wanted to check this one in (I think DVLOG is okay - locally I go straight
to
> LOG(WARNING) ;)).  It's useful information for debugging, and shouldn't
clutter
> the console, since it shouldn't likely be hit.

Ok, just wasn't sure since a checked-in DVLOG is mostly only useful for local
builds. If this is something you foresee using a lot, I can see how having it
checked in is better than having to carry the patch around and always
re-applying it (eg via a git stash or whatever) and remembering to remove it
when uploading CLs.

[Devlin, 2016-04-15 21:39:24]

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/1866103002/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:133: // in
//extensions.
On 2016/04/13 23:02:40, Antony Sargent wrote:
> Out of curiousity, what is it about LazyBackgroundPageNativeHandler that is
> chrome specific? It looks like the methods there just send IPCs that get
handled
> in extensions/browser/process_manager.cc.

Right now, it's only used by tts, and tts is a feature declared in chrome.

> BTW, I imagine that both the i18n API and event pages are things that
non-chrome
> consumers of //extensions probably would want, so I guess we eventually will
end
> up needing to move these back. 

I imagine that's probably true, but we should move them over in bulk - if we can
help it, we shouldn't have features half-implemented in //chrome and
//extensions (though I admit this is somewhat a losing battle).

[Devlin, 2016-04-15 21:39:27]

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

[Devlin, 2016-04-15 21:39:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1866103002/#ps140001
(title: "Latest master")

[commit-bot: I haz the power, 2016-04-15 21:40:05]

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

[commit-bot: I haz the power, 2016-04-15 21:51:30]

Description was changed from

==========
[Extensions] Expand bindings access checks

BUG=601149
BUG=601073
==========

to

==========
[Extensions] Expand bindings access checks

BUG=601149
BUG=601073
==========

[commit-bot: I haz the power, 2016-04-15 21:51:31]

Committed patchset #2 (id:140001)

[commit-bot: I haz the power, 2016-04-15 21:53:49]

Description was changed from

==========
[Extensions] Expand bindings access checks

BUG=601149
BUG=601073
==========

to

==========
[Extensions] Expand bindings access checks

BUG=601149
BUG=601073

Committed: https://crrev.com/c0569cc04741cccf6548c2169fcc1609d958523f
Cr-Commit-Position: refs/heads/master@{#387710}
==========

[commit-bot: I haz the power, 2016-04-15 21:53:49]

Patchset 2 (id:??) landed as
https://crrev.com/c0569cc04741cccf6548c2169fcc1609d958523f
Cr-Commit-Position: refs/heads/master@{#387710}