Add a leaked reference to g_browser_process before showing the uninstall dialog to avoid the views …

Add a leaked reference to g_browser_process before showing the uninstall dialog to avoid the views framework from erroneously exiting the browser process.

BUG=241366
TEST=Uninstall Chrome while it is set as the default browser. Select an item from the "make other browser default" combobox. Observe that the dialog does not immediately close.

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

[robertshield, 2013-05-16 15:17:07]

On 2013/05/16 15:14:24, robertshield wrote:

FWIW, I'm not 100% happy with this. It would be better in the abstract to make
the Views code work in a chrome.exe that hasn't yet bumped the refcount on
g_browser_process to a non-zero value, though I'm not sure how to do that
safely. Suggestions welcome :-)

[msw, 2013-05-16 17:45:38]

Wow, what an awful bug. I suggest getting review from someone that understands
the Menu code a bit better, specifically why MenuController::Run needs
g_browser_process at all (though I only saw add/release refs on
ViewsDelegate::views_delegate), perhaps Scott knows more. This LGTM as a
short-term workaround fix for this one case (I wonder if there are other dialogs
we might show without Chrome running that might have the same issue), but again
I'm not totally familiar with menu's g_browser_process add/release ref.

[robertshield, 2013-05-16 17:53:54]

On 2013/05/16 17:45:38, msw wrote:
> Wow, what an awful bug. I suggest getting review from someone that understands
> the Menu code a bit better, specifically why MenuController::Run needs
> g_browser_process at all (though I only saw add/release refs on
> ViewsDelegate::views_delegate

In practice ViewsDelegate::views_delegate is a ChromeViewsDelegate whose
AddRef/Release implementations forward directly to g_browser_process'
AddRefModule/ReleaseModule methods.

> ), perhaps Scott knows more. This LGTM as a
> short-term workaround fix for this one case (I wonder if there are other
dialogs
> we might show without Chrome running that might have the same issue), but
again
> I'm not totally familiar with menu's g_browser_process add/release ref.

Yeah, I'd welcome a better way to do this too :-) Right now it isn't really safe
to use Views menus anywhere during Chrome startup until g_browser_process has
been refcounted.

Scott, any ideas?

[grt (UTC plus 2), 2013-05-16 17:56:05]

I don't really have anything to add, so LGTM. It would be nice if the hack
wasn't needed, but it seems like a significant undertaking.

[msw, 2013-05-16 18:01:58]

On 2013/05/16 17:53:54, robertshield wrote:
> On 2013/05/16 17:45:38, msw wrote:
> > Wow, what an awful bug. I suggest getting review from someone that
understands
> > the Menu code a bit better, specifically why MenuController::Run needs
> > g_browser_process at all (though I only saw add/release refs on
> > ViewsDelegate::views_delegate
> 
> In practice ViewsDelegate::views_delegate is a ChromeViewsDelegate whose
> AddRef/Release implementations forward directly to g_browser_process'
> AddRefModule/ReleaseModule methods.
> 
> > ), perhaps Scott knows more. This LGTM as a
> > short-term workaround fix for this one case (I wonder if there are other
> dialogs
> > we might show without Chrome running that might have the same issue), but
> again
> > I'm not totally familiar with menu's g_browser_process add/release ref.
> 
> Yeah, I'd welcome a better way to do this too :-) Right now it isn't really
safe
> to use Views menus anywhere during Chrome startup until g_browser_process has
> been refcounted.
> 
> Scott, any ideas?

Could ChromeViewsDelegate::AddRef/ReleaseRef do something else in the case where
the g_browser_process ref count is 0 to start? Also, is leaking the AddRef in
your workaround okay? Will the process shut down correctly when the dialog
closes?

[robertshield, 2013-05-16 18:32:07]

On 2013/05/16 18:01:58, msw wrote:
> On 2013/05/16 17:53:54, robertshield wrote:
> > On 2013/05/16 17:45:38, msw wrote:
> > > Wow, what an awful bug. I suggest getting review from someone that
> understands
> > > the Menu code a bit better, specifically why MenuController::Run needs
> > > g_browser_process at all (though I only saw add/release refs on
> > > ViewsDelegate::views_delegate
> > 
> > In practice ViewsDelegate::views_delegate is a ChromeViewsDelegate whose
> > AddRef/Release implementations forward directly to g_browser_process'
> > AddRefModule/ReleaseModule methods.
> > 
> > > ), perhaps Scott knows more. This LGTM as a
> > > short-term workaround fix for this one case (I wonder if there are other
> > dialogs
> > > we might show without Chrome running that might have the same issue), but
> > again
> > > I'm not totally familiar with menu's g_browser_process add/release ref.
> > 
> > Yeah, I'd welcome a better way to do this too :-) Right now it isn't really
> safe
> > to use Views menus anywhere during Chrome startup until g_browser_process
has
> > been refcounted.
> > 
> > Scott, any ideas?
> 
> Could ChromeViewsDelegate::AddRef/ReleaseRef do something else in the case
where
> the g_browser_process ref count is 0 to start?

I wondered about that, and it would fix it here, but I couldn't convince myself
it would always be correct. 

Greg also suggested setting state on the ChromeViewDelegate to let it know
Chrome is still in a pre-startup phase and no-oping the AddRef/Release while
that state is set.

I'll try one of those.

> Also, is leaking the AddRef in
> your workaround okay? Will the process shut down correctly when the dialog
> closes?

In this case, yes. This code is called from
ChromeBrowserMainParts::PreMainMessageLoopRunImpl and regardless of the dialog
outcome, Chrome exits immediately afterwards.

[robertshield, 2013-05-16 18:59:55]

On 2013/05/16 18:32:07, robertshield wrote:
> On 2013/05/16 18:01:58, msw wrote:
> > On 2013/05/16 17:53:54, robertshield wrote:
> > > On 2013/05/16 17:45:38, msw wrote:
> > > > Wow, what an awful bug. I suggest getting review from someone that
> > understands
> > > > the Menu code a bit better, specifically why MenuController::Run needs
> > > > g_browser_process at all (though I only saw add/release refs on
> > > > ViewsDelegate::views_delegate
> > > 
> > > In practice ViewsDelegate::views_delegate is a ChromeViewsDelegate whose
> > > AddRef/Release implementations forward directly to g_browser_process'
> > > AddRefModule/ReleaseModule methods.
> > > 
> > > > ), perhaps Scott knows more. This LGTM as a
> > > > short-term workaround fix for this one case (I wonder if there are other
> > > dialogs
> > > > we might show without Chrome running that might have the same issue),
but
> > > again
> > > > I'm not totally familiar with menu's g_browser_process add/release ref.
> > > 
> > > Yeah, I'd welcome a better way to do this too :-) Right now it isn't
really
> > safe
> > > to use Views menus anywhere during Chrome startup until g_browser_process
> has
> > > been refcounted.
> > > 
> > > Scott, any ideas?
> > 
> > Could ChromeViewsDelegate::AddRef/ReleaseRef do something else in the case
> where
> > the g_browser_process ref count is 0 to start?
> 
> I wondered about that, and it would fix it here, but I couldn't convince
myself
> it would always be correct. 
> 
> Greg also suggested setting state on the ChromeViewDelegate to let it know
> Chrome is still in a pre-startup phase and no-oping the AddRef/Release while
> that state is set.
> 
> I'll try one of those.
> 
> > Also, is leaking the AddRef in
> > your workaround okay? Will the process shut down correctly when the dialog
> > closes?
> 
> In this case, yes. This code is called from
> ChromeBrowserMainParts::PreMainMessageLoopRunImpl and regardless of the dialog
> outcome, Chrome exits immediately afterwards.

I'll also check this in in the meantime to give folk a chance to merge it if
they need a spot fix.

[commit-bot: I haz the power, 2013-05-16 19:00:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/15134004/3001

[commit-bot: I haz the power, 2013-05-16 19:10:31]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[sky, 2013-05-16 19:26:49]

https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
File chrome/browser/ui/views/uninstall_view.cc (right):

https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
chrome/browser/ui/views/uninstall_view.cc:178:
g_browser_process->AddRefModule();
How does the ref count end up hitting 0 here?

[robertshield, 2013-05-16 19:49:06]

On 2013/05/16 19:26:49, sky wrote:
>
https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
> File chrome/browser/ui/views/uninstall_view.cc (right):
> 
>
https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
> chrome/browser/ui/views/uninstall_view.cc:178:
> g_browser_process->AddRefModule();
> How does the ref count end up hitting 0 here?

It doesn't. This method is called only via DoUninstallTasks in
ChromeBrowserMainParts::PreMainMessageLoopRunImpl. When it returns, Chrome
exits, never starting the main message loops.

[robertshield, 2013-05-16 21:56:53]

On 2013/05/16 19:49:06, robertshield wrote:
> On 2013/05/16 19:26:49, sky wrote:
> >
>
https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
> > File chrome/browser/ui/views/uninstall_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
> > chrome/browser/ui/views/uninstall_view.cc:178:
> > g_browser_process->AddRefModule();
> > How does the ref count end up hitting 0 here?
> 
> It doesn't. This method is called only via DoUninstallTasks in
> ChromeBrowserMainParts::PreMainMessageLoopRunImpl. When it returns, Chrome
> exits, never starting the main message loops.

See here for an alternate approach that blocks the AddRef/Release calls from
views during startup: https://codereview.chromium.org/14845020/

[sky, 2013-05-16 22:57:24]

LGTM

[sky, 2013-05-16 22:58:16]

Ok, I get it. I approved this as I prefer keeping the ref counting
isolate. It's too easy to misuse disabling of ref counting.

On Thu, May 16, 2013 at 2:56 PM,  <robertshield@chromium.org> wrote:
> On 2013/05/16 19:49:06, robertshield wrote:
>>
>> On 2013/05/16 19:26:49, sky wrote:
>> >
>
>
>
https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
>>
>> > File chrome/browser/ui/views/uninstall_view.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/15134004/diff/3001/chrome/browser/ui/views/un...
>>
>> > chrome/browser/ui/views/uninstall_view.cc:178:
>> > g_browser_process->AddRefModule();
>> > How does the ref count end up hitting 0 here?
>
>
>> It doesn't. This method is called only via DoUninstallTasks in
>> ChromeBrowserMainParts::PreMainMessageLoopRunImpl. When it returns, Chrome
>> exits, never starting the main message loops.
>
>
> See here for an alternate approach that blocks the AddRef/Release calls from
> views during startup: https://codereview.chromium.org/14845020/
>
> https://codereview.chromium.org/15134004/

[commit-bot: I haz the power, 2013-05-16 23:20:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/15134004/3001

[commit-bot: I haz the power, 2013-05-17 04:54:42]

Change committed as 200732