Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView.

Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView.

This CL fixes incorrect handling of destroyed GlobalError |error_| in GlobalErrorBubbleView. The code should now account for cases in which the |error_| is null.

Unit tests have been added to reinforce this, check for access of |error_| and for behavior when |error_| is null.

BUG=672344
Review-Url: https://codereview.chromium.org/2650923002
Cr-Commit-Position: refs/heads/master@{#449727}
Committed: https://chromium.googlesource.com/chromium/src/+/40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b

[CJ, 2017-01-24 01:18:17]

lethalantidote@chromium.org changed reviewers:
+ dahlke@chromium.org, sky@chromium.org, wez@chromium.org

[Wez, 2017-01-24 01:28:11]

Thanks for fixing this!  Some CL description suggestions:

- The CL description focuses on unit-tests, while this CL actually fixes bugs -
describe the bug fixed in the title and mention the unit-test being added in the
description text, after the main bug-fix description. :)
- CL title talks about weak_pointer |error_| without context - specific bug
you're fixing is that GlobalErrorBubbleView did not previously cope correctly
with GlobalError going away while it was active, and you're fixing that.

So I'd suggest changing the title to mention fixing GlobalErrorBubbleView to
cope with GlobalError going away, and updating the description to have a
paragraph briefly describing the fix, and a second paragraph describing the
additional unit-tests.

P.S. Thanks for taking the time to add unit-tests. :D

[CJ, 2017-01-24 01:40:36]

Description was changed from

==========
Provides unit tests checking for valid uses of the weak_pointer |error_|.

This CL adds unit tests that exercise the paths that use |error_| to make sure
the case in which error_ is null is handled. Paths that were not handling
|error_| correctly were updated to do so.

BUG=672344
==========

to

==========
Fixes unchecked access to possibly invalid GlobalError weak_pointer in
GlobalErrorBubbleView.

This CL fixes incorrect handling of weak_ptr GlobalError |error_|. The code
should now account for cases in which the weak_ptr is null

Unit tests have been added to reinforce this, check for access of |error_| and
for behavior when |error_| is null.

BUG=672344
==========

[CJ, 2017-01-24 01:40:46]

On 2017/01/24 01:28:11, Wez wrote:
> Thanks for fixing this!  Some CL description suggestions:
> 
> - The CL description focuses on unit-tests, while this CL actually fixes bugs
-
> describe the bug fixed in the title and mention the unit-test being added in
the
> description text, after the main bug-fix description. :)
> - CL title talks about weak_pointer |error_| without context - specific bug
> you're fixing is that GlobalErrorBubbleView did not previously cope correctly
> with GlobalError going away while it was active, and you're fixing that.
> 
> So I'd suggest changing the title to mention fixing GlobalErrorBubbleView to
> cope with GlobalError going away, and updating the description to have a
> paragraph briefly describing the fix, and a second paragraph describing the
> additional unit-tests.
> 
> P.S. Thanks for taking the time to add unit-tests. :D

Done.

[Wez, 2017-01-24 01:45:55]

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:77: if (error_) {
nit: I'd recommend using an early-exit pattern for these, i.e. if |error_| is
invalid then return a dummy value:

if (error_)
  return base::string16();
return error_->GetBubbleViewTitle();

in this case.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:88:
DCHECK(!image.IsEmpty());
This DCHECK will trivially fail if |error_| is null, so suggest moving it inside
the if body.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:106: DCHECK(error_);
Ooooh, this is a fun one :)

It's good to document the expectation that |error_| be non-null here, however
the de-references below will each DCHECK the same thing, for you - so I'd
suggest not bothering with the DCHECKs, and perhaps just add a one-line comment
at the top of Init() saying e.g. "|error_| is assumed to be valid, and stay
valid, at least until Init() returns."

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:134: DCHECK(error_);
See above :)

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:141:
BubbleDialogDelegateView::UpdateButton(button, type);
nit: Does UpdateButton reference |error_| in some way?

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:161: DCHECK(error_);
As noted above, the de-references below will already DCHECK this.

What is it that guarantees that |error_| is valid here, though? Are these
called-back synchronously from within Init, for example?

If there is no such guarantee then I'd suggest having GetDialogButtons() return
ui::DIALOG_BUTTON_NONE and this return an empty string if |error_| is invalid.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:168: DCHECK(error_);
See GetDialogButtonLabel comment above.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
File chrome/browser/ui/views/global_error_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:108:
EXPECT_CALL(*mock_global_error_with_standard_bubble_, GetBubbleViewTitle());
For calls that are basically just pass-through, I'd suggest just omitting them
from the test, OR doing a single test, e.g. named Basic, which has one
expectation for each call.

Then for the Error-is-null case, I'd again have a single test e.g.
CopesWithNullError and do the set of null-friendly calls there.

In general a test-case should test some specific situation, rather then having
one per API.  e.g. a single API may even have several test cases associated with
it, if it has particularly complicated semantics, each testing different
aspects. But then a single test case may also cover multiple APIs, if that makes
sense as a single test-case.

[CJ, 2017-01-25 01:11:21]

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:77: if (error_) {
On 2017/01/24 01:45:55, Wez wrote:
> nit: I'd recommend using an early-exit pattern for these, i.e. if |error_| is
> invalid then return a dummy value:
> 
> if (error_)
>   return base::string16();
> return error_->GetBubbleViewTitle();
> 
> in this case.

Done. you meant !error right?

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:88:
DCHECK(!image.IsEmpty());
On 2017/01/24 01:45:55, Wez wrote:
> This DCHECK will trivially fail if |error_| is null, so suggest moving it
inside
> the if body.

Done.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:106: DCHECK(error_);
On 2017/01/24 01:45:55, Wez wrote:
> Ooooh, this is a fun one :)
> 
> It's good to document the expectation that |error_| be non-null here, however
> the de-references below will each DCHECK the same thing, for you - so I'd
> suggest not bothering with the DCHECKs, and perhaps just add a one-line
comment
> at the top of Init() saying e.g. "|error_| is assumed to be valid, and stay
> valid, at least until Init() returns."

Done.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:134: DCHECK(error_);
On 2017/01/24 01:45:55, Wez wrote:
> See above :)

Done.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:141:
BubbleDialogDelegateView::UpdateButton(button, type);
On 2017/01/24 01:45:55, Wez wrote:
> nit: Does UpdateButton reference |error_| in some way?

Seems to. Found that through my tests as the StrictMock kept failing because of
unexpected calls to |error_| during UpdateButton.
GetBubbleViewAcceptButtonLabel() and GetBubbleViewCancelButtonLabel() get called
sometime in UpdateButton.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:161: DCHECK(error_);
On 2017/01/24 01:45:55, Wez wrote:
> As noted above, the de-references below will already DCHECK this.
> 
> What is it that guarantees that |error_| is valid here, though? Are these
> called-back synchronously from within Init, for example?
> 
> If there is no such guarantee then I'd suggest having GetDialogButtons()
return
> ui::DIALOG_BUTTON_NONE and this return an empty string if |error_| is invalid.

Done.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:168: DCHECK(error_);
On 2017/01/24 01:45:55, Wez wrote:
> See GetDialogButtonLabel comment above.

Done.

[CJ, 2017-01-25 01:12:14]

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
File chrome/browser/ui/views/global_error_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:108:
EXPECT_CALL(*mock_global_error_with_standard_bubble_, GetBubbleViewTitle());
On 2017/01/24 01:45:55, Wez wrote:
> For calls that are basically just pass-through, I'd suggest just omitting them
> from the test, OR doing a single test, e.g. named Basic, which has one
> expectation for each call.
> 
> Then for the Error-is-null case, I'd again have a single test e.g.
> CopesWithNullError and do the set of null-friendly calls there.
> 
> In general a test-case should test some specific situation, rather then having
> one per API.  e.g. a single API may even have several test cases associated
with
> it, if it has particularly complicated semantics, each testing different
> aspects. But then a single test case may also cover multiple APIs, if that
makes
> sense as a single test-case.

Done.

[Wez, 2017-01-26 18:30:20]

nit: Description still mentions "weak_pointer"; using unix_hacker_style implies
you're talking about a variable name, but you're really talking about the
WeakPtr type.  Similarly where yoy say "weak_ptr", I think you mean WeakPtr.

However, I'd suggest just rewording to express the intended behaviour, which is
that the GlobalErrorBubbleView copes gracefully with the GlobalError having been
destroyed - use of a WeakPtr, and correct checking, to achieve that, is
implementation detail.

[Wez, 2017-01-26 18:34:09]

LGTM w/ a few remaining nits.

Thanks again for fixing this, and adding the test.

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:77: if (error_) {
On 2017/01/25 01:11:21, CJ wrote:
> On 2017/01/24 01:45:55, Wez wrote:
> > nit: I'd recommend using an early-exit pattern for these, i.e. if |error_|
is
> > invalid then return a dummy value:
> > 
> > if (error_)
> >   return base::string16();
> > return error_->GetBubbleViewTitle();
> > 
> > in this case.
> 
> Done. you meant !error right?

D'oh! Yes :D

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:141:
BubbleDialogDelegateView::UpdateButton(button, type);
On 2017/01/25 01:11:21, CJ wrote:
> On 2017/01/24 01:45:55, Wez wrote:
> > nit: Does UpdateButton reference |error_| in some way?
> 
> Seems to. Found that through my tests as the StrictMock kept failing because
of
> unexpected calls to |error_| during UpdateButton.
> GetBubbleViewAcceptButtonLabel() and GetBubbleViewCancelButtonLabel() get
called
> sometime in UpdateButton.

Ahhh! That makes sense - UpdateButton() is causing the BubbleDialogdelegateView
to need to know what the button text should be, and which button(s) to show.

Suggest adding a brief comment to state that UpdateButton() can result in calls
back in to the GlobalErrorBubbleView.

[sky, 2017-01-26 19:06:07]

Can you elaborate when error_ is destroyed? It seems to me there is no point in
continuing to show the bubble once error_ is destroyed. Shouldn't we close the
bubble if error_ is destroyed?

https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.cc:100: // |error_| is assumed
to be valid, and stay valid, at least until Init()
optional: This comment applies to Init(), so I would move it insid the function.

[CJ, 2017-01-26 19:25:07]

Description was changed from

==========
Fixes unchecked access to possibly invalid GlobalError weak_pointer in
GlobalErrorBubbleView.

This CL fixes incorrect handling of weak_ptr GlobalError |error_|. The code
should now account for cases in which the weak_ptr is null

Unit tests have been added to reinforce this, check for access of |error_| and
for behavior when |error_| is null.

BUG=672344
==========

to

==========
Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView.

This CL fixes incorrect handling of destroyed GlobalError |error_| in
GlobalErrorBubbleView. The code should now account for cases in which the
|error_| is null.

Unit tests have been added to reinforce this, check for access of |error_| and
for behavior when |error_| is null.

BUG=672344
==========

[CJ, 2017-01-26 19:25:16]

On 2017/01/26 18:30:20, Wez wrote:
> nit: Description still mentions "weak_pointer"; using unix_hacker_style
implies
> you're talking about a variable name, but you're really talking about the
> WeakPtr type.  Similarly where yoy say "weak_ptr", I think you mean WeakPtr.
> 
> However, I'd suggest just rewording to express the intended behaviour, which
is
> that the GlobalErrorBubbleView copes gracefully with the GlobalError having
been
> destroyed - use of a WeakPtr, and correct checking, to achieve that, is
> implementation detail.

Done.

[CJ, 2017-01-27 00:44:04]

lethalantidote@chromium.org changed reviewers:
+ avi@chromium.org

[CJ, 2017-01-27 00:44:05]

+avi. 

Avi, do you think we could make |error_| a regular ptr. We could null out
|error_| on destruction and handle the cases in which it is null. What do you
think?

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
chrome/browser/ui/views/global_error_bubble_view.cc:141:
BubbleDialogDelegateView::UpdateButton(button, type);
On 2017/01/26 18:34:09, Wez wrote:
> On 2017/01/25 01:11:21, CJ wrote:
> > On 2017/01/24 01:45:55, Wez wrote:
> > > nit: Does UpdateButton reference |error_| in some way?
> > 
> > Seems to. Found that through my tests as the StrictMock kept failing because
> of
> > unexpected calls to |error_| during UpdateButton.
> > GetBubbleViewAcceptButtonLabel() and GetBubbleViewCancelButtonLabel() get
> called
> > sometime in UpdateButton.
> 
> Ahhh! That makes sense - UpdateButton() is causing the
BubbleDialogdelegateView
> to need to know what the button text should be, and which button(s) to show.
> 
> Suggest adding a brief comment to state that UpdateButton() can result in
calls
> back in to the GlobalErrorBubbleView.

Done.

https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view.cc:100: // |error_| is assumed
to be valid, and stay valid, at least until Init()
On 2017/01/26 19:06:07, sky wrote:
> optional: This comment applies to Init(), so I would move it insid the
function.

Done.

[Avi (use Gerrit), 2017-02-03 19:46:28]

GlobalErrorBubbleView::ShouldShowCloseButton already assumes that error_ can be
null.

Otherwise I'm not sure; I did some cleanup on GlobalError but am not an expert
here.

On 2017/01/27 00:44:05, CJ wrote:
> +avi. 
> 
> Avi, do you think we could make |error_| a regular ptr. We could null out
> |error_| on destruction and handle the cases in which it is null. What do you
> think?
> 
>
https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
> File chrome/browser/ui/views/global_error_bubble_view.cc (right):
> 
>
https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
> chrome/browser/ui/views/global_error_bubble_view.cc:141:
> BubbleDialogDelegateView::UpdateButton(button, type);
> On 2017/01/26 18:34:09, Wez wrote:
> > On 2017/01/25 01:11:21, CJ wrote:
> > > On 2017/01/24 01:45:55, Wez wrote:
> > > > nit: Does UpdateButton reference |error_| in some way?
> > > 
> > > Seems to. Found that through my tests as the StrictMock kept failing
because
> > of
> > > unexpected calls to |error_| during UpdateButton.
> > > GetBubbleViewAcceptButtonLabel() and GetBubbleViewCancelButtonLabel() get
> > called
> > > sometime in UpdateButton.
> > 
> > Ahhh! That makes sense - UpdateButton() is causing the
> BubbleDialogdelegateView
> > to need to know what the button text should be, and which button(s) to show.
> > 
> > Suggest adding a brief comment to state that UpdateButton() can result in
> calls
> > back in to the GlobalErrorBubbleView.
> 
> Done.
> 
>
https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/global_error_bubble_view.cc (right):
> 
>
https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/global_error_bubble_view.cc:100: // |error_| is
assumed
> to be valid, and stay valid, at least until Init()
> On 2017/01/26 19:06:07, sky wrote:
> > optional: This comment applies to Init(), so I would move it insid the
> function.
> 
> Done.

[sky, 2017-02-04 22:16:24]

On 2017/02/03 19:46:28, Avi wrote:
> GlobalErrorBubbleView::ShouldShowCloseButton already assumes that error_ can
be
> null.
> 
> Otherwise I'm not sure; I did some cleanup on GlobalError but am not an expert
> here.
> 
> On 2017/01/27 00:44:05, CJ wrote:
> > +avi. 
> > 
> > Avi, do you think we could make |error_| a regular ptr. We could null out
> > |error_| on destruction and handle the cases in which it is null. What do
you
> > think?
> > 
> >
>
https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
> > File chrome/browser/ui/views/global_error_bubble_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/2650923002/diff/1/chrome/browser/ui/views/glo...
> > chrome/browser/ui/views/global_error_bubble_view.cc:141:
> > BubbleDialogDelegateView::UpdateButton(button, type);
> > On 2017/01/26 18:34:09, Wez wrote:
> > > On 2017/01/25 01:11:21, CJ wrote:
> > > > On 2017/01/24 01:45:55, Wez wrote:
> > > > > nit: Does UpdateButton reference |error_| in some way?
> > > > 
> > > > Seems to. Found that through my tests as the StrictMock kept failing
> because
> > > of
> > > > unexpected calls to |error_| during UpdateButton.
> > > > GetBubbleViewAcceptButtonLabel() and GetBubbleViewCancelButtonLabel()
get
> > > called
> > > > sometime in UpdateButton.
> > > 
> > > Ahhh! That makes sense - UpdateButton() is causing the
> > BubbleDialogdelegateView
> > > to need to know what the button text should be, and which button(s) to
show.
> > > 
> > > Suggest adding a brief comment to state that UpdateButton() can result in
> > calls
> > > back in to the GlobalErrorBubbleView.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/global_error_bubble_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/2650923002/diff/20001/chrome/browser/ui/views...
> > chrome/browser/ui/views/global_error_bubble_view.cc:100: // |error_| is
> assumed
> > to be valid, and stay valid, at least until Init()
> > On 2017/01/26 19:06:07, sky wrote:
> > > optional: This comment applies to Init(), so I would move it insid the
> > function.
> > 
> > Done.

*SIGH* I think this is wrong and that we need to clean up the ownership
semantics of GlobalErrors. But I'll approve this so we don't crash.

[sky, 2017-02-04 22:16:31]

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:71: const SkBitmap
CreateBitmap(int width, int height) {
Can you use CreateImage() in image_unittest_util?

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:105: };
private: DISALLOW...

[Wez, 2017-02-06 21:13:46]

On 2017/02/04 22:16:24, sky wrote:
> *SIGH* I think this is wrong and that we need to clean up the ownership
> semantics of GlobalErrors. But I'll approve this so we don't crash.

Perhaps we can follow-up with that? Who is the right person to shed light on the
current ownership, which seems to have "managed" and "unmanaged" cases?

[sky, 2017-02-06 21:25:58]

I'm not sure there is a good owner. I know Sailesh had worked on this
long ago, but he has since moved on.

  -Scott

On Mon, Feb 6, 2017 at 1:13 PM,  <wez@chromium.org> wrote:
> On 2017/02/04 22:16:24, sky wrote:
>> *SIGH* I think this is wrong and that we need to clean up the ownership
>> semantics of GlobalErrors. But I'll approve this so we don't crash.
>
> Perhaps we can follow-up with that? Who is the right person to shed light on
> the
> current ownership, which seems to have "managed" and "unmanaged" cases?
>
>
> https://codereview.chromium.org/2650923002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[CJ, 2017-02-06 23:54:50]

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
On 2017/02/04 22:16:31, sky wrote:
> no (c)

Done. Is this a new thing with the no (c)? (I've only just seen it with the (c),
so just curious).

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:71: const SkBitmap
CreateBitmap(int width, int height) {
On 2017/02/04 22:16:31, sky wrote:
> Can you use CreateImage() in image_unittest_util?

Yup. Done. I think there was something funky going on before that prevented it,
but now it's done.

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:105: };
On 2017/02/04 22:16:31, sky wrote:
> private: DISALLOW...

Done.

[Avi (use Gerrit), 2017-02-07 00:11:46]

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/global_error_bubble_view_unittest.cc (right):

https://codereview.chromium.org/2650923002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/global_error_bubble_view_unittest.cc:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
On 2017/02/06 23:54:50, CJ wrote:
> On 2017/02/04 22:16:31, sky wrote:
> > no (c)
> 
> Done. Is this a new thing with the no (c)? (I've only just seen it with the
(c),
> so just curious).

It's been that way for years officially, but we never did a cleanup so it's a
mix of styles.

[sky, 2017-02-07 00:30:38]

LGTM

[CJ, 2017-02-07 00:55:56]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2017-02-07 00:55:56]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2650923002/#ps60001
(title: "Addresses sky's #18 comments.")

[commit-bot: I haz the power, 2017-02-07 00:56:34]

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, 2017-02-07 01:30:28]

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

[commit-bot: I haz the power, 2017-02-07 01:30:29]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[CJ, 2017-02-07 21:53:53]

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

[commit-bot: I haz the power, 2017-02-07 21:54:42]

Dry run: 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, 2017-02-07 22:21:35]

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

[commit-bot: I haz the power, 2017-02-07 22:21:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[CJ, 2017-02-08 00:12:15]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2017-02-08 00:12:15]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2650923002/#ps100001
(title: "Updates BUILD to correct compilation issues.")

[commit-bot: I haz the power, 2017-02-08 00:12:58]

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, 2017-02-08 01:22:37]

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

[commit-bot: I haz the power, 2017-02-08 01:22:38]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[CJ, 2017-02-09 20:26:02]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2017-02-09 20:26:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2650923002/#ps160001
(title: "Fix more leaks.")

[commit-bot: I haz the power, 2017-02-09 20:26:46]

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, 2017-02-09 21:17:55]

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

[commit-bot: I haz the power, 2017-02-09 21:17:56]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[CJ, 2017-02-09 21:29:00]

The CQ bit was checked by lethalantidote@chromium.org

[commit-bot: I haz the power, 2017-02-09 21:29:35]

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, 2017-02-09 22:09:32]

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

[commit-bot: I haz the power, 2017-02-09 22:09:33]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dcheng, 2017-02-10 19:12:51]

On 2017/02/09 22:09:33, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

OK, so this is because we get pass by-value semantics inside gmock. LatencyInfo
has stricter alignment requirement (due to its embedded SmallMap) than the x86
calling convention provides, so MSVC warns.

I think the only workaround here is to write your own test fake instead of using
gmock

[CJ, 2017-02-10 19:28:37]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2017-02-10 19:28:37]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2650923002/#ps200001
(title: "Use fake instead of mock button listener")

[commit-bot: I haz the power, 2017-02-10 19:29:37]

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, 2017-02-10 20:53:05]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486754917097060,
"parent_rev": "73ac839f6e9eb6934f00d61fb3793d15632d7c9d", "commit_rev":
"40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b"}

[commit-bot: I haz the power, 2017-02-10 20:53:37]

Description was changed from

==========
Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView.

This CL fixes incorrect handling of destroyed GlobalError |error_| in
GlobalErrorBubbleView. The code should now account for cases in which the
|error_| is null.

Unit tests have been added to reinforce this, check for access of |error_| and
for behavior when |error_| is null.

BUG=672344
==========

to

==========
Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView.

This CL fixes incorrect handling of destroyed GlobalError |error_| in
GlobalErrorBubbleView. The code should now account for cases in which the
|error_| is null.

Unit tests have been added to reinforce this, check for access of |error_| and
for behavior when |error_| is null.

BUG=672344

Review-Url: https://codereview.chromium.org/2650923002
Cr-Commit-Position: refs/heads/master@{#449727}
Committed:
https://chromium.googlesource.com/chromium/src/+/40d0dc39f4bfaf7bdf919ce98dfd...
==========

[commit-bot: I haz the power, 2017-02-10 20:53:39]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/40d0dc39f4bfaf7bdf919ce98dfd...