Mac: Fix crash when opening Incognito window

Mac: Fix crash when opening Incognito window

Regressed by https://crrev.com/de66e360e2a2944bdf71608726b19b3bf2a2a6ff.

ExtensionCommandsGlobalRegistry::Get() returns null when incognito, add a check
for it.

BUG=450903
Committed: https://crrev.com/6fe37681decffe98d09682f52fd3b66cd90cafef
Cr-Commit-Position: refs/heads/master@{#312726}

[Andre, 2015-01-22 21:18:39]

andresantoso@chromium.org changed reviewers:
+ rsesek@chromium.org

[Andre, 2015-01-22 21:18:40]

Robert, please review.
I investigated why browser_tests did not catch this.
There are tests that open incognito windows, but windowDidBecomeMain did
not get a chance to get executed because it is not called synchronously when
showing the window.

[Robert Sesek, 2015-01-22 21:20:16]

LGTM

[Andre, 2015-01-22 21:27:48]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-01-22 21:28:54]

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

[commit-bot: I haz the power, 2015-01-22 22:41:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 22:41:10]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Andre, 2015-01-22 23:23:25]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-01-22 23:25:08]

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

[commit-bot: I haz the power, 2015-01-22 23:27:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 23:27:01]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Andre, 2015-01-23 01:15:40]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-01-23 01:17:17]

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

[commit-bot: I haz the power, 2015-01-23 01:20:18]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-23 01:21:05]

Patchset 1 (id:??) landed as
https://crrev.com/6fe37681decffe98d09682f52fd3b66cd90cafef
Cr-Commit-Position: refs/heads/master@{#312726}

[Finnur, 2015-01-23 10:49:38]

finnur@chromium.org changed reviewers:
+ finnur@chromium.org

[Finnur, 2015-01-23 10:49:38]

https://codereview.chromium.org/822083004/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/822083004/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:635: }
Hmm... I'm not sure this is the right fix...

We should be updating the global registry to let it know the registry for the
incognito window is now the active one. Otherwise, it will think the wrong
registry is active and disable shortcuts for the wrong window.

I'm thinking we should adjust the Global Registry to define:

  static const bool kServiceRedirectedInIncognito = true;

... so that the same instance (of the global registry) is returned in both
incognito and regular windows.

[Andre, 2015-01-23 18:24:01]

https://codereview.chromium.org/822083004/diff/1/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/822083004/diff/1/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:635: }
On 2015/01/23 10:49:38, Finnur wrote:
> Hmm... I'm not sure this is the right fix...
> 
> We should be updating the global registry to let it know the registry for the
> incognito window is now the active one. Otherwise, it will think the wrong
> registry is active and disable shortcuts for the wrong window.
> 
> I'm thinking we should adjust the Global Registry to define:
> 
>   static const bool kServiceRedirectedInIncognito = true;
> 
> ... so that the same instance (of the global registry) is returned in both
> incognito and regular windows.

Thanks Finnur.
When recording a shortcut for an extension, the active window is always the
window that has the settings page, and we don't allow opening settings on
an incognito window. We also cancel recording state when the active window
is switched.
The counterpart code in Views (toolbar_view.cc) does the same check as above.
I'm having problem imagining the problem scenario, do you have one in mind?

[Finnur, 2015-01-26 09:24:54]

> I'm having problem imagining the problem scenario, do you have one in mind?

Possibly (bear with me, I'll get to it). But first, lets take a step back.
There's two ways to fix the crash you were seeing. Either add null-checks (as
per the checked in code) or make sure the pointer is never NULL (my proposal).

I don't see an obvious benefit for the null-checks, but I see an obvious
drawback in that there's a non-zero chance that someone will forget to add the
check, as we've already discovered. :)

Conversely...

I don't see an obvious drawback for making sure the pointer is never NULL but I
see a couple of obvious benefits to doing so:

1) We don't have to sprinkle null-checks around the code base.
2) We might add code in the future that relies on always having the right
registry available to us and currently it can be wrong (when we switch to an
incognito) or even never set (if we start Chrome in incognito).

Thinking about that last point, I don't remember if Chrome on ChromeOS can be
started in incognito (guest mode?) but if it can then the
RewriteWithKeyboardRemappingsByKeyCode code is going to fail to avoid converting
some shortcuts (see discussion in
https://code.google.com/p/chromium/issues/detail?id=410944, which is what got
the active-registry-tracking introduced).

So, there may be a problem today and there may be a problem in the future, both
of which we can avoid by making the global registry accessible to incognito
windows.

[Andre, 2015-01-27 01:08:14]

On 2015/01/26 09:24:54, Finnur wrote:
> > I'm having problem imagining the problem scenario, do you have one in mind?
> 
> Possibly (bear with me, I'll get to it). But first, lets take a step back.
> There's two ways to fix the crash you were seeing. Either add null-checks (as
> per the checked in code) or make sure the pointer is never NULL (my proposal).
> 
> I don't see an obvious benefit for the null-checks, but I see an obvious
> drawback in that there's a non-zero chance that someone will forget to add the
> check, as we've already discovered. :)
> 
> Conversely...
> 
> I don't see an obvious drawback for making sure the pointer is never NULL but
I
> see a couple of obvious benefits to doing so:
> 
> 1) We don't have to sprinkle null-checks around the code base.
> 2) We might add code in the future that relies on always having the right
> registry available to us and currently it can be wrong (when we switch to an
> incognito) or even never set (if we start Chrome in incognito).
> 
> Thinking about that last point, I don't remember if Chrome on ChromeOS can be
> started in incognito (guest mode?) but if it can then the
> RewriteWithKeyboardRemappingsByKeyCode code is going to fail to avoid
converting
> some shortcuts (see discussion in
> https://code.google.com/p/chromium/issues/detail?id=410944, which is what got
> the active-registry-tracking introduced).
> 
> So, there may be a problem today and there may be a problem in the future,
both
> of which we can avoid by making the global registry accessible to incognito
> windows.

Got it, thanks.
I think I was confused by your previous comment about disabling shortcuts for
the wrong
window, so I thought you were wanting to call set_registry_for_active_window()
with the
incognito window. (And I did not know kServiceRedirectedInIncognito is an
existing thing).
Removing the necessity to null-check SGTM, PTAL at
https://codereview.chromium.org/883553002/