Tracking push events for lucid sleep

Tracking push events for lucid sleep

Phase 2 of lucid sleep involves allowing GCM-enabled apps and extensions
to wake up the system to process push messages.  This brings up the
problem of knowing when those apps/extensions are done processing the
event so that the system can go back to sleep.

This CL introduces a new class whose responsibility is to monitor
GCM-enabled apps and extensions and delay the system suspend while they
are processing push messages.  Additionally, network requests that are
started while the app/extension is processing a push message are
considered related to that message and can further delay the system
suspend even after the push message itself has been acked by the
extension.

BUG=397328
Committed: https://crrev.com/79788f68a65e9f99eb23c19fa5ba52d1a52996bf
Cr-Commit-Position: refs/heads/master@{#314219}

[Chirantan Ekbote, 2014-12-23 02:37:36]

chirantan@chromium.org changed reviewers:
+ rockot@chromium.org

[Chirantan Ekbote, 2014-12-23 02:37:36]

Hi Ken, this is my rough attempt at using the lazy_keepalive_count for
extensions to determine how long to stay awake for push messages.  The major
problem I'm running into right now is that chrome is always hitting the suspend
delay timeout.  I'm guessing because there are too many "things" that are being
counted for the lazy_keepalive_count.  For example, if an app/extension shows a
notification then its lazy_keepalive_count doesn't appear to drop back to 0
until the event is acknowledged by the user.

So it seems at this point that the lazy_keepalive_count is too coarse and we may
need a more specialized mechanism for tracking push events specifically.  What
do you think?

[Ken Rockot(use gerrit already), 2015-01-05 18:40:55]

Ah, OK. Having a separate count for outstanding GCM events SGTM.

[Chirantan Ekbote, 2015-01-07 22:32:22]

Hi Ken,

Here is a rough version 2.  I ended up using a NotificationService for tracking
the event dispatch.  I know you're not a big fan of NotificationService but
going the Observer route is a little more tricky since DispatchExtensionMessage
is a static function.  If the general idea looks good to you, then I will clean
it up for merging.

There are still a couple of concerns I have with this approach:

- If an app/extension tries to fetch something asynchronously from the network
in response to a push message, it might not have enough time to get everything
it needs.  Ideally I'd like to be able to add the number of outstanding resource
loads to the count of "things" keeping the system awake.  Do you know if there
is some way I can hook into this?

- I think apps with persistent background pages can still receive push messages
(I am checking with zea@ if this is actually the case).  If that's true, then
those apps will never send back an ExtensionHostMsg_EventAck since this message
is only sent by apps with lazy background pages.  One potential way around this
might be to use a new unconditional message that gets routed to the
ProcessManager instead of the ExtensionHost.  Maybe something like
ExtensionHostMsg_MessageAck?

[Ken Rockot(use gerrit already), 2015-01-07 23:14:26]

On 2015/01/07 22:32:22, Chirantan Ekbote wrote:
> Hi Ken,
> 
> Here is a rough version 2.  I ended up using a NotificationService for
tracking
> the event dispatch.  I know you're not a big fan of NotificationService but
> going the Observer route is a little more tricky since
DispatchExtensionMessage
> is a static function.  If the general idea looks good to you, then I will
clean
> it up for merging.
> 
> There are still a couple of concerns I have with this approach:
> 
> - If an app/extension tries to fetch something asynchronously from the network
> in response to a push message, it might not have enough time to get everything
> it needs.  Ideally I'd like to be able to add the number of outstanding
resource
> loads to the count of "things" keeping the system awake.  Do you know if there
> is some way I can hook into this?
> 
> - I think apps with persistent background pages can still receive push
messages
> (I am checking with zea@ if this is actually the case).  If that's true, then
> those apps will never send back an ExtensionHostMsg_EventAck since this
message
> is only sent by apps with lazy background pages.  One potential way around
this
> might be to use a new unconditional message that gets routed to the
> ProcessManager instead of the ExtensionHost.  Maybe something like
> ExtensionHostMsg_MessageAck?

On the second point, your suggestion seems reasonable but I'm not sure we should
do anything to support that use case. We want developers to move away from
persistent background pages and ideally (IMO) they will be removed in the
not-so-distant future.

Off the top of my head, I'm not sure about the right way to track outstanding
resource loads. This could be especially tricky anyway: if waking up the process
leads to other unrelated logic being executed (say, the firing of event handlers
unrelated to GCM) which in turn also trigger resource loads, might
differentiating the source of each resource request introduce excessive
complexity? I suppose perhaps you could wait only on resource loads which were
initiated within a fixed-size time window.

[Chirantan Ekbote, 2015-01-07 23:33:35]

On 2015/01/07 23:14:26, Ken Rockot wrote:
> On 2015/01/07 22:32:22, Chirantan Ekbote wrote:
> > Hi Ken,
> > 
> > Here is a rough version 2.  I ended up using a NotificationService for
> tracking
> > the event dispatch.  I know you're not a big fan of NotificationService but
> > going the Observer route is a little more tricky since
> DispatchExtensionMessage
> > is a static function.  If the general idea looks good to you, then I will
> clean
> > it up for merging.
> > 
> > There are still a couple of concerns I have with this approach:
> > 
> > - If an app/extension tries to fetch something asynchronously from the
network
> > in response to a push message, it might not have enough time to get
everything
> > it needs.  Ideally I'd like to be able to add the number of outstanding
> resource
> > loads to the count of "things" keeping the system awake.  Do you know if
there
> > is some way I can hook into this?
> > 
> > - I think apps with persistent background pages can still receive push
> messages
> > (I am checking with zea@ if this is actually the case).  If that's true,
then
> > those apps will never send back an ExtensionHostMsg_EventAck since this
> message
> > is only sent by apps with lazy background pages.  One potential way around
> this
> > might be to use a new unconditional message that gets routed to the
> > ProcessManager instead of the ExtensionHost.  Maybe something like
> > ExtensionHostMsg_MessageAck?
> 
> On the second point, your suggestion seems reasonable but I'm not sure we
should
> do anything to support that use case. We want developers to move away from
> persistent background pages and ideally (IMO) they will be removed in the
> not-so-distant future.
> 

Leaving it unsupported sgtm.  More reasons for developers to switch over.

> Off the top of my head, I'm not sure about the right way to track outstanding
> resource loads. This could be especially tricky anyway: if waking up the
process
> leads to other unrelated logic being executed (say, the firing of event
handlers
> unrelated to GCM) which in turn also trigger resource loads, might
> differentiating the source of each resource request introduce excessive
> complexity? I suppose perhaps you could wait only on resource loads which were
> initiated within a fixed-size time window.

I was thinking of only waiting for resource loads that are initiated between a
GCM onmessage event and the corresponding ack.  I was thinking I'll go ahead and
land at least this part now now and then do the resource load stuff in a
separate CL.

[Ken Rockot(use gerrit already), 2015-01-08 01:24:02]

I think this would be an OK approach.

You do have a BrowserContext handle in both ExtensionHost and
DispatchEventMessage, so I think you could leverage an existing
BrowserContext-keyed service (like ProcessManager or ExtensionHost perhaps?) to
provide observer hooks, rather than relying on NotificationService. I may be
missing something, but if not, that would be preferable.

https://codereview.chromium.org/823703004/diff/20001/extensions/browser/event...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/20001/extensions/browser/event...
extensions/browser/event_router.cc:144:
content::NotificationService::current()->Notify(
This function runs on the IO thread, but NotificationService lives on the UI
thread.

https://codereview.chromium.org/823703004/diff/20001/extensions/browser/notif...
File extensions/browser/notification_types.h (right):

https://codereview.chromium.org/823703004/diff/20001/extensions/browser/notif...
extensions/browser/notification_types.h:219: // hoding the message id.
nit: "holding" I assume

[Ken Rockot(use gerrit already), 2015-01-08 01:25:58]

https://codereview.chromium.org/823703004/diff/20001/extensions/browser/event...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/20001/extensions/browser/event...
extensions/browser/event_router.cc:144:
content::NotificationService::current()->Notify(
On 2015/01/08 01:24:02, Ken Rockot wrote:
> This function runs on the IO thread, but NotificationService lives on the UI
> thread.

I realize my pronoun may be ambiguous here. What I mean is that
DispatchExtensionMessage runs on the IO thread. You'll have to hop to UI if you
want to touch NotificationService.

[Chirantan Ekbote, 2015-01-08 01:56:07]

On 2015/01/08 01:24:02, Ken Rockot wrote:
> I think this would be an OK approach.
> 
> You do have a BrowserContext handle in both ExtensionHost and
> DispatchEventMessage, so I think you could leverage an existing
> BrowserContext-keyed service (like ProcessManager or ExtensionHost perhaps?)
to
> provide observer hooks, rather than relying on NotificationService. I may be
> missing something, but if not, that would be preferable.
> 

It didn't click in my head that DispatchExtensionMessage has a BrowserContext
handle so using that sounds like a good idea.  And while I was reading through
ProcessManager to figure out how to hook it in, I found OnNetworkRequestStarted
and OnNetworkRequestDone, which look like exactly what I need.  Two birds with
one stone :-)

[Chirantan Ekbote, 2015-01-13 23:02:58]

chirantan@chromium.org changed reviewers:
+ derat@chromium.org, jln@chromium.org, thestig@chromium.org

[Chirantan Ekbote, 2015-01-13 23:02:59]

PTAL.

Ken, please review extensions/.
Dan, please review chromeos/.
Lei, please review chrome/browser/net and chrome/test.
Julien, please review extension_messages.h.

[Daniel Erat, 2015-01-13 23:49:10]

lgtm for c/b/chromeos after comments are addressed (approving now since i'll be
ooo; please find another reviewer if you have any questions)

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/extension_event_observer.cc (right):

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.cc:68: for (Profile*
profile : active_profiles_) {
nit: omit curly brackets

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.cc:253: dark_suspend ?
base::TimeDelta::FromSeconds(1) : base::TimeDelta());
nit: move this to a millisecond constant at the top of the file in an anon
namespace?

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/extension_event_observer.h (right):

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:36: // that arrive from
Google's GCM servers and network requests ininitiated by
s/ininitiated/initiated/

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:69: scoped_ptr<TestApi>
GetApiForTesting();
rename to CreateTestApi to make it clearer that each call makes a new one

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:73: void
ShouldDelaySuspend(bool should_delay);
SetShouldDelaySuspend()? the current name makes it sound like it should return a
bool.

also, the above "to determine" comment is unclear. do you mean "... to control
whether ExtensionEventObserver should delay ..."?

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:111: // Maybe report
readiness to suspend to the PowerManagerClient.
nit: explain what determines whether it's reported or not

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/extension_event_observer_unittest.cc (right):

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer_unittest.cc:48:
~ExtensionEventObserverTest() {
override

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer_unittest.cc:136:
EXPECT_EQ(power_manager_client_->GetNumPendingSuspendReadinessCallbacks(), 1);
here and elsewhere in the file: should be EXPECT_EQ(expected, actual) rather
than (actual, expected)

[Lei Zhang, 2015-01-14 00:05:54]

On 2015/01/13 23:02:59, Chirantan Ekbote wrote:
> Lei, please review chrome/browser/net and chrome/test.

LGTM

[Ken Rockot(use gerrit already), 2015-01-14 20:23:01]

lgtm, not sure where the failures are coming from

[Chirantan Ekbote, 2015-01-14 20:26:05]

On 2015/01/14 20:23:01, Ken Rockot wrote:
> lgtm, not sure where the failures are coming from

The failures are due to
https://chromium.googlesource.com/chromium/src/+/8f04c823c4914de603e1a9d5beaa...
which landed just as I uploaded this patch set.  I have it fixed locally so the
next one should be fine.

[Chirantan Ekbote, 2015-01-15 05:06:36]

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/extension_event_observer.cc (right):

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.cc:68: for (Profile*
profile : active_profiles_) {
On 2015/01/13 23:49:09, Daniel Erat (OOO 1-14 to 1-19) wrote:
> nit: omit curly brackets

Done.

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.cc:253: dark_suspend ?
base::TimeDelta::FromSeconds(1) : base::TimeDelta());
On 2015/01/13 23:49:10, Daniel Erat (OOO 1-14 to 1-19) wrote:
> nit: move this to a millisecond constant at the top of the file in an anon
> namespace?

Done.

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/extension_event_observer.h (right):

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:36: // that arrive from
Google's GCM servers and network requests ininitiated by
On 2015/01/13 23:49:10, Daniel Erat (OOO 1-14 to 1-19) wrote:
> s/ininitiated/initiated/

Done.

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:69: scoped_ptr<TestApi>
GetApiForTesting();
On 2015/01/13 23:49:10, Daniel Erat (OOO 1-14 to 1-19) wrote:
> rename to CreateTestApi to make it clearer that each call makes a new one

Done.

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:73: void
ShouldDelaySuspend(bool should_delay);
On 2015/01/13 23:49:10, Daniel Erat (OOO 1-14 to 1-19) wrote:
> SetShouldDelaySuspend()? the current name makes it sound like it should return
a
> bool.
> 
> also, the above "to determine" comment is unclear. do you mean "... to control
> whether ExtensionEventObserver should delay ..."?

Done.

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer.h:111: // Maybe report
readiness to suspend to the PowerManagerClient.
On 2015/01/13 23:49:10, Daniel Erat (OOO 1-14 to 1-19) wrote:
> nit: explain what determines whether it's reported or not

Done.

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/extension_event_observer_unittest.cc (right):

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer_unittest.cc:48:
~ExtensionEventObserverTest() {
On 2015/01/13 23:49:10, Daniel Erat (OOO 1-14 to 1-19) wrote:
> override

Done.

https://codereview.chromium.org/823703004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/extension_event_observer_unittest.cc:136:
EXPECT_EQ(power_manager_client_->GetNumPendingSuspendReadinessCallbacks(), 1);
On 2015/01/13 23:49:10, Daniel Erat (OOO 1-14 to 1-19) wrote:
> here and elsewhere in the file: should be EXPECT_EQ(expected, actual) rather
> than (actual, expected)

Done.

[jln (very slow on Chromium), 2015-01-15 20:19:11]

https://chromiumcodereview.appspot.com/823703004/diff/80001/extensions/browse...
File extensions/browser/extension_host.cc (right):

https://chromiumcodereview.appspot.com/823703004/diff/80001/extensions/browse...
extensions/browser/extension_host.cc:388:
FOR_EACH_OBSERVER(ExtensionHostObserver, observer_list_,
Would there be a way to validate |message_id| here?

It's very easy to forget that it's "untrusted" down the stack.
hrome/browser/chromeos/power/extension_event_observer.cc doesn't even have a
comment on that.

https://chromiumcodereview.appspot.com/823703004/diff/80001/extensions/browse...
File extensions/browser/extension_host_observer.h (right):

https://chromiumcodereview.appspot.com/823703004/diff/80001/extensions/browse...
extensions/browser/extension_host_observer.h:39: };
DISALLOW_COPY_AND_ASSIGN?

[Chirantan Ekbote, 2015-01-15 21:53:27]

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
extensions/browser/extension_host.cc:388:
FOR_EACH_OBSERVER(ExtensionHostObserver, observer_list_,
On 2015/01/15 20:19:11, jln wrote:
> Would there be a way to validate |message_id| here?
> 
> It's very easy to forget that it's "untrusted" down the stack.
> hrome/browser/chromeos/power/extension_event_observer.cc doesn't even have a
> comment on that.

I'm a little unclear on what you mean by validating |message_id|.  Is the
concern that a compromised renderer could start sending back bogus message ids? 
One thing I can think of is to keep a list of all unacked message ids dispatched
for the current ExtensionHost and only call the observer methods if the acked
message id exists in that list.  However, a compromised renderer could still get
lucky and send back an id that _is_ in the list and I'm not sure how we could
detect that.

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
File extensions/browser/extension_host_observer.h (right):

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
extensions/browser/extension_host_observer.h:39: };
On 2015/01/15 20:19:11, jln wrote:
> DISALLOW_COPY_AND_ASSIGN?

Acknowledged.

[Chirantan Ekbote, 2015-01-20 23:55:43]

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
extensions/browser/extension_host.cc:388:
FOR_EACH_OBSERVER(ExtensionHostObserver, observer_list_,
On 2015/01/15 21:53:27, Chirantan Ekbote wrote:
> On 2015/01/15 20:19:11, jln wrote:
> > Would there be a way to validate |message_id| here?
> > 
> > It's very easy to forget that it's "untrusted" down the stack.
> > hrome/browser/chromeos/power/extension_event_observer.cc doesn't even have a
> > comment on that.
> 
> I'm a little unclear on what you mean by validating |message_id|.  Is the
> concern that a compromised renderer could start sending back bogus message
ids? 
> One thing I can think of is to keep a list of all unacked message ids
dispatched
> for the current ExtensionHost and only call the observer methods if the acked
> message id exists in that list.  However, a compromised renderer could still
get
> lucky and send back an id that _is_ in the list and I'm not sure how we could
> detect that.

Ping?  I want to land this soon and I don't want to make a change just for the
sake of making a change.  I'd like to understand your concern so that I can
properly address it.

[jln (very slow on Chromium), 2015-01-22 00:16:35]

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/80001/extensions/browser/exten...
extensions/browser/extension_host.cc:388:
FOR_EACH_OBSERVER(ExtensionHostObserver, observer_list_,
On 2015/01/15 21:53:27, Chirantan Ekbote wrote:
> On 2015/01/15 20:19:11, jln wrote:
> > Would there be a way to validate |message_id| here?
> > 
> > It's very easy to forget that it's "untrusted" down the stack.
> > hrome/browser/chromeos/power/extension_event_observer.cc doesn't even have a
> > comment on that.
> 
> I'm a little unclear on what you mean by validating |message_id|.  Is the
> concern that a compromised renderer could start sending back bogus message
ids? 
> One thing I can think of is to keep a list of all unacked message ids
dispatched
> for the current ExtensionHost and only call the observer methods if the acked
> message id exists in that list.  However, a compromised renderer could still
get
> lucky and send back an id that _is_ in the list and I'm not sure how we could
> detect that.

As you noted, a bogus renderer could start sending back bogus message ids. This
could cause many different types of security issues, from memory corruption to
more subtle, logical bugs.

It's not really possible for a security reviewing to go down 20 calls down the
stack to figure out why this is safe. Even if it was possible, it wouldn't be
robust as this could change at any time and it would be a time sync every time
someone looks at this.

Could you explain what the security model looks like (ideally as code comments)?
- Why is there no risk of memory corruption? Perhaps your suggestion of keeping
a list of unacked message ids is the right solution here. Or is there another
way to convince ourselves (without going 20 functions deeps) that sending a
random message_id won't cause trouble?

- What is the risk if a compromised renderer sends back a message_id that is in
the list? Can this interfere with other renderers? I don't have enough knowledge
of this area and I'm relying on your to explain this. Perhaps the risk is
acceptable, but we need to make that decision and document it.

[Chirantan Ekbote, 2015-01-26 22:08:25]

ptal.  I added validation for the message id and some comments explaining the
potential risk.

[jln (very slow on Chromium), 2015-01-28 01:17:38]

jln@chromium.org changed reviewers:
- rockot@chromium.org

[jln (very slow on Chromium), 2015-01-28 01:18:28]

This looks good, thanks!

Could an extension owner (rockot?) please take another look?

https://chromiumcodereview.appspot.com/823703004/diff/100001/extensions/brows...
File extensions/browser/extension_host.cc (right):

https://chromiumcodereview.appspot.com/823703004/diff/100001/extensions/brows...
extensions/browser/extension_host.cc:397: }
We should kill the renderer in the else {} clause here.

Could you please call BadMessageReceived()?
(content/public/browser/browser_message_filter.h)

[Chirantan Ekbote, 2015-01-28 22:11:52]

https://chromiumcodereview.appspot.com/823703004/diff/100001/extensions/brows...
File extensions/browser/extension_host.cc (right):

https://chromiumcodereview.appspot.com/823703004/diff/100001/extensions/brows...
extensions/browser/extension_host.cc:397: }
On 2015/01/28 01:18:27, jln wrote:
> We should kill the renderer in the else {} clause here.
> 
> Could you please call BadMessageReceived()?
> (content/public/browser/browser_message_filter.h)

Done.

[jln (very slow on Chromium), 2015-01-28 22:18:53]

extensions/common/extension_messages.h lgtm

I would love if rockot could take another quick look.

[Ken Rockot(use gerrit already), 2015-02-02 22:49:45]

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

[Ken Rockot(use gerrit already), 2015-02-02 22:49:46]

lgtm

somehow i was dropped from the reviewer list and this CL disappeared from my
view altogether. sorry for the delay!

[Chirantan Ekbote, 2015-02-02 23:09:18]

The CQ bit was checked by chirantan@chromium.org

[commit-bot: I haz the power, 2015-02-02 23:10:44]

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

[commit-bot: I haz the power, 2015-02-02 23:57:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-02 23:58:59]

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

[not at google - send to devlin, 2015-02-26 23:24:31]

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

[not at google - send to devlin, 2015-02-26 23:24:32]

This CL was brought to my attention, sorry for this, but I believe it's caused
crbug.com/462026.

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
extensions/browser/event_router.cc:126: g_message_id_lock.Get().Release();
It looks like this method is only called on the IO thread, why the lock? I'd
rather assert that we're on the IO thread than take chances like this.

Also it would be a little tidier to make g_extension_message_id a static local
variable.

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
extensions/browser/event_router.cc:135: args.Set(3, new
base::FundamentalValue(message_id));
The reason these other arguments are sent in a ListValue is to pass them to JS.
The message ID doesn't need to be passed to JS, so it should be part of the
ExtensionMsg_MessageInvoke method -

However, that's conflating the generic message invocation with dispatching
events (which dispatcher.cc already does, but I think this complicates it
further). We should have a separate IPC for dispatching an event.

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
extensions/browser/extension_host.cc:221: unacked_messages_.insert(message_id);
This method is called on the IO thread, but below unacked_messages_ is accessed
on the UI thread (I believe that WebContentsObserver dispatches its IPC on the
UI thread).

https://codereview.chromium.org/823703004/diff/100001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:506: args.GetInteger(3, &message_id);
Can you assert that there actually is a message ID? (or better, make a new IPC)

[not at google - send to devlin, 2015-02-26 23:32:36]

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/exte...
extensions/browser/extension_host.cc:401:
render_process_host()->ReceivedBadMessage();
Also as I mentioned in the bug, please UMA this.

[Chirantan Ekbote, 2015-02-26 23:38:57]

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
extensions/browser/event_router.cc:126: g_message_id_lock.Get().Release();
On 2015/02/26 23:24:32, kalman wrote:
> It looks like this method is only called on the IO thread, why the lock? I'd
> rather assert that we're on the IO thread than take chances like this.
> 

This method is called by EventRouter::DispatchEvent, which can be called on any
thread.

> Also it would be a little tidier to make g_extension_message_id a static local
> variable.

I was actually planning on making it an AtomicSequenceNumber so that we can get
rid of all the locking.  So yes, I will clean this up.

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
extensions/browser/event_router.cc:135: args.Set(3, new
base::FundamentalValue(message_id));
On 2015/02/26 23:24:32, kalman wrote:
> The reason these other arguments are sent in a ListValue is to pass them to
JS.
> The message ID doesn't need to be passed to JS, so it should be part of the
> ExtensionMsg_MessageInvoke method -
> 
> However, that's conflating the generic message invocation with dispatching
> events (which dispatcher.cc already does, but I think this complicates it
> further). We should have a separate IPC for dispatching an event.

I'm not very familiar with the extension system so I don't fully understand what
you mean here.  It sounds like you want to use a separate IPC message for
dispatching events and an ExtensionMsg_MessageInvoke for everything else that
MessageInvoke is currently being used for.  Is that correct?

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
extensions/browser/extension_host.cc:221: unacked_messages_.insert(message_id);
On 2015/02/26 23:24:32, kalman wrote:
> This method is called on the IO thread, but below unacked_messages_ is
accessed
> on the UI thread (I believe that WebContentsObserver dispatches its IPC on the
> UI thread).

This method is called by NotifyEventDispatched in event_router.cc.  The first
thing that function does is make sure it is running on the UI thread.

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/exte...
extensions/browser/extension_host.cc:401:
render_process_host()->ReceivedBadMessage();
On 2015/02/26 23:32:36, kalman wrote:
> Also as I mentioned in the bug, please UMA this.

Acknowledged.

[not at google - send to devlin, 2015-02-26 23:52:33]

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
extensions/browser/event_router.cc:135: args.Set(3, new
base::FundamentalValue(message_id));
On 2015/02/26 23:38:57, Chirantan Ekbote wrote:
> On 2015/02/26 23:24:32, kalman wrote:
> > The reason these other arguments are sent in a ListValue is to pass them to
> JS.
> > The message ID doesn't need to be passed to JS, so it should be part of the
> > ExtensionMsg_MessageInvoke method -
> > 
> > However, that's conflating the generic message invocation with dispatching
> > events (which dispatcher.cc already does, but I think this complicates it
> > further). We should have a separate IPC for dispatching an event.
> 
> I'm not very familiar with the extension system so I don't fully understand
what
> you mean here.  It sounds like you want to use a separate IPC message for
> dispatching events and an ExtensionMsg_MessageInvoke for everything else that
> MessageInvoke is currently being used for.  Is that correct?

Yes that's correct.

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
extensions/browser/extension_host.cc:221: unacked_messages_.insert(message_id);
On 2015/02/26 23:38:57, Chirantan Ekbote wrote:
> On 2015/02/26 23:24:32, kalman wrote:
> > This method is called on the IO thread, but below unacked_messages_ is
> accessed
> > on the UI thread (I believe that WebContentsObserver dispatches its IPC on
the
> > UI thread).
> 
> This method is called by NotifyEventDispatched in event_router.cc.  The first
> thing that function does is make sure it is running on the UI thread.

Ah, you're right. NotifyEventDispatched tricked me.

In that case, in crazy-land (but perhaps this power-on case is crazy land) this
is racy: it's possible for the message to be sent and acked all on the IO
thread, meaning this code may never get hit, and hence the message ID not being
found.

[Chirantan Ekbote, 2015-02-27 00:15:00]

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/even...
extensions/browser/event_router.cc:135: args.Set(3, new
base::FundamentalValue(message_id));
On 2015/02/26 23:52:33, kalman wrote:
> On 2015/02/26 23:38:57, Chirantan Ekbote wrote:
> > On 2015/02/26 23:24:32, kalman wrote:
> > > The reason these other arguments are sent in a ListValue is to pass them
to
> > JS.
> > > The message ID doesn't need to be passed to JS, so it should be part of
the
> > > ExtensionMsg_MessageInvoke method -
> > > 
> > > However, that's conflating the generic message invocation with dispatching
> > > events (which dispatcher.cc already does, but I think this complicates it
> > > further). We should have a separate IPC for dispatching an event.
> > 
> > I'm not very familiar with the extension system so I don't fully understand
> what
> > you mean here.  It sounds like you want to use a separate IPC message for
> > dispatching events and an ExtensionMsg_MessageInvoke for everything else
that
> > MessageInvoke is currently being used for.  Is that correct?
> 
> Yes that's correct.

Ok, I'll make a new CL for this change.

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
extensions/browser/extension_host.cc:221: unacked_messages_.insert(message_id);
On 2015/02/26 23:52:33, kalman wrote:
> On 2015/02/26 23:38:57, Chirantan Ekbote wrote:
> > On 2015/02/26 23:24:32, kalman wrote:
> > > This method is called on the IO thread, but below unacked_messages_ is
> > accessed
> > > on the UI thread (I believe that WebContentsObserver dispatches its IPC on
> the
> > > UI thread).
> > 
> > This method is called by NotifyEventDispatched in event_router.cc.  The
first
> > thing that function does is make sure it is running on the UI thread.
> 
> Ah, you're right. NotifyEventDispatched tricked me.
> 
> In that case, in crazy-land (but perhaps this power-on case is crazy land)
this
> is racy: it's possible for the message to be sent and acked all on the IO
> thread, meaning this code may never get hit, and hence the message ID not
being
> found.

I don't understand this.  Both this function and ExtensionHost::OnEventAck have
to be called on the UI thread because the ExtensionHost object lives on the UI
thread.  How can the message be sent and acked all on the IO thread?

[not at google - send to devlin, 2015-02-27 01:00:50]

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/100001/extensions/browser/exte...
extensions/browser/extension_host.cc:221: unacked_messages_.insert(message_id);
On 2015/02/27 00:15:00, Chirantan Ekbote wrote:
> On 2015/02/26 23:52:33, kalman wrote:
> > On 2015/02/26 23:38:57, Chirantan Ekbote wrote:
> > > On 2015/02/26 23:24:32, kalman wrote:
> > > > This method is called on the IO thread, but below unacked_messages_ is
> > > accessed
> > > > on the UI thread (I believe that WebContentsObserver dispatches its IPC
on
> > the
> > > > UI thread).
> > > 
> > > This method is called by NotifyEventDispatched in event_router.cc.  The
> first
> > > thing that function does is make sure it is running on the UI thread.
> > 
> > Ah, you're right. NotifyEventDispatched tricked me.
> > 
> > In that case, in crazy-land (but perhaps this power-on case is crazy land)
> this
> > is racy: it's possible for the message to be sent and acked all on the IO
> > thread, meaning this code may never get hit, and hence the message ID not
> being
> > found.
> 
> I don't understand this.  Both this function and ExtensionHost::OnEventAck
have
> to be called on the UI thread because the ExtensionHost object lives on the UI
> thread.  How can the message be sent and acked all on the IO thread?

Yep you're right, sorry.

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
extensions/browser/event_router.cc:80:
ProcessManager::Get(context)->GetBackgroundHostForExtension(extension_id);
This looks like it could be a problem. A message was definitely sent earlier,
and it went somewhere - so if there's not background page right now you won't
register the message ID.

At face value this would appear to be a bug, but dispatcher.cc does ack to the
background page's view, and only if there's a persistent background page - so it
should exist. However, if it doesn't, this line condemns it to die in the near
future.

The way to fix this is to move the message ID handling inside eventrouter, I
think.

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/exte...
File extensions/browser/extension_host.cc (right):

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/exte...
extensions/browser/extension_host.cc:387: router->OnEventAck(browser_context_,
extension_id());
Actually - it seems like this event ID tracking should be done inside
EventRouter, not ExtensionHost.

[not at google - send to devlin, 2015-02-27 01:09:42]

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
extensions/browser/event_router.cc:80:
ProcessManager::Get(context)->GetBackgroundHostForExtension(extension_id);
On 2015/02/27 01:00:50, kalman wrote:
> This looks like it could be a problem. A message was definitely sent earlier,
> and it went somewhere - so if there's not background page right now you won't
> register the message ID.
> 
> At face value this would appear to be a bug, but dispatcher.cc does ack to the
> background page's view, and only if there's a persistent background page - so
it
> should exist. However, if it doesn't, this line condemns it to die in the near
> future.
> 
> The way to fix this is to move the message ID handling inside eventrouter, I
> think.

s/persistent/event/

it is getting late in the day.

[Chirantan Ekbote, 2015-03-02 22:27:31]

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
extensions/browser/event_router.cc:80:
ProcessManager::Get(context)->GetBackgroundHostForExtension(extension_id);
On 2015/02/27 01:00:50, kalman wrote:
> This looks like it could be a problem. A message was definitely sent earlier,
> and it went somewhere - so if there's not background page right now you won't
> register the message ID.
> 

If this is the case then isn't it a bigger concern that an IPC message was sent
to an extension that doesn't have a corresponding ExtensionHost?  Under what
circumstances would this be a valid state?  Maybe this should be a DCHECK or a
CHECK rather than an if statement.

> At face value this would appear to be a bug, but dispatcher.cc does ack to the
> background page's view, and only if there's a persistent background page - so
it
> should exist. However, if it doesn't, this line condemns it to die in the near
> future.
> 
> The way to fix this is to move the message ID handling inside eventrouter, I
> think.

I'm a little wary about doing this since we would need to post a task to the UI
thread to access the EventRouter for the profile where the event is being
dispatched, which adds more asynchronous steps to this process.  Even after
that, we would be invoking the observers on every event dispatch.  The nice
things about the way things work now is that observers figure out whether they
care about a particular ExtensionHost when it is created and are then only
notified when an event for that ExtensionHost is dispatched.

[not at google - send to devlin, 2015-03-02 22:38:27]

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
File extensions/browser/event_router.cc (right):

https://codereview.chromium.org/823703004/diff/120001/extensions/browser/even...
extensions/browser/event_router.cc:80:
ProcessManager::Get(context)->GetBackgroundHostForExtension(extension_id);
> If this is the case then isn't it a bigger concern that an IPC message was
sent
> to an extension that doesn't have a corresponding ExtensionHost?

Correction: corresponding *background* hosts. There are many types of
ExtensionHosts, including background pages. In theory there should always be a
background host active if any other ExtensionHost is active, but I'm not aware
of that being an official invariant.

The odd part about all of this is that a IPC is sent somewhere, then the
notification is sent to the background page.

> I'm a little wary about doing this since we would need to post a task to the
UI
> thread to access the EventRouter for the profile where the event is being
> dispatched, which adds more asynchronous steps to this process.  Even after
> that, we would be invoking the observers on every event dispatch.  The nice
> things about the way things work now is that observers figure out whether they
> care about a particular ExtensionHost when it is created and are then only
> notified when an event for that ExtensionHost is dispatched.

I've paged out this code over the weekend, but I find it weird that an IPC is
sent from one place, then the Ack for that IPC handled in the other. What is the
problem with things being asynchronous?