Potential fix for browser crash while ChromeVox is on.

Potential fix for browser crash while ChromeVox is on.

BUG=512000
TEST=navigate to the launcher without and then with ChromeVox on. No crashes.

Committed: https://crrev.com/22786b9f14a49d1bb0ddacbd9a9e343e482c23c9
Cr-Commit-Position: refs/heads/master@{#339681}

[David Tseng, 2015-07-20 22:25:04]

dtseng@chromium.org changed reviewers:
+ dmazzoni@chromium.org, plundblad@chromium.org

[David Tseng, 2015-07-20 22:26:37]

Please take a look; either of you can do the review.

[dmazzoni, 2015-07-20 22:40:19]

The OnWindowDestroying change makes sense.

Why the change to OnNativeFocusChanged to unregister and register an observer?
Does it really crash without that, and if so can we better understand why?

[David Tseng, 2015-07-20 23:04:11]

On Mon, Jul 20, 2015 at 3:40 PM, <dmazzoni@chromium.org> wrote:

> The OnWindowDestroying change makes sense.
>
> Why the change to OnNativeFocusChanged to unregister and register an
> observer?
> Does it really crash without that, and if so can we better understand why?
>
>
AddObserver calls NOTREACHED if you attempt to add the same observer more
than once.



> https://codereview.chromium.org/1245003002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-07-21 15:53:53]

If I understand, you want to add this as an observer when focus changes, but not
if we're already an observer.

I think you just want to call HasObserver to check instead of calling Remove and
then Add. lgtm if you make that change.

[David Tseng, 2015-07-21 16:09:38]

On Tue, Jul 21, 2015 at 8:53 AM, <dmazzoni@chromium.org> wrote:

> If I understand, you want to add this as an observer when focus changes,
> but not
> if we're already an observer.
>
> I think you just want to call HasObserver to check instead of calling
> Remove and
> then Add. lgtm if you make that change.
>
> Made this change. Hopefully, this helps things. I am not sure this cl
fixes the issue because I can still get thigns to stop responding with
ChromeVox and am pretty sure it's timing related. I can't get a debug build
to crash. If you get the chance, could you try with a mouse (as specified
with the original repo)?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-07-21 16:12:45]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2015-07-21 16:12:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1245003002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-07-21 16:13:02]

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

[commit-bot: I haz the power, 2015-07-21 17:25:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-07-21 17:26:37]

Patchset 2 (id:??) landed as
https://crrev.com/22786b9f14a49d1bb0ddacbd9a9e343e482c23c9
Cr-Commit-Position: refs/heads/master@{#339681}

[David Tseng, 2015-07-22 16:20:10]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1247373002/ by dtseng@chromium.org.

The reason for reverting is: Causing crash: crbug.com/512837.