Don't stop and restart GCM when extension/app is being updated

Don't stop and restart GCM when extension/app is being updated

BUG=384010
TEST=new tests added

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277865

[not at google - send to devlin, 2014-06-12 21:04:52]

I'm going to rubberstamp lgtm this now. it would be nice if this were in the
chrome/browser/extesnsions/api/gcm directory so that OWNERS covers it.

[fgorski, 2014-06-12 21:38:33]

mostly looks good. A couple of comments though.

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler.cc:112:
RemoveAppHandler(extension->id());
If we are unlucky enough to get a message for the extension/app while it is
unloaded for that reason. What happens? Do we need to update the code
dispatching the message, or would you rather hold on to it until the extension
is added back?

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:167: virtual
void AddAppHandler(const std::string& app_id) OVERRIDE {
There is a DCHECK for not adding duplicates in GCMDriver::AddAppHandler, so what
you really want to test for is that you never enter this method, rather than the
old_size is different than new size.

I looked at GCMDriver::RemoveAppHandler and it does a different set of checks.
Your code makes more sense in that case, do you think we could update the
production code to check that the handler is registered first?

[jianli, 2014-06-12 21:48:31]

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler.cc:112:
RemoveAppHandler(extension->id());
On 2014/06/12 21:38:32, fgorski wrote:
> If we are unlucky enough to get a message for the extension/app while it is
> unloaded for that reason. What happens? Do we need to update the code
> dispatching the message, or would you rather hold on to it until the extension
> is added back?

We hit DCHECK_EQ(LOADING, state_) in GCMClientImpl::OnLoadCompleted due to that
GCMClient is started and then stopped immediately. This underlying problem is
fixed by https://codereview.chromium.org/327263003/.

In addition to this fix, I think we should not stop and restart the GCM when the
extension/app is being updated.

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:167: virtual
void AddAppHandler(const std::string& app_id) OVERRIDE {
On 2014/06/12 21:38:32, fgorski wrote:
> There is a DCHECK for not adding duplicates in GCMDriver::AddAppHandler, so
what
> you really want to test for is that you never enter this method, rather than
the
> old_size is different than new size.

Then it will get caught by the DCHECK assert. Here I am just adding an extra
check.

> 
> I looked at GCMDriver::RemoveAppHandler and it does a different set of checks.
> Your code makes more sense in that case, do you think we could update the
> production code to check that the handler is registered first?

Not sure I understand "update the
> production code to check that the handler is registered first". Could you
please explain it in more details? Thanks.

[fgorski, 2014-06-13 15:20:21]

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler.cc:112:
RemoveAppHandler(extension->id());
On 2014/06/12 21:48:30, jianli wrote:
> On 2014/06/12 21:38:32, fgorski wrote:
> > If we are unlucky enough to get a message for the extension/app while it is
> > unloaded for that reason. What happens? Do we need to update the code
> > dispatching the message, or would you rather hold on to it until the
extension
> > is added back?
> 
> We hit DCHECK_EQ(LOADING, state_) in GCMClientImpl::OnLoadCompleted due to
that
> GCMClient is started and then stopped immediately. This underlying problem is
> fixed by https://codereview.chromium.org/327263003/.
> 
> In addition to this fix, I think we should not stop and restart the GCM when
the
> extension/app is being updated.

I am with you on not restarting GCM connection.

What I mean by "what happens?" is: "what happens to the message?"
Will it get delivered to the app/ext or not?

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:167: virtual
void AddAppHandler(const std::string& app_id) OVERRIDE {
On 2014/06/12 21:48:31, jianli wrote:
> On 2014/06/12 21:38:32, fgorski wrote:
> > There is a DCHECK for not adding duplicates in GCMDriver::AddAppHandler, so
> what
> > you really want to test for is that you never enter this method, rather than
> the
> > old_size is different than new size.
> 
> Then it will get caught by the DCHECK assert. Here I am just adding an extra
> check.
> 
> > 
> > I looked at GCMDriver::RemoveAppHandler and it does a different set of
checks.
> > Your code makes more sense in that case, do you think we could update the
> > production code to check that the handler is registered first?
> 
> Not sure I understand "update the
> > production code to check that the handler is registered first". Could you
> please explain it in more details? Thanks.
> 

Let's discuss it today.

[jianli, 2014-06-17 00:41:57]

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/335693003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_gcm_app_handler.cc:112:
RemoveAppHandler(extension->id());
On 2014/06/13 15:20:21, fgorski wrote:
> On 2014/06/12 21:48:30, jianli wrote:
> > On 2014/06/12 21:38:32, fgorski wrote:
> > > If we are unlucky enough to get a message for the extension/app while it
is
> > > unloaded for that reason. What happens? Do we need to update the code
> > > dispatching the message, or would you rather hold on to it until the
> extension
> > > is added back?
> > 
> > We hit DCHECK_EQ(LOADING, state_) in GCMClientImpl::OnLoadCompleted due to
> that
> > GCMClient is started and then stopped immediately. This underlying problem
is
> > fixed by https://codereview.chromium.org/327263003/.
> > 
> > In addition to this fix, I think we should not stop and restart the GCM when
> the
> > extension/app is being updated.
> 
> I am with you on not restarting GCM connection.
> 
> What I mean by "what happens?" is: "what happens to the message?"
> Will it get delivered to the app/ext or not?
> 

The message routing will not be interrupted since unload and reloading are
happening in a single function.

I've found out a problem with previous approach. I've updated the code to
address this.

[fgorski, 2014-06-17 03:36:21]

lgtm

and a +1 to the permission removed test. Really thought through!

[jianli, 2014-06-17 17:15:11]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:16:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/335693003/40001

[commit-bot: I haz the power, 2014-06-17 21:19:54]

Change committed as 277865