Add a way to kill apps without killing windows

Add a way to kill apps without killing windows

This change allows apps to remember the app state when handling onSuspend event.

Add a new method ProcessManager::ReleaseLazyKeepaliveCountForView to decrement the lazy keepalive count without killing AppWindow.
Rename ProcessManager::IncrementLazyKeepaliveCountForView to AcquireLazyKeepaliveCountForView.
Close remaining views in CloseLazyBackgroundPageNow().

Before this change, things happen in this order:
 1. All AppWindows are destroyed
 2. Keepalive count becomes 0
 3. chrome.runtime.onSuspend is invoked
 4. The background page is unloaded.

After this change:
 1. All AppWindows have released keepalive. (by explicitly releasing it or being destroyed)
 2. Keepalive count becomes 0
 3. chrome.runtime.onSuspend is invoked
 4. Remaining AppWindows are destroyed
 5. The background page is unloaded.

BUG=416999
Committed: https://crrev.com/5bca2278b8ecb8ae3b933807d3cb9a207616127c
Cr-Commit-Position: refs/heads/master@{#302584}

[hashimoto, 2014-10-31 05:47:41]

Patchset #1 (id:1) has been deleted

[hashimoto, 2014-10-31 05:47:45]

Patchset #1 (id:20001) has been deleted

[hashimoto, 2014-10-31 05:47:50]

Patchset #1 (id:40001) has been deleted

[hashimoto, 2014-10-31 05:47:54]

Patchset #1 (id:60001) has been deleted

[hashimoto, 2014-10-31 06:01:49]

hashimoto@chromium.org changed reviewers:
+ scheib@chromium.org

[hashimoto, 2014-10-31 06:01:50]

Hi Vincent,

In an offline chat, we've discussed the idea to add a way to unload the
background page before the keepalive count reaches 0, with introducing a new
concept launch_id to avoid possible race conditions.
However, when trying to add launch_id, I found some difficulty especially around
ExtensionHost::OnInrementLazyKeepaliveCount/OnDecrementLazyKeepaliveCount.

So I came up with a new idea to add a way to decrement the keepalive count
without killing AppWindow and I think this should result in a code not too
complicated.
How do you find this patch?

[scheib, 2014-10-31 19:49:06]

Thanks, this is a good patch and most comments are small nits, however I'm
concerned I don't understand the logic in
ProcessManager::ClearBackgroundPageData, and we must ensure that's very clear
and correct.

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:195: struct
ProcessManager::ExtensionRenderViewData {
Probably comment the struct. All classes have summary comments by style guide,
and it's seems good practice for structs as well.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:207: return view_type != VIEW_TYPE_INVALID
&&
Consider a switch(view_type) so that there will be a compiler error if the any
new ViteType enumerations are created.

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:960: if (GetExtensionID(it->first) ==
extension_id && it->second.has_keepalive)
If this is only run when has_keepalive is true,
then inside AcquireLazyKeepaliveCountForView the
logic "if (data->CanKeepalive() && !data->has_keepalive)"
will always be false, and the keep alive count will fail to
be incremented!

Under what circumstance will we have a view that doesn't have a keepalive?
Previously we incremented for all views, I suppose I'm not seeing why we don't
continue to do so. If there's a reason, please explain it in comments.

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.h:80: // If the view isn't keeping the lazy
background page alive, increments the
These can be private.

[hashimoto, 2014-11-03 15:57:58]

Patchset #2 (id:100001) has been deleted

[hashimoto, 2014-11-03 16:00:22]

Patchset #2 (id:120001) has been deleted

[hashimoto, 2014-11-03 16:02:26]

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:195: struct
ProcessManager::ExtensionRenderViewData {
On 2014/10/31 19:49:05, scheib wrote:
> Probably comment the struct. All classes have summary comments by style guide,
> and it's seems good practice for structs as well.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments

Done.

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:207: return view_type != VIEW_TYPE_INVALID
&&
On 2014/10/31 19:49:05, scheib wrote:
> Consider a switch(view_type) so that there will be a compiler error if the any
> new ViteType enumerations are created.

Done.

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:960: if (GetExtensionID(it->first) ==
extension_id && it->second.has_keepalive)
On 2014/10/31 19:49:05, scheib wrote:
> If this is only run when has_keepalive is true,
> then inside AcquireLazyKeepaliveCountForView the
> logic "if (data->CanKeepalive() && !data->has_keepalive)"
> will always be false, and the keep alive count will fail to
> be incremented!
Thanks!
You're right.
I was about to break this logic completely.
Fixed to do the thing apparently needed by the comment.

BTW, this logic was added in https://codereview.chromium.org/10310005 against a
race bug, in a day when CloseLazyBackgroundPageNow() was not checking the close
sequence ID.
However, since we are checking the close sequence ID now, this logic seems
unneeded to me.
Maybe it's better to just remove this function at all?
> 
> Under what circumstance will we have a view that doesn't have a keepalive?
> Previously we incremented for all views, I suppose I'm not seeing why we don't
> continue to do so. If there's a reason, please explain it in comments. 
As written in the reply to your comment in the header, Athena needs to decrement
the keepalive count when its AppWindow becomes inactive (and needs to increment
the count when being active).

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.h (right):

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.h:80: // If the view isn't keeping the lazy
background page alive, increments the
On 2014/10/31 19:49:06, scheib wrote:
> These can be private.

Sorry for not clearly describing my intention.
I want these methods to be public as Athena's resource management code (namely,
athena::AppActivity::SetCurrentState()) needs to use them to decrement the
keepalive count when the app becomes inactive, and increment again when getting
active.

[scheib, 2014-11-03 17:48:28]

lgtm, though try bots find a compile error given the switch rewrite:

e:\b\build\slave\win\build\src\extensions\browser\process_manager.cc(224) :error
C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\extensions\browser\process_manager.cc(224) :
warning C4715:
'extensions::ProcessManager::ExtensionRenderViewData::CanKeepalive' : not all
control paths return a value

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:960: if (GetExtensionID(it->first) ==
extension_id && it->second.has_keepalive)
On 2014/11/03 16:02:26, hashimoto wrote:
> On 2014/10/31 19:49:05, scheib wrote:
> > If this is only run when has_keepalive is true,
> > then inside AcquireLazyKeepaliveCountForView the
> > logic "if (data->CanKeepalive() && !data->has_keepalive)"
> > will always be false, and the keep alive count will fail to
> > be incremented!
> Thanks!
> You're right.
> I was about to break this logic completely.
> Fixed to do the thing apparently needed by the comment.
> 
> BTW, this logic was added in https://codereview.chromium.org/10310005 against
a
> race bug, in a day when CloseLazyBackgroundPageNow() was not checking the
close
> sequence ID.
> However, since we are checking the close sequence ID now, this logic seems
> unneeded to me.
> Maybe it's better to just remove this function at all?
> > 
> > Under what circumstance will we have a view that doesn't have a keepalive?
> > Previously we incremented for all views, I suppose I'm not seeing why we
don't
> > continue to do so. If there's a reason, please explain it in comments. 
> As written in the reply to your comment in the header, Athena needs to
decrement
> the keepalive count when its AppWindow becomes inactive (and needs to
increment
> the count when being active).

It is hard to be certain that this method is not needed. I suspect the
background_page_data_.erase() is needed to reset values to be prepared for when
a background page is started again.

This logic with views reference count is too indirect. I suggest you land this
patch as is, and if you're motivated to refactor then we create an issue, add
DCHECKs, run tests, and commit to see if any assertions are revealed on
developer machines. If it all runs through without requiring the
IncrementLazyKeepaliveCount for an extension that just had
ClearBackgroundPageData called then we should remove this code block.

[hashimoto, 2014-11-04 05:47:34]

Fixed the win compile error by adding return false with NOTREACHED().

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/689833005/diff/80001/extensions/browser/proce...
extensions/browser/process_manager.cc:960: if (GetExtensionID(it->first) ==
extension_id && it->second.has_keepalive)
On 2014/11/03 17:48:28, scheib wrote:
> On 2014/11/03 16:02:26, hashimoto wrote:
> > On 2014/10/31 19:49:05, scheib wrote:
> > > If this is only run when has_keepalive is true,
> > > then inside AcquireLazyKeepaliveCountForView the
> > > logic "if (data->CanKeepalive() && !data->has_keepalive)"
> > > will always be false, and the keep alive count will fail to
> > > be incremented!
> > Thanks!
> > You're right.
> > I was about to break this logic completely.
> > Fixed to do the thing apparently needed by the comment.
> > 
> > BTW, this logic was added in https://codereview.chromium.org/10310005
against
> a
> > race bug, in a day when CloseLazyBackgroundPageNow() was not checking the
> close
> > sequence ID.
> > However, since we are checking the close sequence ID now, this logic seems
> > unneeded to me.
> > Maybe it's better to just remove this function at all?
> > > 
> > > Under what circumstance will we have a view that doesn't have a keepalive?
> > > Previously we incremented for all views, I suppose I'm not seeing why we
> don't
> > > continue to do so. If there's a reason, please explain it in comments. 
> > As written in the reply to your comment in the header, Athena needs to
> decrement
> > the keepalive count when its AppWindow becomes inactive (and needs to
> increment
> > the count when being active).
> 
> It is hard to be certain that this method is not needed. I suspect the
> background_page_data_.erase() is needed to reset values to be prepared for
when
> a background page is started again.
> 
> This logic with views reference count is too indirect. I suggest you land this
> patch as is, and if you're motivated to refactor then we create an issue, add
> DCHECKs, run tests, and commit to see if any assertions are revealed on
> developer machines. If it all runs through without requiring the
> IncrementLazyKeepaliveCount for an extension that just had
> ClearBackgroundPageData called then we should remove this code block.

Sounds good.
Filed crbug.com/429959.

[hashimoto, 2014-11-04 05:47:50]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-11-04 05:48:34]

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

[commit-bot: I haz the power, 2014-11-04 07:35:25]

Committed patchset #3 (id:150001)

[commit-bot: I haz the power, 2014-11-04 07:36:08]

Patchset 3 (id:??) landed as
https://crrev.com/5bca2278b8ecb8ae3b933807d3cb9a207616127c
Cr-Commit-Position: refs/heads/master@{#302584}