Before deleting a profile, cancel all in-progress downloads

Before deleting a profile, cancel all in-progress downloads to prevent a "Do you really want to exit Chrome" popup, which (1) is confusing because the message is not accurate, and (2) prevents a window corresponding to the deleted profile from closing, leaving it in an inconsistent state.

A UI update (warning the user that downloads will be canceled) will follow in a separate CL.

Note: I'm aware that this doesn't address crbug.com/289390 which is another way to trigger the same underlying bug. Working on it...

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

[Marc Treib, 2014-03-28 14:58:25]

rlp@chromium.org: Please review changes in profile_manager.cc

benjhayden@chromium.org: Please review changes in download_service.h/cc

Try failures seem unrelated.

This doesn't have a test - most tests related to Profiles and the ProfileManager
have been flaky/disabled for a long time (see crbug.com/165760), so there
doesn't seem to be any point in creating another disabled test.

[benjhayden, 2014-03-28 18:51:18]

Looks like a straightforward code move refactor in DownloadService.
LGTM

[asanka, 2014-03-28 19:51:15]

Why is this necessary? The in-progress downloads are supposed to be destroyed
during profile shutdown. See the code path from DownloadService::ShutDown() ->
DownloadManager::ShutDown().

Destroying the current in-progress downloads doesn't reliably solve the problem
if the profile is alive and capable of starting a new download after this point.

[Marc Treib, 2014-03-31 07:36:46]

On 2014/03/28 19:51:15, asanka wrote:
> Why is this necessary? The in-progress downloads are supposed to be destroyed
> during profile shutdown. See the code path from DownloadService::ShutDown() ->
> DownloadManager::ShutDown().
> 
> Destroying the current in-progress downloads doesn't reliably solve the
problem
> if the profile is alive and capable of starting a new download after this
point.

The profile is not actually shut down at this point. It is however removed from
the ProfileInfoCache, and so doesn't appear in the avatar menu anymore. So once
all of its windows have been closed, it shouldn't be capable of starting a new
download (or am I missing something?). At least, this gets rid of the confusing
message popup.

[asanka, 2014-03-31 15:57:21]

On 2014/03/31 07:36:46, treib wrote:
> On 2014/03/28 19:51:15, asanka wrote:
> > Why is this necessary? The in-progress downloads are supposed to be
destroyed
> > during profile shutdown. See the code path from DownloadService::ShutDown()
->
> > DownloadManager::ShutDown().
> > 
> > Destroying the current in-progress downloads doesn't reliably solve the
> problem
> > if the profile is alive and capable of starting a new download after this
> point.
> 
> The profile is not actually shut down at this point. It is however removed
from
> the ProfileInfoCache, and so doesn't appear in the avatar menu anymore. So
once
> all of its windows have been closed, it shouldn't be capable of starting a new
> download (or am I missing something?). At least, this gets rid of the
confusing
> message popup.

It seems this doesn't address the underlying problem. If the profile is slated
for deletion while downloads are in-progress we should either:
a) figure out a less confusing way to warn the user about what's about to happen
(that their downloads are going to be discarded). Or
b) assume it's okay to discard the downloads without a warning (not very user
friendly), and not perform the in-progress download check for such profiles.

[Marc Treib, 2014-04-01 11:12:46]

On 2014/03/31 15:57:21, asanka wrote:
> On 2014/03/31 07:36:46, treib wrote:
> > On 2014/03/28 19:51:15, asanka wrote:
> > > Why is this necessary? The in-progress downloads are supposed to be
> destroyed
> > > during profile shutdown. See the code path from
DownloadService::ShutDown()
> ->
> > > DownloadManager::ShutDown().
> > > 
> > > Destroying the current in-progress downloads doesn't reliably solve the
> > problem
> > > if the profile is alive and capable of starting a new download after this
> > point.
> > 
> > The profile is not actually shut down at this point. It is however removed
> from
> > the ProfileInfoCache, and so doesn't appear in the avatar menu anymore. So
> once
> > all of its windows have been closed, it shouldn't be capable of starting a
new
> > download (or am I missing something?). At least, this gets rid of the
> confusing
> > message popup.
> 
> It seems this doesn't address the underlying problem. If the profile is slated
> for deletion while downloads are in-progress we should either:
> a) figure out a less confusing way to warn the user about what's about to
happen
> (that their downloads are going to be discarded). Or
> b) assume it's okay to discard the downloads without a warning (not very user
> friendly), and not perform the in-progress download check for such profiles.

Right, this is the first part of a). The less confusing message will follow in
another CL:
https://codereview.chromium.org/220593003

[Marc Treib, 2014-04-03 10:45:29]

On 2014/04/01 11:12:46, treib wrote:
> On 2014/03/31 15:57:21, asanka wrote:
> > It seems this doesn't address the underlying problem. If the profile is
slated
> > for deletion while downloads are in-progress we should either:
> > a) figure out a less confusing way to warn the user about what's about to
> happen
> > (that their downloads are going to be discarded). Or
> > b) assume it's okay to discard the downloads without a warning (not very
user
> > friendly), and not perform the in-progress download check for such profiles.
> 
> Right, this is the first part of a). The less confusing message will follow in
> another CL:
> https://codereview.chromium.org/220593003

Ping: Any more feedback here? Otherwise I'll be abandoning this since I have
other things to do now.

[asanka, 2014-04-03 15:42:54]

On 2014/04/03 10:45:29, treib wrote:
> On 2014/04/01 11:12:46, treib wrote:
> > On 2014/03/31 15:57:21, asanka wrote:
> > > It seems this doesn't address the underlying problem. If the profile is
> slated
> > > for deletion while downloads are in-progress we should either:
> > > a) figure out a less confusing way to warn the user about what's about to
> > happen
> > > (that their downloads are going to be discarded). Or
> > > b) assume it's okay to discard the downloads without a warning (not very
> user
> > > friendly), and not perform the in-progress download check for such
profiles.
> > 
> > Right, this is the first part of a). The less confusing message will follow
in
> > another CL:
> > https://codereview.chromium.org/220593003
> 
> Ping: Any more feedback here? Otherwise I'll be abandoning this since I have
> other things to do now.

Sorry about the delay. I don't mean to block this CL, but I'm pointing out that
while this CL would prevent this specific bug from manifesting, it may not be
addressing the underlying problems in the code.
LGTM if the profile lifetime extends substantially longer past
ProfileManager::FinishDeletingProfile() since this change would release
resources sooner.

What's the behavior if a profile being deleted has a window open that has an
onUnload handler that prompts? It seems the logic in ProfileManager assumes that
CloseAllBrowsersWithProfile() will synchronously close the browser windows and
that's part of the cause of this bug. I agree that warning the user upfront as
is done in the followup CL is the correct thing to do. Once the user has given
their consent to nuke the profile, the browser windows should be able to close
without any additional UI. In-progress downloads is just one of the things that
cause browser windows to delay closing. If the browser window closure could take
profile deletion into account (e.g. if the profile could be safely queued or
marked for deletion before browser windows are closed), then we won't need to
kill downloads manually ahead of profile shutdown.

Edit: Just noticed that I might be describing crbug.com/289390.

[Marc Treib, 2014-04-03 16:31:42]

On 2014/04/03 15:42:54, asanka wrote:
> Sorry about the delay. I don't mean to block this CL, but I'm pointing out
that
> while this CL would prevent this specific bug from manifesting, it may not be
> addressing the underlying problems in the code.
> LGTM if the profile lifetime extends substantially longer past
> ProfileManager::FinishDeletingProfile() since this change would release
> resources sooner.
> 
> What's the behavior if a profile being deleted has a window open that has an
> onUnload handler that prompts? It seems the logic in ProfileManager assumes
that
> CloseAllBrowsersWithProfile() will synchronously close the browser windows and
> that's part of the cause of this bug. I agree that warning the user upfront as
> is done in the followup CL is the correct thing to do. Once the user has given
> their consent to nuke the profile, the browser windows should be able to close
> without any additional UI. In-progress downloads is just one of the things
that
> cause browser windows to delay closing. If the browser window closure could
take
> profile deletion into account (e.g. if the profile could be safely queued or
> marked for deletion before browser windows are closed), then we won't need to
> kill downloads manually ahead of profile shutdown.
> 
> Edit: Just noticed that I might be describing crbug.com/289390.

Thanks for the feedback! You are correct that this CL does not address the
underlying problem, i.e. that CloseAllBrowsersWithProfile() does not actually
close the windows synchronously (or at all in some cases). This CL was supposed
to be a quick-fix for the (presumably) most common way to trigger the bug, while
we figure out what to do about the general case.
I agree that the most sensible thing from the user's perspective would be to
nuke all browser windows on profile deletion, without any UI. But afaict
BrowserWindow does not allow this at the moment, and introducing that capability
seems like quite a big change to handle a small edge case. So maybe we should
just wait until all windows are gone before actually removing the profile?

[asanka, 2014-04-03 21:45:47]

On 2014/04/03 16:31:42, treib wrote:
> On 2014/04/03 15:42:54, asanka wrote:
> > Sorry about the delay. I don't mean to block this CL, but I'm pointing out
> that
> > while this CL would prevent this specific bug from manifesting, it may not
be
> > addressing the underlying problems in the code.
> > LGTM if the profile lifetime extends substantially longer past
> > ProfileManager::FinishDeletingProfile() since this change would release
> > resources sooner.
> > 
> > What's the behavior if a profile being deleted has a window open that has an
> > onUnload handler that prompts? It seems the logic in ProfileManager assumes
> that
> > CloseAllBrowsersWithProfile() will synchronously close the browser windows
and
> > that's part of the cause of this bug. I agree that warning the user upfront
as
> > is done in the followup CL is the correct thing to do. Once the user has
given
> > their consent to nuke the profile, the browser windows should be able to
close
> > without any additional UI. In-progress downloads is just one of the things
> that
> > cause browser windows to delay closing. If the browser window closure could
> take
> > profile deletion into account (e.g. if the profile could be safely queued or
> > marked for deletion before browser windows are closed), then we won't need
to
> > kill downloads manually ahead of profile shutdown.
> > 
> > Edit: Just noticed that I might be describing crbug.com/289390.
> 
> Thanks for the feedback! You are correct that this CL does not address the
> underlying problem, i.e. that CloseAllBrowsersWithProfile() does not actually
> close the windows synchronously (or at all in some cases). This CL was
supposed
> to be a quick-fix for the (presumably) most common way to trigger the bug,
while
> we figure out what to do about the general case.
> I agree that the most sensible thing from the user's perspective would be to
> nuke all browser windows on profile deletion, without any UI. But afaict
> BrowserWindow does not allow this at the moment, and introducing that
capability
> seems like quite a big change to handle a small edge case. So maybe we should
> just wait until all windows are gone before actually removing the profile?

Someone more familiar with browser shutdown should comment here since I'm not
sure BrowserCloseManager is the model to follow. Based on briefly looking at
BrowserCloseManager::CloseBrowsers(), it looks like it's not too complicated to
close a browser window without user interaction. But that runs the risk of the
user not getting an opportunity to deal with any onUnload handler initiated
warnings.

Waiting for browsers to close could look something like
BrowserCloseManager::TryToCloseBrowsers() except it would only iterate over
windows belonging to the profile being deleted.

[rpetterson, 2014-04-09 20:24:36]

profiles LGTM

[Marc Treib, 2014-04-10 07:14:39]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-04-10 07:14:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/treib@chromium.org/214523003/1

[commit-bot: I haz the power, 2014-04-10 13:43:25]

Change committed as 262970