make "reload" on chrome://extensions automatically relaunch running apps

make "reload" on chrome://extensions automatically relaunch running platform apps

BUG=133829
TEST=launch an app, then reload via the page reload button or the Reload link.

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

[Evan Stade, 2012-06-22 05:17:51]

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:418: //
onLaunched event, so we just wait half a second.
suggestions for improvement welcome.

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:509: // views),
then mark it for relaunch later.
this is kind of a hokey definition of "running", but it seems to do the trick
most of the time. You could imagine a platform app that does something which
doesn't create render views when you launch it, in which case this wouldn't be
able to determine that the app had been launched and needs relaunching.

[Aaron Boodman, 2012-06-22 05:35:02]

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:418: //
onLaunched event, so we just wait half a second.
On 2012/06/22 05:17:51, Evan Stade wrote:
> suggestions for improvement welcome.

LazyBackgroundTaskQueue ?

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:509: // views),
then mark it for relaunch later.
On 2012/06/22 05:17:51, Evan Stade wrote:
> this is kind of a hokey definition of "running", but it seems to do the trick
> most of the time. You could imagine a platform app that does something which
> doesn't create render views when you launch it, in which case this wouldn't be
> able to determine that the app had been launched and needs relaunching.

I think if there are no running render views, the app is by definition not
running.

You could also just check if there is any process for this extension. That would
be about the same.

[Evan Stade, 2012-06-22 19:39:51]

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:418: //
onLaunched event, so we just wait half a second.
On 2012/06/22 05:35:02, Aaron Boodman wrote:
> On 2012/06/22 05:17:51, Evan Stade wrote:
> > suggestions for improvement welcome.
> 
> LazyBackgroundTaskQueue ?

awesome

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:509: // views),
then mark it for relaunch later.
On 2012/06/22 05:35:02, Aaron Boodman wrote:
> On 2012/06/22 05:17:51, Evan Stade wrote:
> > this is kind of a hokey definition of "running", but it seems to do the
trick
> > most of the time. You could imagine a platform app that does something which
> > doesn't create render views when you launch it, in which case this wouldn't
be
> > able to determine that the app had been launched and needs relaunching.
> 
> I think if there are no running render views, the app is by definition not
> running.
> 
> You could also just check if there is any process for this extension. That
would
> be about the same.

I don't want to count background pages.

[Aaron Boodman, 2012-06-22 20:30:54]

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:509: // views),
then mark it for relaunch later.
On 2012/06/22 19:39:51, Evan Stade wrote:
> On 2012/06/22 05:35:02, Aaron Boodman wrote:
> > On 2012/06/22 05:17:51, Evan Stade wrote:
> > > this is kind of a hokey definition of "running", but it seems to do the
> trick
> > > most of the time. You could imagine a platform app that does something
which
> > > doesn't create render views when you launch it, in which case this
wouldn't
> be
> > > able to determine that the app had been launched and needs relaunching.
> > 
> > I think if there are no running render views, the app is by definition not
> > running.
> > 
> > You could also just check if there is any process for this extension. That
> would
> > be about the same.
> 
> I don't want to count background pages.

I'm surprised that the event page is not returned here. An EventPage is a
WebContents that contains a RVH. It seems like it should be returned, and is
probably a bug that will eventually be fixed if it isn't.

[Aaron Boodman, 2012-06-22 20:32:47]

On Fri, Jun 22, 2012 at 1:30 PM,  <aa@chromium.org> wrote:
>
>
http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
> File chrome/browser/ui/webui/extensions/extension_settings_handler.cc
> (right):
>
>
http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
> chrome/browser/ui/webui/extensions/extension_settings_handler.cc:509: //
> views), then mark it for relaunch later.
> On 2012/06/22 19:39:51, Evan Stade wrote:
>>
>> On 2012/06/22 05:35:02, Aaron Boodman wrote:
>> > On 2012/06/22 05:17:51, Evan Stade wrote:
>> > > this is kind of a hokey definition of "running", but it seems to
>
> do the
>>
>> trick
>> > > most of the time. You could imagine a platform app that does
>
> something which
>>
>> > > doesn't create render views when you launch it, in which case this
>
> wouldn't
>>
>> be
>> > > able to determine that the app had been launched and needs
>
> relaunching.
>>
>> >
>> > I think if there are no running render views, the app is by
>
> definition not
>>
>> > running.
>> >
>> > You could also just check if there is any process for this
>
> extension. That
>>
>> would
>> > be about the same.
>
>
>> I don't want to count background pages.
>
>
> I'm surprised that the event page is not returned here. An EventPage is
> a WebContents that contains a RVH. It seems like it should be returned,
> and is probably a bug that will eventually be fixed if it isn't.

It occurs to me that we need to make this exact same decision to know
when to shut down event pages (are there any visible views). You could
look and see how EPM makes this call. Or just ask Matt.

[Matt Perry, 2012-06-22 20:40:29]

On 2012/06/22 20:32:47, Aaron Boodman wrote:
> On Fri, Jun 22, 2012 at 1:30 PM,  <mailto:aa@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
> > File chrome/browser/ui/webui/extensions/extension_settings_handler.cc
> > (right):
> >
> >
>
http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
> > chrome/browser/ui/webui/extensions/extension_settings_handler.cc:509: //
> > views), then mark it for relaunch later.
> > On 2012/06/22 19:39:51, Evan Stade wrote:
> >>
> >> On 2012/06/22 05:35:02, Aaron Boodman wrote:
> >> > On 2012/06/22 05:17:51, Evan Stade wrote:
> >> > > this is kind of a hokey definition of "running", but it seems to
> >
> > do the
> >>
> >> trick
> >> > > most of the time. You could imagine a platform app that does
> >
> > something which
> >>
> >> > > doesn't create render views when you launch it, in which case this
> >
> > wouldn't
> >>
> >> be
> >> > > able to determine that the app had been launched and needs
> >
> > relaunching.
> >>
> >> >
> >> > I think if there are no running render views, the app is by
> >
> > definition not
> >>
> >> > running.
> >> >
> >> > You could also just check if there is any process for this
> >
> > extension. That
> >>
> >> would
> >> > be about the same.
> >
> >
> >> I don't want to count background pages.
> >
> >
> > I'm surprised that the event page is not returned here. An EventPage is
> > a WebContents that contains a RVH. It seems like it should be returned,
> > and is probably a bug that will eventually be fixed if it isn't.
> 
> It occurs to me that we need to make this exact same decision to know
> when to shut down event pages (are there any visible views). You could
> look and see how EPM makes this call. Or just ask Matt.

I'm also surprised that GetRenderViewHostsForExtension does not include the
event page. I had to explicitly check for the event page in that set when
keeping track of the visible views.

So, yes, Aaron is right that the EPM needs to decide this as well. But included
in that decision is whether the event page is doing anything (like making a
network request, or calling an extension API). The EPM does not distinguish
between any of these signals, so if you just want to know visible views, you
need to do this check. If you want to answer the question "Is the event page
inactive and ready to shut down?" then check if EPM::GetLazyKeepaliveCount
returns 0.

[Evan Stade, 2012-06-22 22:29:49]

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10641017/diff/1003/chrome/browser/ui/webui/ext...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:509: // views),
then mark it for relaunch later.
On 2012/06/22 20:30:54, Aaron Boodman wrote:
> On 2012/06/22 19:39:51, Evan Stade wrote:
> > On 2012/06/22 05:35:02, Aaron Boodman wrote:
> > > On 2012/06/22 05:17:51, Evan Stade wrote:
> > > > this is kind of a hokey definition of "running", but it seems to do the
> > trick
> > > > most of the time. You could imagine a platform app that does something
> which
> > > > doesn't create render views when you launch it, in which case this
> wouldn't
> > be
> > > > able to determine that the app had been launched and needs relaunching.
> > > 
> > > I think if there are no running render views, the app is by definition not
> > > running.
> > > 
> > > You could also just check if there is any process for this extension. That
> > would
> > > be about the same.
> > 
> > I don't want to count background pages.
> 
> I'm surprised that the event page is not returned here. An EventPage is a
> WebContents that contains a RVH. It seems like it should be returned, and is
> probably a bug that will eventually be fixed if it isn't.

oh, it does. I was confused because the background page was inactive at the
time.

http://codereview.chromium.org/10641017/diff/10001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10641017/diff/10001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:418: if
(queue->ShouldEnqueueTask(profile, extension)) {
you would expect this to work, but it doesn't, because right after this LOADED
event we get an UNLOADED for the same extension, which flushes the queue. It's
easy enough to see why this is happening, here is the stack trace:

	ExtensionSettingsHandler::LaunchApplication() [0x7f18baa07329]
	extensions::LazyBackgroundTaskQueue::ProcessPendingTasks() [0x7f18bc6c09fc]
	extensions::LazyBackgroundTaskQueue::Observe() [0x7f18bc6c0fe1]
	NotificationServiceImpl::Notify() [0x7f18bc81e5f4]
	ExtensionHost::~ExtensionHost() [0x7f18bc65d98b]
	ExtensionProcessManager::CloseBackgroundHost() [0x7f18bc6800c5]
	ExtensionProcessManager::Observe() [0x7f18bc681632]
	NotificationServiceImpl::Notify() [0x7f18bc81da94]
	ExtensionService::NotifyExtensionUnloaded() [0x7f18bc695f33]
	ExtensionService::UnloadExtension() [0x7f18bc6966e3]
	ExtensionService::InitializePermissions() [0x7f18bc697ed3]
	ExtensionService::AddExtension() [0x7f18bc699d8f]
	ExtensionService::OnExtensionInstalled() [0x7f18bc692f92]
	extensions::UnpackedInstaller::OnLoaded() [0x7f18bc6d447f]

but it's harder to see what the correct fix is. The end result is that when you
reload an unpacked extension, you get an UNLOADED, LOADED (from
EnableExtension), UNLOADED (from InitializePermissions), and then another LOADED
(from AddExtension).

[Mihai Parparita -not on Chrome, 2012-07-11 15:22:45]

(lost track of this CL in the pre-I/O rush)

Naive question: Is there a reason why you can't use LaunchPlatformApp (from
plaform_app_launcher.h)? That tries to do the right thing as far as enqueuing
the launch event, etc. And if it doesn't, then it should be fixed (it's what the
NTP uses for launching apps)

[Evan Stade, 2012-07-12 01:41:54]

On 2012/07/11 15:22:45, Mihai Parparita wrote:
> (lost track of this CL in the pre-I/O rush)
> 
> Naive question: Is there a reason why you can't use LaunchPlatformApp (from
> plaform_app_launcher.h)? That tries to do the right thing as far as enqueuing
> the launch event, etc. And if it doesn't, then it should be fixed (it's what
the
> NTP uses for launching apps)

I could try that, and it sounds like it would simplify this code, but I doubt it
would work because it would have the same problem as above.

[Mihai Parparita -not on Chrome, 2012-07-12 03:30:55]

On Wed, Jul 11, 2012 at 6:41 PM, <estade@chromium.org> wrote:

> I could try that, and it sounds like it would simplify this code, but I
> doubt it
> would work because it would have the same problem as above.
>

True.

We already have something similar with re-connecting of devtools when
reloading the extension (search for orphaned_dev_tools_ in
extension_service.cc). Perhaps that can be generalized a bit to also
relaunch the app if it was running.

Mihai

[Aaron Boodman, 2012-08-06 23:41:24]

Old CL is old. Can it be deleted?

[Evan Stade, 2012-08-07 00:23:00]

no, I only put this aside because of m22 crunch and because of discussed
technical difficulties. I still think this is a good idea.

[Evan Stade, 2012-08-18 00:49:55]

ok, updated it with a slight hack to ignore the LOAD notifications until ENABLED
has already been seen. This works because EnableExtension() sends its LOAD
before ENABLED, so the relaunch skips over that one. I tried to fix this by
modifying ExtensionService to not send so many LOAD and UNLOADs, but pretty much
everything I tried broke something.

[Aaron Boodman, 2012-08-21 21:02:48]

Is it possible to just do the launch in response to ENABLE?

[Evan Stade, 2012-08-21 21:04:11]

On 2012/08/21 21:02:48, Aaron Boodman wrote:
> Is it possible to just do the launch in response to ENABLE?

no, because the ensuing LOAD/UNLOAD/LOAD flushes the LazyBackgroundTaskQueue

[Mihai Parparita -not on Chrome, 2012-08-21 22:38:35]

Any thoughts on my devtools re-opening pointer? I think your code is actually
more correct (it relies on DidCreateRenderViewForBackgroundPage being called,
but for event pages I don't think that will happen), it might be cleaner to have
a extension/app ID -> RestoreState map, where RestoreState is a struct with
- a RestorePhase enum, that you can use to step through the LOADED/ENABLED
notifications
- a shouldRelaunch bool
- a devtools cookie

http://codereview.chromium.org/10641017/diff/17001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right):

http://codereview.chromium.org/10641017/diff/17001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_settings_handler.cc:439:
weak_factory_.GetWeakPtr()));
You can get both the Extension and Profile instances from the passed in
ExtensionHost, so I don't think you need the callback to be a bound method.

[Evan Stade, 2012-08-22 01:16:06]

are you suggesting also moving the inspector code into ExtensionSettingsHandler?
things are a little simpler if we move it all to ExtensionService instead, and
arguably this should always work regardless of whether the extension settings
page is open.

[Aaron Boodman, 2012-08-22 21:23:47]

I'm going to leave this review to mihai.

[Mihai Parparita -not on Chrome, 2012-08-22 21:27:38]

On 2012/08/22 01:16:06, Evan Stade wrote:
> are you suggesting also moving the inspector code into
ExtensionSettingsHandler?
> things are a little simpler if we move it all to ExtensionService instead, and
> arguably this should always work regardless of whether the extension settings
> page is open.

Didn't mean that it all has to live in ExtensionSettingsHandler, just that I
wanted "restore state after reload" code to be in one place. Doing it all in
ExtensionService seems fine.

[Evan Stade, 2012-08-23 21:21:31]

I looked into combining with the devtools re-attachment code. Trying to reattach
the devtools with a lazy background page queue task didn't work. That task fires
later than DidCreateRenderViewForBackgroundPage, and by that time Reattach no
longer seems to completely work (e.g., sources are not loaded).

Therefore I left the devtools code alone, but moved the relaunch code to
extension_service which obviated the need for the second-LOADED-event hack.

I also found this bug:
https://code.google.com/p/chromium/issues/detail?id=144495

[Mihai Parparita -not on Chrome, 2012-08-24 21:15:58]

LGTM