[Extensions Bindings] Notify of event unregistration on invalidation

[Extensions Bindings] Notify of event unregistration on invalidation

When contexts are invalidated, we consider the events in that context
unregistered and notify the browser accordingly. Hook up an
InvalidateContext() method on APIEventHandler and call it through
blink's WillReleaseScriptContext().

Add associated tests.

BUG=653596
Review-Url: https://codereview.chromium.org/2722463006
Cr-Commit-Position: refs/heads/master@{#454911}
Committed: https://chromium.googlesource.com/chromium/src/+/317a9b943cc6e28bb3f7d0cda4ae9382d84f9288

[Devlin, 2017-02-28 22:39:03]

Description was changed from

==========
event
BUG=
==========

to

==========
[Extensions Bindings] Notify of event unregistration on invalidation

When contexts are invalidated, we consider the events in that context
unregistered and notify the browser accordingly. Hook up an
InvalidateContext() method on APIEventHandler and call it through
blink's WillReleaseScriptContext().

Add associated tests.

BUG=653596
==========

[Devlin, 2017-02-28 22:39:11]

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

[commit-bot: I haz the power, 2017-02-28 22:40:05]

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-02-28 22:44:30]

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

[commit-bot: I haz the power, 2017-02-28 22:44:30]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2017-03-01 01:26:07]

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

[commit-bot: I haz the power, 2017-03-01 01:26: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-03-01 03:23:28]

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

[commit-bot: I haz the power, 2017-03-01 03:23:28]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-03-01 15:14:40]

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

[Devlin, 2017-03-01 15:14:41]

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:30: ~APIEventPerContextData() override
{
If we wanted, we could maybe also make this DCHECK(event_data.empty()); and
assume that InvalidateContext() will be called for each context.  It would mean
updating our tests, but should be doable if it makes sense.  Lemme know if you
think it's worthwhile.

[jbroman, 2017-03-01 17:49:47]

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:30: ~APIEventPerContextData() override
{
On 2017/03/01 at 15:14:41, Devlin wrote:
> If we wanted, we could maybe also make this DCHECK(event_data.empty()); and
assume that InvalidateContext() will be called for each context.  It would mean
updating our tests, but should be doable if it makes sense.  Lemme know if you
think it's worthwhile.

Yeah, it's a little messy, but I'd rather not have two paths to clean this up,
especially given we only expect one of them to be reached in production.

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:53: // destructor), but we need to do
this before the context is fully removed.
Because it's gone from the ScriptContextSet, right?

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:61: if (!valid_)
This won't catch such objects which don't are created after the fact. If we're
worried about listeners being added after the context has begun to be torn down
(not sure whether there's a way to run script as that happens -- hope not), then
we should be equally worried about event instances that haven't yet been created
(but will be created by an accessor when they are first reached). For that
reason I'd expect this state to be checked on the PerContextData, rather than
being a per-emitter bit.

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:89: if (!valid_)
Isn't |listeners_| empty in this case anyhow?

[Devlin, 2017-03-01 19:03:51]

In a followup, I think I'm gonna rework contexts a bit in the test
infrastructure to make some of this a little simpler, but I think it's okay-ish
for this CL.

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:30: ~APIEventPerContextData() override
{
On 2017/03/01 17:49:47, jbroman wrote:
> On 2017/03/01 at 15:14:41, Devlin wrote:
> > If we wanted, we could maybe also make this DCHECK(event_data.empty()); and
> assume that InvalidateContext() will be called for each context.  It would
mean
> updating our tests, but should be doable if it makes sense.  Lemme know if you
> think it's worthwhile.
> 
> Yeah, it's a little messy, but I'd rather not have two paths to clean this up,
> especially given we only expect one of them to be reached in production.

Done.

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
File extensions/renderer/api_event_handler.h (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/api...
extensions/renderer/api_event_handler.h:53: // destructor), but we need to do
this before the context is fully removed.
On 2017/03/01 17:49:47, jbroman wrote:
> Because it's gone from the ScriptContextSet, right?

Correct (and the ScriptContext wrapper is deleted); extended the comment.

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:61: if (!valid_)
On 2017/03/01 17:49:47, jbroman wrote:
> This won't catch such objects which don't are created after the fact. If we're
> worried about listeners being added after the context has begun to be torn
down
> (not sure whether there's a way to run script as that happens -- hope not),
then
> we should be equally worried about event instances that haven't yet been
created
> (but will be created by an accessor when they are first reached). For that
> reason I'd expect this state to be checked on the PerContextData, rather than
> being a per-emitter bit.

Yeah, I still don't have a complete grasp of all the weirdness that can happen.
:)

Re new event emitters: we currently have a DCHECK(per_context_data) in
CreateEventInstance() that should prevent new emitters from being created once
tear down has started and the PerContextData is destroyed.  Depending on if we
see crashes, we may need to change it to a graceful handling.

Re this check: my (very holey) understanding was that sometimes contexts (or
objects from the context) can leak even after willReleaseScriptContext() is
called by blink.  In that case, an object like a specific event, if held onto by
another object, could maybe be called after the fact.  That's what this is
guarding against.  Is that not a concern?

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:89: if (!valid_)
On 2017/03/01 17:49:47, jbroman wrote:
> Isn't |listeners_| empty in this case anyhow?

Yep, but this seemed more obvious to readers.  If you prefer, I can remove it
and insert a comment instead.  (Although both may be moot with above?)

[jbroman, 2017-03-01 19:26:35]

lgtm

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:61: if (!valid_)
On 2017/03/01 at 19:03:50, Devlin wrote:
> On 2017/03/01 17:49:47, jbroman wrote:
> > This won't catch such objects which don't are created after the fact. If
we're
> > worried about listeners being added after the context has begun to be torn
down
> > (not sure whether there's a way to run script as that happens -- hope not),
then
> > we should be equally worried about event instances that haven't yet been
created
> > (but will be created by an accessor when they are first reached). For that
> > reason I'd expect this state to be checked on the PerContextData, rather
than
> > being a per-emitter bit.
> 
> Yeah, I still don't have a complete grasp of all the weirdness that can
happen. :)
> 
> Re new event emitters: we currently have a DCHECK(per_context_data) in
CreateEventInstance() that should prevent new emitters from being created once
tear down has started and the PerContextData is destroyed.  Depending on if we
see crashes, we may need to change it to a graceful handling.

Well, that's slightly later than WillReleaseScriptContext, but I don't think
script can run in between (basically I'd have to audit every override of
WillReleaseScriptContext to make sure none of them run script -- but doing so
seems insane, so I'll assume not).

> Re this check: my (very holey) understanding was that sometimes contexts (or
objects from the context) can leak even after willReleaseScriptContext() is
called by blink.  In that case, an object like a specific event, if held onto by
another object, could maybe be called after the fact.  That's what this is
guarding against.  Is that not a concern?

Ah yes, you're quite correct. My bad. :)

If you are going to keep this as a bit on the emitters themselves as opposed to
on the context (fair enough), would you mind a comment in
APIEventHandler::InvalidateContext explaining that no more event emitters get
created after that point?

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:121: DCHECK(valid_);
Should this be a DCHECK or an early return? Scripts can presently call this
(with no bug in our code), right?

[Devlin, 2017-03-02 17:26:32]

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
File extensions/renderer/event_emitter.cc (right):

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:61: if (!valid_)
On 2017/03/01 19:26:34, jbroman wrote:
> On 2017/03/01 at 19:03:50, Devlin wrote:
> > On 2017/03/01 17:49:47, jbroman wrote:
> > > This won't catch such objects which don't are created after the fact. If
> we're
> > > worried about listeners being added after the context has begun to be torn
> down
> > > (not sure whether there's a way to run script as that happens -- hope
not),
> then
> > > we should be equally worried about event instances that haven't yet been
> created
> > > (but will be created by an accessor when they are first reached). For that
> > > reason I'd expect this state to be checked on the PerContextData, rather
> than
> > > being a per-emitter bit.
> > 
> > Yeah, I still don't have a complete grasp of all the weirdness that can
> happen. :)
> > 
> > Re new event emitters: we currently have a DCHECK(per_context_data) in
> CreateEventInstance() that should prevent new emitters from being created once
> tear down has started and the PerContextData is destroyed.  Depending on if we
> see crashes, we may need to change it to a graceful handling.
> 
> Well, that's slightly later than WillReleaseScriptContext, but I don't think
> script can run in between (basically I'd have to audit every override of
> WillReleaseScriptContext to make sure none of them run script -- but doing so
> seems insane, so I'll assume not).
> 
> > Re this check: my (very holey) understanding was that sometimes contexts (or
> objects from the context) can leak even after willReleaseScriptContext() is
> called by blink.  In that case, an object like a specific event, if held onto
by
> another object, could maybe be called after the fact.  That's what this is
> guarding against.  Is that not a concern?
> 
> Ah yes, you're quite correct. My bad. :)
> 
> If you are going to keep this as a bit on the emitters themselves as opposed
to
> on the context (fair enough), would you mind a comment in
> APIEventHandler::InvalidateContext explaining that no more event emitters get
> created after that point?

Done.  Added comments in InvalidateContext() and also AddListener() to explain
what we're guarding against here.

https://codereview.chromium.org/2722463006/diff/40001/extensions/renderer/eve...
extensions/renderer/event_emitter.cc:121: DCHECK(valid_);
On 2017/03/01 19:26:34, jbroman wrote:
> Should this be a DCHECK or an early return? Scripts can presently call this
> (with no bug in our code), right?

Oh, right, we had to expose this.  Forgot about that.  (Ideally, this *should*
only be called by our own code, so it *shouldn't* happen after the context is
torn down - but the old system exposes it on the public object.)

Gracefully handling.

[lazyboy, 2017-03-03 22:14:20]

lgtm

https://codereview.chromium.org/2722463006/diff/80001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2722463006/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:141: data->event_data.clear();
FYI: If there's a chance that |data->event_data| can be accessed by lines
131-137 before this clear()...
I'd be careful and consider setting event_data aside first using std::swap() and
then iterating them on line 130. If not, then ignore.

[Devlin, 2017-03-06 18:22:55]

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

[commit-bot: I haz the power, 2017-03-06 18:23:40]

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-03-06 19:09:16]

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

[commit-bot: I haz the power, 2017-03-06 19:09:17]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-03-06 19:11:06]

https://codereview.chromium.org/2722463006/diff/80001/extensions/renderer/api...
File extensions/renderer/api_event_handler.cc (right):

https://codereview.chromium.org/2722463006/diff/80001/extensions/renderer/api...
extensions/renderer/api_event_handler.cc:141: data->event_data.clear();
On 2017/03/03 22:14:19, lazyboy wrote:
> FYI: If there's a chance that |data->event_data| can be accessed by lines
> 131-137 before this clear()...
> I'd be careful and consider setting event_data aside first using std::swap()
and
> then iterating them on line 130. If not, then ignore.

It shouldn't be possible, but I've added a comment documenting that.

[Devlin, 2017-03-06 19:11:11]

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

[Devlin, 2017-03-06 19:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2722463006/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-06 19:11:56]

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-03-06 19:18:04]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488827471817050,
"parent_rev": "56a39345f227b13c9aed2d62b3b9c99ad43481ad", "commit_rev":
"317a9b943cc6e28bb3f7d0cda4ae9382d84f9288"}

[commit-bot: I haz the power, 2017-03-06 19:18:41]

Description was changed from

==========
[Extensions Bindings] Notify of event unregistration on invalidation

When contexts are invalidated, we consider the events in that context
unregistered and notify the browser accordingly. Hook up an
InvalidateContext() method on APIEventHandler and call it through
blink's WillReleaseScriptContext().

Add associated tests.

BUG=653596
==========

to

==========
[Extensions Bindings] Notify of event unregistration on invalidation

When contexts are invalidated, we consider the events in that context
unregistered and notify the browser accordingly. Hook up an
InvalidateContext() method on APIEventHandler and call it through
blink's WillReleaseScriptContext().

Add associated tests.

BUG=653596

Review-Url: https://codereview.chromium.org/2722463006
Cr-Commit-Position: refs/heads/master@{#454911}
Committed:
https://chromium.googlesource.com/chromium/src/+/317a9b943cc6e28bb3f7d0cda4ae...
==========

[commit-bot: I haz the power, 2017-03-06 19:18:42]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/317a9b943cc6e28bb3f7d0cda4ae...