Fix crash closing the Web Share target picker dialog.

Fix crash closing the Web Share target picker dialog.

Was caused by a subtle calling-virtual-method-during-destruction error
resulting in what looks like undefined behaviour (it crashes on Windows
but not Linux).

Refactored the whole WebShareTargetPickerView to contain, rather than
inherit, a TableModel, which is a more sane design.

BUG=689805
Review-Url: https://codereview.chromium.org/2682753004
Cr-Commit-Position: refs/heads/master@{#449166}
Committed: https://chromium.googlesource.com/chromium/src/+/14d6fb7e4ec6fef9464a0122af899d8440a310d3

[Matt Giuca, 2017-02-08 06:24:01]

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

[commit-bot: I haz the power, 2017-02-08 06:24:18]

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

[Matt Giuca, 2017-02-08 06:24:26]

mgiuca@chromium.org changed reviewers:
+ constantina@google.com

[Matt Giuca, 2017-02-08 06:24:26]

Hi Connie, can you do a quick review of this fix?

[Matt Giuca, 2017-02-08 07:50:44]

What, there is a ChromeOS-only warning:

In file included from
../../chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:5:0:
../../chrome/browser/ui/views/webshare/webshare_target_picker_view.h:34:7:
error: 'WebShareTargetPickerView' has a field
'WebShareTargetPickerView::table_model_' whose type uses the anonymous namespace
[-Werror]
 class WebShareTargetPickerView : public views::DialogDelegateView,
       ^

I didn't know that wasn't allowed. Oh well, have to move it out.

[Matt Giuca, 2017-02-08 07:52:00]

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

[commit-bot: I haz the power, 2017-02-08 07:52:16]

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

[Matt Giuca, 2017-02-08 07:56:14]

mgiuca@chromium.org changed reviewers:
+ sky@chromium.org

[Matt Giuca, 2017-02-08 07:56:14]

Hi Scott,

I encountered this crash on Windows (covered by the tests in the next CL) and
felt it was best to refactor this rather than the "quick fix" I mentioned in the
bug https://crbug.com/689805#c2.

[commit-bot: I haz the power, 2017-02-08 08:32:10]

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

[commit-bot: I haz the power, 2017-02-08 08:32:11]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-08 16:58:26]

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right):

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:30:
TargetPickerTableModel(
explicit

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:41: };
DISALLOW...

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:110:
table_->SetModel(nullptr);
I'm ok with the separate model, but couldn't you have also fixed the bug by
leaving the code as is and adding this single line here?

[Matt Giuca, 2017-02-08 23:15:48]

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/webshare/webshare_target_picker_view.cc (right):

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:30:
TargetPickerTableModel(
On 2017/02/08 16:58:25, sky wrote:
> explicit

Done.

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:41: };
On 2017/02/08 16:58:25, sky wrote:
> DISALLOW...

Sorry I'm so bad at remembering this...

https://codereview.chromium.org/2682753004/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/webshare/webshare_target_picker_view.cc:110:
table_->SetModel(nullptr);
On 2017/02/08 16:58:25, sky wrote:
> I'm ok with the separate model, but couldn't you have also fixed the bug by
> leaving the code as is and adding this single line here?

Yes, it would technically be fixed. But we'd still be calling a virtual method
(SetObserver) during the destructor (the call to SetModel) which, while
well-defined, strikes me as confusing and bad. After discussion with some
colleagues, I decided to go this route even though it's a bit more verbose.

See https://crbug.com/689805#c2.

[sky, 2017-02-08 23:58:38]

LGTM

[Matt Giuca, 2017-02-09 00:00:17]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2017-02-09 00:01:05]

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 00:54:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486598417420740,
"parent_rev": "da017dea554304382e19d7ecbec4608eb47a2995", "commit_rev":
"14d6fb7e4ec6fef9464a0122af899d8440a310d3"}

[commit-bot: I haz the power, 2017-02-09 00:55:10]

Description was changed from

==========
Fix crash closing the Web Share target picker dialog.

Was caused by a subtle calling-virtual-method-during-destruction error
resulting in what looks like undefined behaviour (it crashes on Windows
but not Linux).

Refactored the whole WebShareTargetPickerView to contain, rather than
inherit, a TableModel, which is a more sane design.

BUG=689805
==========

to

==========
Fix crash closing the Web Share target picker dialog.

Was caused by a subtle calling-virtual-method-during-destruction error
resulting in what looks like undefined behaviour (it crashes on Windows
but not Linux).

Refactored the whole WebShareTargetPickerView to contain, rather than
inherit, a TableModel, which is a more sane design.

BUG=689805

Review-Url: https://codereview.chromium.org/2682753004
Cr-Commit-Position: refs/heads/master@{#449166}
Committed:
https://chromium.googlesource.com/chromium/src/+/14d6fb7e4ec6fef9464a0122af89...
==========

[commit-bot: I haz the power, 2017-02-09 00:55:11]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/14d6fb7e4ec6fef9464a0122af89...