Fix a crash seen in Desktop Chrome Windows while displaying a bubble

Fix a crash seen in Desktop Chrome Windows while displaying a bubble

It is not clear as to how this crash occurs. From the callstack, the crash occurs in the context of a bubble getting
created and before it is initialized. In the context of the bubble creation, DesktopNativeWidgetAura instantiates the IME
COM objects which instantiate a local COM server. This causes the main UI loop to be reentered and the bubble is resized in
this context. The WidgetObserver OnWidgetBoundsChanged method in the BubbleDelegateView gets invoked which crashes while
dereferencing the top level widget as it has not been initialized yet.

It is unclear as to how the WidgetObserver is set up here. From the code the BubbleDelegateView observer is setup after the
bubble is initialized, i.e in the BubbleDelegateView::CreateBubble function after the bubble is created.

Fix is to delay the initialization of the IME on Windows until we receive the first IME message or an input
language change notification

BUG=454375
TEST=None at the moment, as it is unclear as to how this crash can happen.

Committed: https://crrev.com/00a39f50c2c623987ab68a4ae253147101d21876
Cr-Commit-Position: refs/heads/master@{#322669}

[ananta, 2015-03-27 02:11:56]

ananta@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-03-27 15:29:16]

On 2015/03/27 02:11:57, ananta wrote:

I'm worried that this isn't enough and you're working around the problem that is
likely to require all sorts of other similarly ugly checks.

Could we instead delay IME creation until we've created everything?

[msw, 2015-03-27 17:52:03]

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

[msw, 2015-03-27 17:52:04]

It'd be helpful to find the regression CL (I highlighted some suspicious CLs on
the bug). Perhaps checking for the presence of a Widget in
ExtensionPopup::OnExtensionSizeChanged would be a more appropriate temporary
workaround, if needed? The caller, ExtensionViewViews::PreferredSizeChanged,
already checks for the presence of a |container_|, which is the
BubbleDelegateView, so presumably extensions have been trying to resize their
containers before the containers are initialized for some time.

[ananta, 2015-03-27 18:52:59]

ananta@chromium.org changed reviewers:
+ mukai@chromium.org

[ananta, 2015-03-27 18:53:55]

On 2015/03/27 15:29:16, sky wrote:
> On 2015/03/27 02:11:57, ananta wrote:
> 
> I'm worried that this isn't enough and you're working around the problem that
is
> likely to require all sorts of other similarly ugly checks.
> 
> Could we instead delay IME creation until we've created everything?

Fair enough. I updated the patch to delay the initialization of the IME and
verified that it works correctly for
non english keyboards.

[ananta, 2015-03-27 18:54:40]

+mukai for IME owners review.

[msw, 2015-03-27 19:05:23]

https://codereview.chromium.org/1037103004/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1037103004/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:69: default_input_language_initialized_ = true;
Nix this, it's set in OnInputLocaleChanged().

https://codereview.chromium.org/1037103004/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:70: // Gets the initial input locale.
Optionally nix comment and curlies, or outdent this by two spaces.

[ananta, 2015-03-27 19:08:39]

https://codereview.chromium.org/1037103004/diff/20001/ui/base/ime/input_metho...
File ui/base/ime/input_method_win.cc (right):

https://codereview.chromium.org/1037103004/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:69: default_input_language_initialized_ = true;
On 2015/03/27 19:05:23, msw wrote:
> Nix this, it's set in OnInputLocaleChanged().

Done.

https://codereview.chromium.org/1037103004/diff/20001/ui/base/ime/input_metho...
ui/base/ime/input_method_win.cc:70: // Gets the initial input locale.
On 2015/03/27 19:05:23, msw wrote:
> Optionally nix comment and curlies, or outdent this by two spaces.

Done.

[Jun Mukai, 2015-03-27 19:49:22]

mukai@chromium.org changed reviewers:
+ yukawa@chromium.org

[Jun Mukai, 2015-03-27 19:49:56]

it looks good, but yukawa@ should review input_method_win.

[yukawa, 2015-03-27 20:29:55]

Can you update the CL description?  It looks a bit obsolete. (We no longer touch
GetWidget(), right?)

[yukawa, 2015-03-27 20:48:56]

yukawa@chromium.org changed reviewers:
+ shuchen@chromium.org

[yukawa, 2015-03-27 20:48:56]

Regarding the initialization of a COM server, probably that's here, right?
https://chromium.googlesource.com/chromium/src/+/5e16d46c92747ae76914b9a5db11...

Just for the record, we can remove that code after http://crbug.com/296509 is
fixed.  Basically we are not using the returned value on Windows anymore.

CC-ing Shu who is willing to own that bug.

[ananta, 2015-03-27 20:52:03]

On 2015/03/27 20:48:56, yukawa wrote:
> Regarding the initialization of a COM server, probably that's here, right?
>
https://chromium.googlesource.com/chromium/src/+/5e16d46c92747ae76914b9a5db11...
> 
> Just for the record, we can remove that code after http://crbug.com/296509 is
> fixed.  Basically we are not using the returned value on Windows anymore.
> 
> CC-ing Shu who is willing to own that bug.

[ananta, 2015-03-27 20:52:36]

On 2015/03/27 20:48:56, yukawa wrote:
> Regarding the initialization of a COM server, probably that's here, right?
>
https://chromium.googlesource.com/chromium/src/+/5e16d46c92747ae76914b9a5db11...
> 
> Just for the record, we can remove that code after http://crbug.com/296509 is
> fixed.  Basically we are not using the returned value on Windows anymore.
> 
> CC-ing Shu who is willing to own that bug.

Updated the CL description.

Thanks
Ananta

[yukawa, 2015-03-27 21:00:14]

lgtm

LGTM.

Thanks. I'll file a follow-up issue as a reminder to (try to) revert this CL
after http://crbug.com/296509 is fixed.

[ananta, 2015-03-27 22:10:15]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-03-27 22:10:45]

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

[commit-bot: I haz the power, 2015-03-27 23:10:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-27 23:11:36]

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