Fix the issue that GetFrameWindow() may change between AddObserver and RemoveObserver in KeyboardCo…

Fix the issue that GetFrameWindow() may change between AddObserver and RemoveObserver in KeyboardController.

BUG=408995
TEST=Verified on linux_chromeos.

Committed: https://crrev.com/afba61456205f2254c7b69f74e92857b3b51a856
Cr-Commit-Position: refs/heads/master@{#292949}

[Shu Chen, 2014-09-02 04:38:44]

shuchen@chromium.org changed reviewers:
+ bshe@chromium.org, kevers@chromium.org

[Shu Chen, 2014-09-02 04:38:45]

Kevin/Biao, can you please review this cl? Thanks

[Shu Chen, 2014-09-02 04:43:42]

https://codereview.chromium.org/527153002/diff/20001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/527153002/diff/20001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:150: while(window->parent() &&
window->parent()->id() < 0) {
Kevin/Biao, do you think there is a case that a top Window had a negative id?

I've tested this change a little bit on linux_chromeos and cannot find any
problems.

If you think it'd better to be safe to check negative id here, an alternative
solution is to hold a set of "observed windows", and make sure RemoveObserver()
on all "observed windows" at destructor.

The crash bug was caused by
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_obs....

[bshe, 2014-09-02 12:58:47]

https://codereview.chromium.org/527153002/diff/20001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/527153002/diff/20001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:150: while(window->parent() &&
window->parent()->id() < 0) {
On 2014/09/02 04:43:42, Shu Chen wrote:
> Kevin/Biao, do you think there is a case that a top Window had a negative id?
> 
> I've tested this change a little bit on linux_chromeos and cannot find any
> problems.
> 
> If you think it'd better to be safe to check negative id here, an alternative
> solution is to hold a set of "observed windows", and make sure
RemoveObserver()
> on all "observed windows" at destructor.
> 
> The crash bug was caused by
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_obs....
> 

I am not familiar with this code. kevers@ could comment better.

But it seems like the new code returns the top level window, while the old code
returns the immediate child of top level window if I am reading it correctly. I
am a little bit nervous about this change but I dont know enough to be certain.

[kevers, 2014-09-02 14:10:34]

https://codereview.chromium.org/527153002/diff/60001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/527153002/diff/60001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:221: window->RemoveObserver(this);
observed_windows_.erase(window);

[Shu Chen, 2014-09-02 14:30:44]

https://codereview.chromium.org/527153002/diff/20001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/527153002/diff/20001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:150: while(window->parent() &&
window->parent()->id() < 0) {
On 2014/09/02 12:58:47, bshe wrote:
> On 2014/09/02 04:43:42, Shu Chen wrote:
> > Kevin/Biao, do you think there is a case that a top Window had a negative
id?
> > 
> > I've tested this change a little bit on linux_chromeos and cannot find any
> > problems.
> > 
> > If you think it'd better to be safe to check negative id here, an
alternative
> > solution is to hold a set of "observed windows", and make sure
> RemoveObserver()
> > on all "observed windows" at destructor.
> > 
> > The crash bug was caused by
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_obs....
> > 
> 
> I am not familiar with this code. kevers@ could comment better.
> 
> But it seems like the new code returns the top level window, while the old
code
> returns the immediate child of top level window if I am reading it correctly.
I
> am a little bit nervous about this change but I dont know enough to be
certain.

Done. I've changed to safer solution.

https://codereview.chromium.org/527153002/diff/60001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (right):

https://codereview.chromium.org/527153002/diff/60001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:221: window->RemoveObserver(this);
On 2014/09/02 14:10:34, kevers wrote:
> observed_windows_.erase(window);

Done.

[kevers, 2014-09-02 14:32:30]

https://codereview.chromium.org/527153002/diff/80001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/527153002/diff/80001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:221: window->RemoveObserver(this);
observed_windows_.erase(window);

Otherwise RemoveAllObservedWindows could attempt to remove an observer from a
window that has already been destroyed.

[Shu Chen, 2014-09-02 14:33:20]

https://codereview.chromium.org/527153002/diff/80001/ui/keyboard/keyboard_con...
File ui/keyboard/keyboard_controller.cc (left):

https://codereview.chromium.org/527153002/diff/80001/ui/keyboard/keyboard_con...
ui/keyboard/keyboard_controller.cc:221: window->RemoveObserver(this);
On 2014/09/02 14:32:30, kevers wrote:
> observed_windows_.erase(window);
> 
> Otherwise RemoveAllObservedWindows could attempt to remove an observer from a
> window that has already been destroyed.

Done.

[kevers, 2014-09-02 14:43:37]

lgtm

[Shu Chen, 2014-09-02 14:45:07]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-09-02 14:46:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shuchen@chromium.org/527153002/100001

[commit-bot: I haz the power, 2014-09-02 16:34:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-09-02 17:06:34]

Committed patchset #6 (id:100001) as 857e5ffa297c2162e7cc37658bd117b15a60549c

[commit-bot: I haz the power, 2014-09-10 03:19:11]

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