Move the event attach/detach logic on unload from event.js to

Move the event attach/detach logic on unload from event.js to
event_bindings.cc.

This removes one of the reasons to call into JavaScript on context unload,
which can crash. It's also more robust; it's confusing trying to maintain a
data structure in JavaScript which reflects C++ state, which contributes to
bugs like crbug.com/474718.

Also clean up and formalise the script context invalidation system: CHECK
rather than guarding against multiple invalidations, only invalidate script
context related member variables, add an invalidation observer interface.

BUG=474718, 475536
R=rdevlin.cronin@chromium.org

Committed: https://crrev.com/d99095034d2e88897ae82c8353f3327a3a1d03a5
Cr-Commit-Position: refs/heads/master@{#324933}

[Devlin, 2015-04-10 18:10:40]

Mostly nits, but I wanna take another look to make sure I've fully processed all
of this. :)

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/events_apitest.cc (right):

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:10: using extensions::EventRouter;
Can't we just shove this whole file into the extensions namespace?

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:19:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, EventsAreUnregistered) {
I trust this test fails without the rest of your patch?

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:26: const char* test_extension_name
= "events_are_unregistered";
nit: may as well make this a std::string, since it'll be constructed into one
twice.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:184: // possible because extension can
create and attach events themselves. Very
nit: extensions

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:350: }
For kicks and grins, can we DCHECK(attached_event_names_.empty()) here?

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:19: class EventFilter;
nit: remove EventFilter* here.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:75: Dispatcher* dispatcher_;
DISALLOW_COPY_AND_ASSIGN() (not sure why it was never added)

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

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:159: CHECK(is_valid());
It's a little odd that each subclass is expected to CHECK(is_valid()) here. 
Can't we just CHECK it in NativeHandler, since each subclass is already expected
to call it's super?

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.cc:119: // TODO(kalman): Make ModuleSystem
use AddInvalidateObserver.
Wait, didn't you?

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.cc:125: // sure it's not mutated as we
iterate.
I mean, I guess swap is pretty cheap - but how would it be modified as we run? 
There's no "RemoveInvalidateObserver" (and couldn't really be, since closure
comparison is kinda hard).

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.h:52: void AddInvalidateObserver(const
base::Closure& observer);
nit: maybe Invalidated, or Invalidation?  InvalidateObserver sounds weird.

[Devlin, 2015-04-10 18:25:50]

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/native_...
File extensions/renderer/native_handler.cc (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/native_...
extensions/renderer/native_handler.cc:15: Invalidate();
In retrospect, this won't work.  When a class is destroyed, it loses it's
v-table, so calling Invalidate() here is only ever going to call
NativeHandler::Invalidate(), not, e.g., ModuleSystem::Invalidate().

[not at google - send to devlin, 2015-04-10 18:39:02]

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/events_apitest.cc (right):

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:10: using extensions::EventRouter;
On 2015/04/10 18:10:39, Devlin wrote:
> Can't we just shove this whole file into the extensions namespace?

Sure.

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:19:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, EventsAreUnregistered) {
On 2015/04/10 18:10:39, Devlin wrote:
> I trust this test fails without the rest of your patch?

The test passes without my patch, as it should - this is a refactor, after all.
It would be exceedingly hard to write a test which used to fail, but now passes
- one test would need to look for a memory leak (ugh), another test would need
to set up a corner case that I haven't managed to create myself (only
occasionally crashes IRL).

However, if I take out the logic which doesn't DetachEvent on
ScriptContext::Invalidate, the test does fail.

https://codereview.chromium.org/1074273002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/events_apitest.cc:26: const char* test_extension_name
= "events_are_unregistered";
On 2015/04/10 18:10:39, Devlin wrote:
> nit: may as well make this a std::string, since it'll be constructed into one
> twice.

If you insist.

Plus I like the look of "const char*", vim makes it green.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:184: // possible because extension can
create and attach events themselves. Very
On 2015/04/10 18:10:40, Devlin wrote:
> nit: extensions

Done.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.cc:350: }
On 2015/04/10 18:10:39, Devlin wrote:
> For kicks and grins, can we DCHECK(attached_event_names_.empty()) here?

Done.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:19: class EventFilter;
On 2015/04/10 18:10:40, Devlin wrote:
> nit: remove EventFilter* here.

Done.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:75: Dispatcher* dispatcher_;
On 2015/04/10 18:10:40, Devlin wrote:
> DISALLOW_COPY_AND_ASSIGN() (not sure why it was never added)

Done.

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

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:159: CHECK(is_valid());
On 2015/04/10 18:10:40, Devlin wrote:
> It's a little odd that each subclass is expected to CHECK(is_valid()) here. 
> Can't we just CHECK it in NativeHandler, since each subclass is already
expected
> to call it's super?

Done.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.cc:119: // TODO(kalman): Make ModuleSystem
use AddInvalidateObserver.
On 2015/04/10 18:10:40, Devlin wrote:
> Wait, didn't you?

No I made EventBindings.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.cc:125: // sure it's not mutated as we
iterate.
On 2015/04/10 18:10:40, Devlin wrote:
> I mean, I guess swap is pretty cheap - but how would it be modified as we run?

> There's no "RemoveInvalidateObserver" (and couldn't really be, since closure
> comparison is kinda hard).

I might be missing something... but if an InvalidationObserver calls
AddInvalidationObserver, it would be insane, but it would also be a bug. I
generally have misgivings about running observer callbacks over a live set of
observers. I'll add the neat little DCHECK you suggested though.

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.h:52: void AddInvalidateObserver(const
base::Closure& observer);
On 2015/04/10 18:10:40, Devlin wrote:
> nit: maybe Invalidated, or Invalidation?  InvalidateObserver sounds weird.

Invalidation it is.

[not at google - send to devlin, 2015-04-10 19:21:54]

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/native_...
File extensions/renderer/native_handler.cc (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/native_...
extensions/renderer/native_handler.cc:15: Invalidate();
On 2015/04/10 18:25:50, Devlin wrote:
> In retrospect, this won't work.  When a class is destroyed, it loses it's
> v-table, so calling Invalidate() here is only ever going to call
> NativeHandler::Invalidate(), not, e.g., ModuleSystem::Invalidate().

Fixed, though moot.

[Devlin, 2015-04-10 22:00:24]

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:19: class EventFilter;
On 2015/04/10 18:39:01, kalman wrote:
> On 2015/04/10 18:10:40, Devlin wrote:
> > nit: remove EventFilter* here.
> 
> Done.

The star was meant to encapsulate the EventFilteringInfo, too. :)

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/script_...
extensions/renderer/script_context.cc:125: // sure it's not mutated as we
iterate.
On 2015/04/10 18:39:01, kalman wrote:
> On 2015/04/10 18:10:40, Devlin wrote:
> > I mean, I guess swap is pretty cheap - but how would it be modified as we
run?
> 
> > There's no "RemoveInvalidateObserver" (and couldn't really be, since closure
> > comparison is kinda hard).
> 
> I might be missing something... but if an InvalidationObserver calls
> AddInvalidationObserver, it would be insane, but it would also be a bug. I
> generally have misgivings about running observer callbacks over a live set of
> observers. I'll add the neat little DCHECK you suggested though.

Yeah, that would be a bug if anyone did that - but totally 100% insane for them
to do so, so I like the DCHECK rather than silently pretending it's okay :P

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

https://codereview.chromium.org/1074273002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:352: << "Events cannot be attached during
invalidation";
nit of nits: The error kinda sounds weird to me.  How about instead "All events
must be detached after invalidation" or something?  (Right now, it sounds like
you're ensuring that no event is attached during the course of invalidation).

https://codereview.chromium.org/1074273002/diff/40001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/1074273002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:111: CHECK(!is_valid_) << "ScriptContexts
must be invalidated before destruction";
I'm liking all these CHECKs.  Hopefully it's not already broken to where they
trigger.

https://codereview.chromium.org/1074273002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:121: module_system_->Invalidate();
You're getting a compile error here (Invalidate() is protected on ModuleSystem),
so it might almost be worth doing this in this patch.  Is there a complication?

[not at google - send to devlin, 2015-04-10 22:31:06]

Patchset #5 (id:60002) has been deleted

[not at google - send to devlin, 2015-04-10 22:31:45]

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1074273002/diff/1/extensions/renderer/event_b...
extensions/renderer/event_bindings.h:19: class EventFilter;
On 2015/04/10 22:00:24, Devlin wrote:
> On 2015/04/10 18:39:01, kalman wrote:
> > On 2015/04/10 18:10:40, Devlin wrote:
> > > nit: remove EventFilter* here.
> > 
> > Done.
> 
> The star was meant to encapsulate the EventFilteringInfo, too. :)

Heh. I thought you just forgot to add a footnote.

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

https://codereview.chromium.org/1074273002/diff/40001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:352: << "Events cannot be attached during
invalidation";
On 2015/04/10 22:00:24, Devlin wrote:
> nit of nits: The error kinda sounds weird to me.  How about instead "All
events
> must be detached after invalidation" or something?  (Right now, it sounds like
> you're ensuring that no event is attached during the course of invalidation).

Actually it's always re-entrancy bugs that I am worried about, and why I take
the copy. The check was to guard against that, not DetachEvent accidentally not
clearing |attached_event_names_|.

https://codereview.chromium.org/1074273002/diff/40001/extensions/renderer/scr...
File extensions/renderer/script_context.cc (right):

https://codereview.chromium.org/1074273002/diff/40001/extensions/renderer/scr...
extensions/renderer/script_context.cc:121: module_system_->Invalidate();
On 2015/04/10 22:00:24, Devlin wrote:
> You're getting a compile error here (Invalidate() is protected on
ModuleSystem),
> so it might almost be worth doing this in this patch.  Is there a
complication?

Fixed in person (I meant to friend ModuleSystemTestEnvironment; ScriptContext is
already friend'd).

[not at google - send to devlin, 2015-04-11 00:09:36]

ping

[Devlin, 2015-04-13 16:07:08]

Just nits this round, but you seem to have a good number of tests failing around
the CHECK in ~NativeHandler().  I'm hoping it's just one place that's not
invalidating it so it can be easily added, and we can keep that extra security.

https://codereview.chromium.org/1074273002/diff/90001/chrome/browser/extensio...
File chrome/browser/extensions/events_apitest.cc (right):

https://codereview.chromium.org/1074273002/diff/90001/chrome/browser/extensio...
chrome/browser/extensions/events_apitest.cc:21: EventRouter* event_router =
EventRouter::Get(profile());
nit: Prefer to initialize these closer to their use.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:8: #include <set>
remove #include <set>, <string>

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:27: #include
"extensions/renderer/object_backed_native_handler.h"
remove

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:29: #include "v8/include/v8.h"
remove

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:147: base::Unretained(this)));
If you can, a really brief note about why unretained is safe would be great.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.h:8: #include <set>
#include <string>, too

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/nat...
File extensions/renderer/native_handler.h (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/nat...
extensions/renderer/native_handler.h:33: // Invalidate() will be called on
destruction, if it hasn't already been.
This is not true anymore.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/obj...
File extensions/renderer/object_backed_native_handler.cc (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/obj...
extensions/renderer/object_backed_native_handler.cc:79: CHECK(is_valid());
Or just leave it to the super class to do this CHECK.  Up to you.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
extensions/renderer/script_context.h:16: #include
"extensions/renderer/module_system.h"
do we need to include this, or is a forward decl okay?

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
extensions/renderer/script_context.h:50: // Registers |closure| to be run when
this context is invalidated. Closures
nit: Registers |observer|

[not at google - send to devlin, 2015-04-13 17:08:30]

I've given up on the CHECK(!is_valid()) in the destructor of NativeHandler. I
fixed up as many instances I can in tests, but then I came across a situation
where we had:

std::map<std::string, linked_ptr<NativeHandler>> handlers;
handlers["logging"] = new LoggingNativeHandler();
...
handlers["logging"] = new LoggingNativeHandler();

but of course in a convoluted sequence of operations. I'm not going to find
every instance of this - so I added a comment to ~NativeHandler - but hopefully
soon there won't be any need for invalidate (as I've mentioned) so I can remove
these all anyway).

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
File extensions/renderer/event_bindings.cc (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:8: #include <set>
On 2015/04/13 16:07:07, Devlin wrote:
> remove #include <set>, <string>

Done.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:27: #include
"extensions/renderer/object_backed_native_handler.h"
On 2015/04/13 16:07:07, Devlin wrote:
> remove

... christ, how do you find these things. I didn't even make changes that would
trigger this.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.cc:147: base::Unretained(this)));
On 2015/04/13 16:07:07, Devlin wrote:
> If you can, a really brief note about why unretained is safe would be great.

There are many many calls of RouteFunction. I'll add a general comment to
extensions/renderer/object_backed_native_handler.h.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
File extensions/renderer/event_bindings.h (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/eve...
extensions/renderer/event_bindings.h:8: #include <set>
On 2015/04/13 16:07:07, Devlin wrote:
> #include <string>, too

Done.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/nat...
File extensions/renderer/native_handler.h (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/nat...
extensions/renderer/native_handler.h:33: // Invalidate() will be called on
destruction, if it hasn't already been.
On 2015/04/13 16:07:07, Devlin wrote:
> This is not true anymore.

Heh, unfortunately, it now is.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/obj...
File extensions/renderer/object_backed_native_handler.cc (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/obj...
extensions/renderer/object_backed_native_handler.cc:79: CHECK(is_valid());
On 2015/04/13 16:07:07, Devlin wrote:
> Or just leave it to the super class to do this CHECK.  Up to you.

Yeah good point, not needed here.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
extensions/renderer/script_context.h:16: #include
"extensions/renderer/module_system.h"
On 2015/04/13 16:07:07, Devlin wrote:
> do we need to include this, or is a forward decl okay?

IIRC you can't forward declare things in scoped_ptr, because the implementation
of scoped ptr is in the header file (and .: so is the destructor)?

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
extensions/renderer/script_context.h:50: // Registers |closure| to be run when
this context is invalidated. Closures
On 2015/04/13 16:07:08, Devlin wrote:
> nit: Registers |observer|

Done.

[Devlin, 2015-04-13 17:24:36]

lgtm as long as bots are happy.

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
File extensions/renderer/script_context.h (right):

https://codereview.chromium.org/1074273002/diff/90001/extensions/renderer/scr...
extensions/renderer/script_context.h:16: #include
"extensions/renderer/module_system.h"
On 2015/04/13 17:08:30, kalman wrote:
> On 2015/04/13 16:07:07, Devlin wrote:
> > do we need to include this, or is a forward decl okay?
> 
> IIRC you can't forward declare things in scoped_ptr, because the
implementation
> of scoped ptr is in the header file (and .: so is the destructor)?

You can actually, as long as you don't create/destroy a scoped_ptr with that
class - which usually you don't in a header.  But it looks like we do this on
line 76, so it won't work.

[not at google - send to devlin, 2015-04-13 20:01:12]

I ended up needing to fix the last remaining failed CHECKs in order to fix a
test memory leak. See diff between 5-6 if you like, the changes are non-trivial
and rather unfortunate.

[Devlin, 2015-04-13 20:12:19]

On 2015/04/13 20:01:12, kalman wrote:
> I ended up needing to fix the last remaining failed CHECKs in order to fix a
> test memory leak. See diff between 5-6 if you like, the changes are
non-trivial
> and rather unfortunate.

Yeah, that is unfortunate, but hopefully temporary.  Still LG.

[not at google - send to devlin, 2015-04-13 20:38:11]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-04-13 20:38:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1074273002/#ps130001
(title: "fix lifetime")

[commit-bot: I haz the power, 2015-04-13 20:39:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1074273002/130001

[commit-bot: I haz the power, 2015-04-13 23:02:26]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2015-04-13 23:03:33]

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

[mlamouri (slow - plz ping), 2015-04-14 10:21:30]

A revert of this CL (patchset #7 id:130001) has been created in
https://codereview.chromium.org/1083663004/ by mlamouri@chromium.org.

The reason for reverting is: This is breaking
ExtensionApiTest.EventsAreUnregistered on Win7 Dedubg bots.

See:
https://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds....

[mlamouri (slow - plz ping), 2015-04-14 10:59:23]

On 2015/04/14 at 10:21:30, Mounir Lamouri wrote:
> A revert of this CL (patchset #7 id:130001) has been created in
https://codereview.chromium.org/1083663004/ by mlamouri@chromium.org.
> 
> The reason for reverting is: This is breaking
ExtensionApiTest.EventsAreUnregistered on Win7 Dedubg bots.
> 
> See:
>
https://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds....

The revert did not apply. Someone disabled it on Windows in the meantime.

[mlamouri (slow - plz ping), 2015-04-14 15:27:36]

On 2015/04/14 at 10:59:23, Mounir Lamouri wrote:
> On 2015/04/14 at 10:21:30, Mounir Lamouri wrote:
> > A revert of this CL (patchset #7 id:130001) has been created in
https://codereview.chromium.org/1083663004/ by mlamouri@chromium.org.
> > 
> > The reason for reverting is: This is breaking
ExtensionApiTest.EventsAreUnregistered on Win7 Dedubg bots.
> > 
> > See:
> >
https://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds....
> 
> The revert did not apply. Someone disabled it on Windows in the meantime.

Reverted.

[huangs, 2015-04-14 15:55:17]

kalman@: As a Sheriff I insisted that this patch be reverted owing to consistent
failure of ExtensionApiTest.EventsAreUnregistered on Win7 Dbg:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...

from #37055 on (success on #37073 was owing to disabling the test -- but since
then we've undone the disabling, then reverted the CL).

[not at google - send to devlin, 2015-04-14 16:06:28]

On 2015/04/14 15:55:17, huangs wrote:
> kalman@: As a Sheriff I insisted that this patch be reverted owing to
consistent
> failure of ExtensionApiTest.EventsAreUnregistered on Win7 Dbg:
> 
>
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
> 
> from #37055 on (success on #37073 was owing to disabling the test -- but since
> then we've undone the disabling, then reverted the CL).

Reverting was the right thing to do, thanks.