New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 689805 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocking:
issue 668389



Sign in to add a comment

Windows: Web Share Target picker crashes on close

Project Member Reported by mgiuca@chromium.org, Feb 8 2017

Issue description

Chrome Version: 58.0.3005.5
OS: Windows
Crash ID: 9afb62e5-ee12-470e-8d0b-f366de458e89 (Server ID: dc8788d580000000)

URL (if applicable) where crash occurred:
https://mgiuca.github.io/web-share/demos/share.html

Can you reproduce this crash?
Yes.

What steps will reproduce this crash (or if it's not reproducible,
what were you doing just before the crash)?
(1) Turn on chrome://flags/#enable-experimental-web-platform-features
(2) Visit https://mgiuca.github.io/web-share/demos/share.html
(3) Click Share.
(4) Cancel.

Confusingly, this only happens on Windows (not Linux). ASAN on Linux doesn't give any errors. And the crash has no stack trace associated with it (it won't log to the console when run on a debug build). But we can view the stack trace from the crash server (or WinDBG).

0x5537297a	(chrome.dll -table_view.cc:157 )	views::TableView::~TableView()
0x553728d8	(chrome.dll + 0x014928d8 )	views::TableView::`scalar deleting destructor'(unsigned int)
0x5535909d	(chrome.dll -view.cc:164 )	views::View::~View()
0x55363d7e	(chrome.dll + 0x01483d7e )	views::ScrollView::Viewport::`scalar deleting destructor'(unsigned int)
0x5535909d	(chrome.dll -view.cc:164 )	views::View::~View()
0x55363b4b	(chrome.dll + 0x01483b4b )	views::`anonymous namespace'::ScrollViewWithBorder::`scalar deleting destructor'(unsigned int)
0x5535909d	(chrome.dll -view.cc:164 )	views::View::~View()
0x5535fbe5	(chrome.dll -dialog_delegate.cc:241 )	views::DialogDelegateView::~DialogDelegateView()
0x556ebb2b	(chrome.dll + 0x0180bb2b )	WebShareTargetPickerView::`scalar deleting destructor'(unsigned int)
0x544900ac	(chrome.dll -device_manager_impl.cc:119 )	device::usb::DeviceManagerImpl::WillDestroyUsbService()
0x55355f1d	(chrome.dll -widget.cc:1077 )	views::Widget::OnNativeWidgetDestroyed()
0x553573a2	(chrome.dll -desktop_native_widget_aura.cc:321 )	views::DesktopNativeWidgetAura::OnHostClosed()
0x55397e01	(chrome.dll -hwnd_message_handler.cc:931 )	views::HWNDMessageHandler::OnWndProc(unsigned int,unsigned int,long)
0x5484fcfc	(chrome.dll -window_impl.cc:303 )	gfx::WindowImpl::WndProc(HWND__ *,unsigned int,unsigned int,long)

That line is:

156  if (model_)
157    model_->SetObserver(NULL);

So |model_| is a non-NULL dangling pointer.
 
Tricksy:

- WebShareTargetPickerView's child TableView's |model_| is a pointer back to the WSTPV.
- During WSTPV's destruction, the View is destructed, which deletes its children, including the TableView.
- During ~TableView, it calls model_->SetObserver(NULL) which is a virtual method on the WSTPV which is currently being destroyed.

It's not quite clear why exactly this crashes (especially why it crashes on Windows but not Linux), but a lot of advice on the Internet says "don't call virtual methods during destruction". I will investigate the precise mechanics but it's safe to say that we shouldn't be doing this.

Fix is easy: just call table_->SetModel(nullptr) in ~WebShareTargetPickerView. (Hmmm.... I think technically that will *still* call the SetObserver method during destruction, but earlier. This doesn't crash on Windows, but it might still not be safe?)
OK, had a think.

The "easy fix" is "correct" in that it doesn't trigger undefined behaviour, but it still has a virtual method call on an under-destruction object. (But it's "correct" because it happens directly during the child class's destructor.) But no, we don't want to be in that business.

I will refactor the whole thing, so that WSTPV contains a TableModel, rather than inheriting from TableModel.

Note: https://cs.chromium.org/chromium/src/ui/views/examples/table_example.h does inherit from TableModel but looks like it avoids this issue (but does some dodgy pointer work to avoid it). I prefer to just have a separate object.
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14d6fb7e4ec6fef9464a0122af899d8440a310d3

commit 14d6fb7e4ec6fef9464a0122af899d8440a310d3
Author: mgiuca <mgiuca@chromium.org>
Date: Thu Feb 09 00:54:28 2017

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}

[modify] https://crrev.com/14d6fb7e4ec6fef9464a0122af899d8440a310d3/chrome/browser/ui/views/webshare/webshare_target_picker_view.cc
[modify] https://crrev.com/14d6fb7e4ec6fef9464a0122af899d8440a310d3/chrome/browser/ui/views/webshare/webshare_target_picker_view.h

Comment 4 by mgiuca@chromium.org, Feb 12 2017

Status: Fixed (was: Started)

Sign in to add a comment