cros: Fix initiating wifi connection caused chrome crash

cros: Fix initiating wifi connection caused chrome crash

Changes:
sub_text could be empty and later become non-empty for wifi entries that are switching connection state. In this case, sub_text_label_ will become obsolete, but if we check if (!sub_text_label_) it will not be recreated, which might cause the crash.

BUG=718411
TEST=tested on device that crash is fixed

[Qiang(Joe) Xu, 2017-05-04 17:05:40]

Description was changed from

==========
cros: Fix initiating wifi connection caused chrome crash

BUG=718411
==========

to

==========
cros: Fix initiating wifi connection caused chrome crash

Changes:
sub_text could be empty and later become non-empty for wifi entries that are
switching connection state. In this case, sub_text_label_ will become obsolete,
but if we check if (!sub_text_label_) it will not be recreated, which might
cause the crash.

BUG=718411
==========

[Qiang(Joe) Xu, 2017-05-04 17:05:40]

warx@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Qiang(Joe) Xu, 2017-05-04 17:07:59]

tdanderson@, would you help take a look if this fixes the issue. I don't know
why there is a DCHECK(!sub_text.empty()) here, it should be possible that
sub_text could be empty:
https://cs.chromium.org/chromium/src/ash/system/tray/hover_highlight_view.cc?...

[Qiang(Joe) Xu, 2017-05-04 17:08:29]

Description was changed from

==========
cros: Fix initiating wifi connection caused chrome crash

Changes:
sub_text could be empty and later become non-empty for wifi entries that are
switching connection state. In this case, sub_text_label_ will become obsolete,
but if we check if (!sub_text_label_) it will not be recreated, which might
cause the crash.

BUG=718411
==========

to

==========
cros: Fix initiating wifi connection caused chrome crash

Changes:
sub_text could be empty and later become non-empty for wifi entries that are
switching connection state. In this case, sub_text_label_ will become obsolete,
but if we check if (!sub_text_label_) it will not be recreated, which might
cause the crash.

BUG=718411
TEST=tested on device that crash is fixed
==========

[Qiang(Joe) Xu, 2017-05-04 17:20:21]

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

[commit-bot: I haz the power, 2017-05-04 17:20:56]

Dry run: CQ is trying da patch.

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

[stevenjb, 2017-05-04 17:25:58]

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

[stevenjb, 2017-05-04 17:25:58]

https://codereview.chromium.org/2865473002/diff/1/ash/system/tray/hover_highl...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2865473002/diff/1/ash/system/tray/hover_highl...
ash/system/tray/hover_highlight_view.cc:62: sub_text_label_ =
TrayPopupUtils::CreateDefaultLabel();
You still need the if (!sub_text_label_) test here, otherwise multiple labels
will be added if this is called more than once.

It looks like the change should actually just be:

if (sub_text.empty()) {
  if (sub_text_label_)
    ... remove the label ...
  return;
}

?

[Qiang(Joe) Xu, 2017-05-04 17:37:20]

Patchset #2 (id:20001) has been deleted

[Qiang(Joe) Xu, 2017-05-04 18:52:55]

On 2017/05/04 17:25:58, stevenjb wrote:
>
https://codereview.chromium.org/2865473002/diff/1/ash/system/tray/hover_highl...
> File ash/system/tray/hover_highlight_view.cc (right):
> 
>
https://codereview.chromium.org/2865473002/diff/1/ash/system/tray/hover_highl...
> ash/system/tray/hover_highlight_view.cc:62: sub_text_label_ =
> TrayPopupUtils::CreateDefaultLabel();
> You still need the if (!sub_text_label_) test here, otherwise multiple labels
> will be added if this is called more than once.
> 
> It looks like the change should actually just be:
> 
> if (sub_text.empty()) {
>   if (sub_text_label_)
>     ... remove the label ...
>   return;
> }
> 
> ?

Let me close this issue. I didn't find the root cause. :/ Will delegate it to
mohsen@.