Avert crash in ExtensionInstalledBubbleView::GetWindowTitle triggered

Avert crash in ExtensionInstalledBubbleView::GetWindowTitle triggered
by screen readers.

BUG=648280
Committed: https://crrev.com/6538bba7706e6266ea33e83a49710fabfb733155
Cr-Commit-Position: refs/heads/master@{#420504}

[Evan Stade, 2016-09-20 19:47:09]

estade@chromium.org changed reviewers:
+ dmazzoni@chromium.org, rdevlin.cronin@chromium.org

[Devlin, 2016-09-20 20:15:44]

https://codereview.chromium.org/2355963002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/2355963002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:173:
base::string16 ExtensionInstalledBubbleView::GetWindowTitle() const {
Is there any way we could detect the view is closed, and if so, would this be
better handled higher up the chain (e.g. DialogDelegateView, View, etc)?  This
seems like something that could happen to any view rather than just this one.

[dmazzoni, 2016-09-20 21:15:38]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-09-20 21:15:39]

lgtm

[dmazzoni, 2016-09-20 21:15:48]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2016-09-20 21:17:16]

On 2016/09/20 at 20:15:44, rdevlin.cronin wrote:
>
https://codereview.chromium.org/2355963002/diff/1/chrome/browser/ui/views/ext...
> File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):
> 
>
https://codereview.chromium.org/2355963002/diff/1/chrome/browser/ui/views/ext...
> chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:173:
base::string16 ExtensionInstalledBubbleView::GetWindowTitle() const {
> Is there any way we could detect the view is closed, and if so, would this be
better handled higher up the chain (e.g. DialogDelegateView, View, etc)?  This
seems like something that could happen to any view rather than just this one.

I think it's coming from DialogDelegateView::GetAccessibleState.

It seems reasonable to me for that to return the empty string there if the
window is closed.

However, it also seems safer to have this fix as well.

[Devlin, 2016-09-20 21:34:39]

On 2016/09/20 21:17:16, dmazzoni wrote:
> On 2016/09/20 at 20:15:44, rdevlin.cronin wrote:
> >
>
https://codereview.chromium.org/2355963002/diff/1/chrome/browser/ui/views/ext...
> > File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2355963002/diff/1/chrome/browser/ui/views/ext...
> > chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:173:
> base::string16 ExtensionInstalledBubbleView::GetWindowTitle() const {
> > Is there any way we could detect the view is closed, and if so, would this
be
> better handled higher up the chain (e.g. DialogDelegateView, View, etc)?  This
> seems like something that could happen to any view rather than just this one.
> 
> I think it's coming from DialogDelegateView::GetAccessibleState.
> 
> It seems reasonable to me for that to return the empty string there if the
> window is closed.
> 
> However, it also seems safer to have this fix as well.

Would hooking into GetAccessibleState() allow us to just [D]CHECK(controller_)
here instead?  IMO, that might be preferable since it takes out a lot of the
ambiguity.

[Evan Stade, 2016-09-21 16:52:52]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[Evan Stade, 2016-09-21 16:52:54]

For most dialogs, GetWindowTitle will work even if the widget is closing because
it's just looking up a bundled string (IDS_...) so calling the function wouldn't
be a problem. I don't know why JAWS is calling this function at this point. Is
it trying to notify the user that window XXX closed? If so, it seems like we
should do our best to return a real string when we can.

Nevertheless, in the latest patchset I've fixed it further up the chain. I don't
know if this actually averts the crash. I suspect it does, but I can't manually
verify.

[commit-bot: I haz the power, 2016-09-21 16:53:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-09-21 17:08:56]

On 2016/09/21 16:52:54, Evan Stade wrote:
> For most dialogs, GetWindowTitle will work even if the widget is closing
because
> it's just looking up a bundled string (IDS_...) so calling the function
wouldn't
> be a problem. I don't know why JAWS is calling this function at this point. Is
> it trying to notify the user that window XXX closed? If so, it seems like we
> should do our best to return a real string when we can.
> 
> Nevertheless, in the latest patchset I've fixed it further up the chain. I
don't
> know if this actually averts the crash. I suspect it does, but I can't
manually
> verify.

This seems reasonable to me.  I'll let dmazzoni comment on whether this should
be sufficient (it looks like it should be?), but you'll need a views/ reviewer
now.

[dmazzoni, 2016-09-21 17:09:56]

lgtm, Nektarios or I can verify when this lands.

[Evan Stade, 2016-09-21 17:14:33]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org

[Evan Stade, 2016-09-21 17:14:33]

+sadrul for owner review

[commit-bot: I haz the power, 2016-09-21 17:45:15]

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

[commit-bot: I haz the power, 2016-09-21 17:45:16]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-09-22 01:44:28]

I would've gone with the first fix (i.e. patchset 1). But this lgtm too

[dmazzoni, 2016-09-22 07:48:40]

Want to compromise on both?

I'd always prefer not to leave a public or protected method that you
know will crash if it's called when the object is in a particular state.
Otherwise this problem could recur in some other way in the future.

[Evan Stade, 2016-09-22 14:37:43]

On 2016/09/22 07:48:40, dmazzoni wrote:
> Want to compromise on both?
> 
> I'd always prefer not to leave a public or protected method that you
> know will crash if it's called when the object is in a particular state.
> Otherwise this problem could recur in some other way in the future.

But handling the null case implies that it is sometimes called when the object
is in that state, which shouldn't be the case. I'd rather DCHECK if anything.

[dmazzoni, 2016-09-22 16:45:38]

On 2016/09/22 at 14:37:43, estade wrote:
> On 2016/09/22 07:48:40, dmazzoni wrote:
> > Want to compromise on both?
> > 
> > I'd always prefer not to leave a public or protected method that you
> > know will crash if it's called when the object is in a particular state.
> > Otherwise this problem could recur in some other way in the future.
> 
> But handling the null case implies that it is sometimes called when the object
is in that state, which shouldn't be the case. I'd rather DCHECK if anything.

Adding a DCHECK sounds good to me.

I didn't mean to keep drawing this out. lgtm with or without the DCHECK

[Evan Stade, 2016-09-22 22:50:01]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-22 22:50:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 23:47:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-22 23:53:29]

Description was changed from

==========
Avert crash in ExtensionInstalledBubbleView::GetWindowTitle triggered
by screen readers.

BUG=648280
==========

to

==========
Avert crash in ExtensionInstalledBubbleView::GetWindowTitle triggered
by screen readers.

BUG=648280

Committed: https://crrev.com/6538bba7706e6266ea33e83a49710fabfb733155
Cr-Commit-Position: refs/heads/master@{#420504}
==========

[commit-bot: I haz the power, 2016-09-22 23:53:30]

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