[Extensions] Improve app custom bindings

[Extensions] Improve app custom bindings

The app custom bindings (and handler counterparts) had issues. Fix some
of them.

BUG=None

Review-Url: https://codereview.chromium.org/2859233003
Cr-Commit-Position: refs/heads/master@{#471085}
Committed: https://chromium.googlesource.com/chromium/src/+/78dd7afe685c9f37a5d0c1e48f1d2f2fc78a1469

[Devlin, 2017-05-04 18:58:49]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-05-04 18:58:51]

Hey folks, mind taking a look?  Not directly related to native bindings, but
we'll reuse some of this code and I wanted to separate out the improvements.

As background, the "chrome.app" API is... bad.  It's totally undocumented, but
we have *no* usage stats on it, so it's probably best to support it until we can
prove it's deprecatable.

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/app_bindings.cc (left):

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/app_bindings.cc:80: v8::Isolate* isolate =
frame->MainWorldScriptContext()->GetIsolate();
Even though this is the same isolate in practice, it seemed wrong.

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/app_bindings.cc:96: frame->MainWorldScriptContext());
This seemed *very* wrong, since it's not the context we're returning the object
to.

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/app_bindings.cc:102: int callback_id = 0;
We should only get valid calls to this function, so remove the graceful
handling.

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:45: #include
"third_party/WebKit/public/platform/WebURL.h"
iwyu fix

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/app_custom_bindings.js:38:
$Object.defineProperty(
all these changes are just to guard against untrusted code.

[Devlin, 2017-05-06 01:11:29]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 01:12:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 02:03:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-06 02:03:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-05-06 02:30:05]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 02:30:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 03:09:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-06 03:09:26]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-05-08 14:25:25]

lgtm

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/app_bindings.cc (left):

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/app_bindings.cc:96: frame->MainWorldScriptContext());
On 2017/05/04 at 18:58:50, Devlin wrote:
> This seemed *very* wrong, since it's not the context we're returning the
object to.

If it can be called in a content script (it seems like it is intended for
main-world usage, from what I can gather from its undocumentedness), that looks
like it would have allowed cross-world object leakage.

https://codereview.chromium.org/2859233003/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/2859233003/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/app_custom_bindings.js:79: get: function()
{ return installState; },
This makes it an accessor property as opposed to a data property. Intentional?
I'd have expected:

$Object.defineProperty(
    app, 'installState',
    {
      __proto__: null,
      configurable: true,
      enumerable: true,
      // possibly also "writable: true" if you want to mimic the kind of
property app.installState ordinarily creates
      value: installState
    });

(Doesn't apply above, because you are replacing an accessor property there.)

[Devlin, 2017-05-10 22:04:11]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 22:05:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 23:52:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 23:52:05]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2017-05-10 23:53:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 23:53:13]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-11 14:30:38]

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/app_bindings.cc (left):

https://codereview.chromium.org/2859233003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/app_bindings.cc:96: frame->MainWorldScriptContext());
On 2017/05/08 14:25:24, jbroman wrote:
> On 2017/05/04 at 18:58:50, Devlin wrote:
> > This seemed *very* wrong, since it's not the context we're returning the
> object to.
> 
> If it can be called in a content script (it seems like it is intended for
> main-world usage, from what I can gather from its undocumentedness), that
looks
> like it would have allowed cross-world object leakage.

Yep, it could be called from any context, which is why this was scary.

https://codereview.chromium.org/2859233003/diff/40001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/app_custom_bindings.js (right):

https://codereview.chromium.org/2859233003/diff/40001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/app_custom_bindings.js:79: get: function()
{ return installState; },
On 2017/05/08 14:25:25, jbroman wrote:
> This makes it an accessor property as opposed to a data property. Intentional?
> I'd have expected:
> 
> $Object.defineProperty(
>     app, 'installState',
>     {
>       __proto__: null,
>       configurable: true,
>       enumerable: true,
>       // possibly also "writable: true" if you want to mimic the kind of
> property app.installState ordinarily creates
>       value: installState
>     });
> 
> (Doesn't apply above, because you are replacing an accessor property there.)

Done.

[lazyboy, 2017-05-11 18:38:42]

lgtm

[Devlin, 2017-05-11 21:53:30]

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

[Devlin, 2017-05-11 21:53:30]

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

[commit-bot: I haz the power, 2017-05-11 21:54:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 22:01:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494539610393240,
"parent_rev": "da176b49eafe0f94f32919620f7ea6fb3e0d808f", "commit_rev":
"78dd7afe685c9f37a5d0c1e48f1d2f2fc78a1469"}

[commit-bot: I haz the power, 2017-05-11 22:01:25]

Description was changed from

==========
[Extensions] Improve app custom bindings

The app custom bindings (and handler counterparts) had issues. Fix some
of them.

BUG=None
==========

to

==========
[Extensions] Improve app custom bindings

The app custom bindings (and handler counterparts) had issues. Fix some
of them.

BUG=None

Review-Url: https://codereview.chromium.org/2859233003
Cr-Commit-Position: refs/heads/master@{#471085}
Committed:
https://chromium.googlesource.com/chromium/src/+/78dd7afe685c9f37a5d0c1e48f1d...
==========

[commit-bot: I haz the power, 2017-05-11 22:01:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/78dd7afe685c9f37a5d0c1e48f1d...