Make new WebUSB implementation more resilient to garbage collection.

Make new WebUSB implementation more resilient to garbage collection.

This patch fixes a couple logic errors on USBDevice cleanup: assuming
the contextDestroyed() was already called and not clearing the request
list on connection failure.

We then attempt to fix the failures reported in issue 605487 by using
WeakPersistenThisPointer when registering the connection error
callbacks.

BUG=605487
Committed: https://crrev.com/d1174ce5ac7e0b7c3300e5607e7869327ec0669f
Cr-Commit-Position: refs/heads/master@{#389610}

[Reilly Grant (use Gerrit), 2016-04-22 18:02:37]

reillyg@chromium.org changed reviewers:
+ rockot@chromium.org

[Reilly Grant (use Gerrit), 2016-04-22 18:02:38]

Ken, please take a look.

[Ken Rockot(use gerrit already), 2016-04-22 19:52:02]

Hmm. After examining the bindings more closely, I'm pretty well convinced that
all these weak ptrs are redundant. If you are able to repro a crash, could you
get a stack trace to verify where specifically it's happening?

[Reilly Grant (use Gerrit), 2016-04-22 19:59:46]

On 2016/04/22 at 19:52:02, rockot wrote:
> Hmm. After examining the bindings more closely, I'm pretty well convinced that
all these weak ptrs are redundant. If you are able to repro a crash, could you
get a stack trace to verify where specifically it's happening?

I can reproduce the crash locally and get something similar to the bug:

	<lambda_e194cc53e1d82eec82c8f0b881fa2420>::operator() [0x00007FF86585EF95+69]
(d:\src\chromium\src\third_party\webkit\source\modules\webusb\usbdevice.cpp:106)
	mojo::internal::Router::OnConnectionError [0x00007FF8659AF3D5+297]
(d:\src\chromium\src\mojo\public\cpp\bindings\lib\router.cc:307)
	mojo::internal::Connector::HandleError [0x00007FF8659A2128+276]
(d:\src\chromium\src\mojo\public\cpp\bindings\lib\connector.cc:352)
	mojo::internal::Connector::OnHandleReadyInternal [0x00007FF8659A21C4+136]
(d:\src\chromium\src\mojo\public\cpp\bindings\lib\connector.cc:235)
	mojo::Watcher::OnHandleReady [0x00007FF8659B2975+201]
(d:\src\chromium\src\mojo\public\cpp\system\watcher.cc:119)
	base::internal::InvokeHelper<1,void,base::internal::RunnableAdapter<void
(__cdecl mojo::Watcher::*)(unsigned int) __ptr64>
>::MakeItSo<base::WeakPtr<mojo::Watcher>,unsigned int const & __ptr64>
[0x00007FF8659B2499+81] (d:\src\chromium\src\base\bind_internal.h:334)
	base::internal::Invoker<base::IndexSequence<0,1>,base::internal::BindState<base::internal::RunnableAdapter<void
(__cdecl mojo::Watcher::*)(unsigned int) __ptr64>,void __cdecl(mojo::Watcher *
__ptr64,unsigned int),base::WeakPtr<mojo::Watcher> & __ptr64,uns
[0x00007FF8659B29CF+55] (d:\src\chromium\src\base\bind_internal.h:375)
	base::debug::TaskAnnotator::RunTask [0x00007FF87FBE386C+316]
(d:\src\chromium\src\base\debug\task_annotator.cc:53)
	scheduler::TaskQueueManager::ProcessTaskFromWorkQueue [0x00007FF8813EA43A+826]
(d:\src\chromium\src\components\scheduler\base\task_queue_manager.cc:293)
	scheduler::TaskQueueManager::DoWork [0x00007FF8813E9834+512]
(d:\src\chromium\src\components\scheduler\base\task_queue_manager.cc:201)
	base::internal::InvokeHelper<1,void,base::internal::RunnableAdapter<void
(__cdecl scheduler::TaskQueueManager::*)(base::TimeTicks,bool) __ptr64>
>::MakeItSo<base::WeakPtr<scheduler::TaskQueueManager>,base::TimeTicks const &
__ptr64,bool const & __ptr64> [0x00007FF8813E8229+93]
(d:\src\chromium\src\base\bind_internal.h:334)
	base::internal::Invoker<base::IndexSequence<0,1,2>,base::internal::BindState<base::internal::RunnableAdapter<void
(__cdecl scheduler::TaskQueueManager::*)(base::TimeTicks,bool) __ptr64>,void
__cdecl(scheduler::TaskQueueManager * __ptr64,base::TimeTicks,bo
[0x00007FF8813EA727+59] (d:\src\chromium\src\base\bind_internal.h:375)
	base::debug::TaskAnnotator::RunTask [0x00007FF87FBE386C+316]
(d:\src\chromium\src\base\debug\task_annotator.cc:53)
	base::MessageLoop::RunTask [0x00007FF87FC114FC+876]
(d:\src\chromium\src\base\message_loop\message_loop.cc:480)
	base::MessageLoop::DoWork [0x00007FF87FC106D3+755]
(d:\src\chromium\src\base\message_loop\message_loop.cc:601)
	base::MessagePumpDefault::Run [0x00007FF87FC12C18+296]
(d:\src\chromium\src\base\message_loop\message_pump_default.cc:33)
	base::RunLoop::Run [0x00007FF87FC420BE+46]
(d:\src\chromium\src\base\run_loop.cc:36)
	base::MessageLoop::Run [0x00007FF87FC1109B+107]
(d:\src\chromium\src\base\message_loop\message_loop.cc:296)
	content::RendererMain [0x00007FF873EB12A4+1540]
(d:\src\chromium\src\content\renderer\renderer_main.cc:220)
	content::RunNamedProcessTypeMain [0x00007FF873FBD42B+267]
(d:\src\chromium\src\content\app\content_main_runner.cc:398)
	content::ContentMainRunnerImpl::Run [0x00007FF873FBD2DF+315]
(d:\src\chromium\src\content\app\content_main_runner.cc:742)
	content::ContentMain [0x00007FF873FBC648+48]
(d:\src\chromium\src\content\app\content_main.cc:20)
	wWinMain [0x00007FF65CFB4F33+83]
(d:\src\chromium\src\content\shell\app\shell_main.cc:33)
	__scrt_common_main_seh [0x00007FF65D7E1EFE+286]
(f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:255)
	BaseThreadInitThunk [0x00007FF8980B13D2+34]
	RtlUserThreadStart [0x00007FF8987954E4+52]

[Reilly Grant (use Gerrit), 2016-04-22 20:00:53]

Line 106 in USBDevice.cpp is:

  m_device.reset();

So I'm guessing that |this| is an invalid pointer but when I put a printf in the
destructor it doesn't look like it's been called yet.

[Reilly Grant (use Gerrit), 2016-04-22 20:26:33]

This patch does appear to fix the crashing tests but the stack trace appears to
indicate that the USBDevice destructor is not being called. That's not right
because USBDevice is GarbageCollectedFinalized.

[esprehn, 2016-04-23 00:54:02]

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

[esprehn, 2016-04-23 00:54:02]

Hmm, I feel like the amount of manual oilpan stuff going on in modules and
around mojo is bad. I think we need to get more support libraries or something

https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webusb/USB.cpp (right):

https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webusb/USB.cpp:191: for
(ScriptPromiseResolver* resolver : m_deviceManagerRequests) {
seems like this needs to be more centralized, doing this in every subsystem is
bad.

[Reilly Grant (use Gerrit), 2016-04-24 23:11:14]

reillyg@chromium.org changed reviewers:
+ haraken@chromium.org

[Reilly Grant (use Gerrit), 2016-04-24 23:11:15]

Adding haraken@ for help understanding Oilpan semantics.

[haraken, 2016-04-25 08:47:04]

haraken@chromium.org changed reviewers:
+ hiroshige@chromium.org

[haraken, 2016-04-25 08:47:05]

The WeakPersistentThisPointer is nasty but it will be gone in hiroshige's
refactoring. So I'm okay with it.

https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):

https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webusb/USBDevice.cpp:103:
m_device.set_connection_error_handler(createBaseCallback(bind(&USBDevice::onConnectionError,
WeakPersistentThisPointer<USBDevice>(this))));

Just to confirm: Your intention is that the error callback should not be called
if the USBDevice object is already garbage-collected when the error callback is
about to run, right?

I'm curious why you cannot just use a Persistent handle (implicitly created by
WTF::bind when you pass the |this| pointer).

[Reilly Grant (use Gerrit), 2016-04-25 15:09:00]

On 2016/04/25 at 08:47:05, haraken wrote:
> The WeakPersistentThisPointer is nasty but it will be gone in hiroshige's
refactoring. So I'm okay with it.
> 
>
https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):
> 
>
https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/webusb/USBDevice.cpp:103:
m_device.set_connection_error_handler(createBaseCallback(bind(&USBDevice::onConnectionError,
WeakPersistentThisPointer<USBDevice>(this))));
> 
> Just to confirm: Your intention is that the error callback should not be
called if the USBDevice object is already garbage-collected when the error
callback is about to run, right?

Yes. The USBDevice destructor should unregister the callback (by destroying the
Mojo InterfacePointer m_device) but that doesn't appear the be happening. That
implies that either the destructor is not being called or that the event loop is
running between when the memory for |this| is invalidated and the destructor is
run.
 
> I'm curious why you cannot just use a Persistent handle (implicitly created by
WTF::bind when you pass the |this| pointer).

These specific callbacks should not prevent the object from being garbage
collected as they do not reflect any actions taken by scripts. The connection
error callback is simply the way for Mojo to tell the object that the connection
has closed. The GetDeviceChanges callback is used to raise events. Both will
always be active for as long as the object exists and so using a Persistent
handle would cause a reference cycle Oilpan can't detect.

[haraken, 2016-04-25 15:30:48]

On 2016/04/25 15:09:00, Reilly Grant wrote:
> On 2016/04/25 at 08:47:05, haraken wrote:
> > The WeakPersistentThisPointer is nasty but it will be gone in hiroshige's
> refactoring. So I'm okay with it.
> > 
> >
>
https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/webusb/USBDevice.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1912223002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/webusb/USBDevice.cpp:103:
>
m_device.set_connection_error_handler(createBaseCallback(bind(&USBDevice::onConnectionError,
> WeakPersistentThisPointer<USBDevice>(this))));
> > 
> > Just to confirm: Your intention is that the error callback should not be
> called if the USBDevice object is already garbage-collected when the error
> callback is about to run, right?
> 
> Yes. The USBDevice destructor should unregister the callback (by destroying
the
> Mojo InterfacePointer m_device) but that doesn't appear the be happening. That
> implies that either the destructor is not being called or that the event loop
is
> running between when the memory for |this| is invalidated and the destructor
is
> run.
>  
> > I'm curious why you cannot just use a Persistent handle (implicitly created
by
> WTF::bind when you pass the |this| pointer).
> 
> These specific callbacks should not prevent the object from being garbage
> collected as they do not reflect any actions taken by scripts. The connection
> error callback is simply the way for Mojo to tell the object that the
connection
> has closed. The GetDeviceChanges callback is used to raise events. Both will
> always be active for as long as the object exists and so using a Persistent
> handle would cause a reference cycle Oilpan can't detect.

Thanks for the clarification. LGTM.

[Ken Rockot(use gerrit already), 2016-04-25 15:39:17]

OK - LGTM but haraken can you please help us understand how this is possible?
Internally the bound Mojo bindings object uses a base::WeakPtrFactory and
base::Callback bound to a WeakPtr - so as soon as the WeakPtrFactory is
invalidated on destruction, it becomes be impossible for the bound method to
run.

The stack traces look like a bound method is running after |this| has been
deleted. The only explanation I've come up with is that |this| is deleted
without the destructor having run, because the internal WeakPtr must have never
been invalidated.

Reilly I see USBDevice is GarbageCollectedFinalized, but USB is not. Wouldn't
this be a problem?

[Reilly Grant (use Gerrit), 2016-04-25 15:44:08]

On 2016/04/25 at 15:39:17, rockot wrote:
> Reilly I see USBDevice is GarbageCollectedFinalized, but USB is not. Wouldn't
this be a problem?

USB inherits it from EventTarget.

[Reilly Grant (use Gerrit), 2016-04-25 18:20:29]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-04-25 18:20:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1912223002/#ps20001
(title: "Rebased and reverted TestExpectations changes.")

[commit-bot: I haz the power, 2016-04-25 18:20:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1912223002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1912223002/20001

[haraken, 2016-04-25 19:15:45]

On 2016/04/25 15:39:17, Ken Rockot wrote:
> OK - LGTM but haraken can you please help us understand how this is possible?
> Internally the bound Mojo bindings object uses a base::WeakPtrFactory and
> base::Callback bound to a WeakPtr - so as soon as the WeakPtrFactory is
> invalidated on destruction, it becomes be impossible for the bound method to
> run.
> 
> The stack traces look like a bound method is running after |this| has been
> deleted. The only explanation I've come up with is that |this| is deleted
> without the destructor having run, because the internal WeakPtr must have
never
> been invalidated.
> 
> Reilly I see USBDevice is GarbageCollectedFinalized, but USB is not. Wouldn't
> this be a problem?

That's a good question. Reilly, how will this CL fix the crash?

I don't think WeakPersistenThisPointer is not related to the crash. If you pass
|this|, it will implicitly create a Persistent handle to the USB object, so the
USB object shouldn't get collected until the callback runs.

[Reilly Grant (use Gerrit), 2016-04-25 19:19:41]

On 2016/04/25 at 19:15:45, haraken wrote:
> On 2016/04/25 15:39:17, Ken Rockot wrote:
> > OK - LGTM but haraken can you please help us understand how this is
possible?
> > Internally the bound Mojo bindings object uses a base::WeakPtrFactory and
> > base::Callback bound to a WeakPtr - so as soon as the WeakPtrFactory is
> > invalidated on destruction, it becomes be impossible for the bound method to
> > run.
> > 
> > The stack traces look like a bound method is running after |this| has been
> > deleted. The only explanation I've come up with is that |this| is deleted
> > without the destructor having run, because the internal WeakPtr must have
never
> > been invalidated.
> > 
> > Reilly I see USBDevice is GarbageCollectedFinalized, but USB is not.
Wouldn't
> > this be a problem?
> 
> That's a good question. Reilly, how will this CL fix the crash?

It fixes the crash because the WeakPersistentThisPointer prevents the callback
from being called after the object has been destroyed. In both cases the objects
are GarbageCollectedFinalized so by the rules of the mojo::InterfacePtr this
call is coming from the callback shouldn't be called to begin with but there is
something Ken and I don't understand about when destructors are called in Oilpan
managed objects.

> I don't think WeakPersistenThisPointer is not related to the crash. If you
pass |this|, it will implicitly create a Persistent handle to the USB object, so
the USB object shouldn't get collected until the callback runs.

The existing code used a C++ lambda, not a WTF::Function so |this| was a raw
pointer, not Persistent.

[commit-bot: I haz the power, 2016-04-25 20:09:36]

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

[commit-bot: I haz the power, 2016-04-25 20:09:39]

Failed to commit the patch.

[commit-bot: I haz the power, 2016-04-25 20:09:53]

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

[Reilly Grant (use Gerrit), 2016-04-25 20:31:02]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-04-25 20:31:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1912223002/#ps40001
(title: "Rebased again.")

[commit-bot: I haz the power, 2016-04-25 20:31:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1912223002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1912223002/40001

[esprehn, 2016-04-25 20:47:25]

I think the fix here is to use WTF::bind, we shouldn't be using C++ lambda in
places where the lifetimes it produces are wrong.

[Reilly Grant (use Gerrit), 2016-04-25 21:03:09]

On 2016/04/25 at 20:47:25, esprehn wrote:
> I think the fix here is to use WTF::bind, we shouldn't be using C++ lambda in
places where the lifetimes it produces are wrong.

And the reason that the C++ lambda produces an incorrect lifetime is that
sweeping is lazy and so the message loop may run in between the object being
marked unreachable and it being destroyed. Otherwise the destruction of the Mojo
InterfacePtrs held as members of USB and USBDevice would make the raw pointer
lifetime correct.

[commit-bot: I haz the power, 2016-04-25 23:33:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-25 23:34:34]

Description was changed from

==========
Make new WebUSB implementation more resilient to garbage collection.

This patch fixes a couple logic errors on USBDevice cleanup: assuming
the contextDestroyed() was already called and not clearing the request
list on connection failure.

We then attempt to fix the failures reported in issue 605487 by using
WeakPersistenThisPointer when registering the connection error
callbacks.

BUG=605487
==========

to

==========
Make new WebUSB implementation more resilient to garbage collection.

This patch fixes a couple logic errors on USBDevice cleanup: assuming
the contextDestroyed() was already called and not clearing the request
list on connection failure.

We then attempt to fix the failures reported in issue 605487 by using
WeakPersistenThisPointer when registering the connection error
callbacks.

BUG=605487

Committed: https://crrev.com/d1174ce5ac7e0b7c3300e5607e7869327ec0669f
Cr-Commit-Position: refs/heads/master@{#389610}
==========

[commit-bot: I haz the power, 2016-04-25 23:34:35]

Patchset 3 (id:??) landed as
https://crrev.com/d1174ce5ac7e0b7c3300e5607e7869327ec0669f
Cr-Commit-Position: refs/heads/master@{#389610}

[sof, 2016-04-26 05:28:09]

On 2016/04/25 21:03:09, Reilly Grant wrote:
> On 2016/04/25 at 20:47:25, esprehn wrote:
> > I think the fix here is to use WTF::bind, we shouldn't be using C++ lambda
in
> places where the lifetimes it produces are wrong.
> 
> And the reason that the C++ lambda produces an incorrect lifetime is that
> sweeping is lazy and so the message loop may run in between the object being
> marked unreachable and it being destroyed. Otherwise the destruction of the
Mojo
> InterfacePtrs held as members of USB and USBDevice would make the raw pointer
> lifetime correct.

Which could have been addressed by making the objects be eagerly finalized, but
"weakifiying" the callbacks as you did here works just as well, if not better.

[haraken, 2016-04-26 07:08:09]

On 2016/04/26 05:28:09, sof wrote:
> On 2016/04/25 21:03:09, Reilly Grant wrote:
> > On 2016/04/25 at 20:47:25, esprehn wrote:
> > > I think the fix here is to use WTF::bind, we shouldn't be using C++ lambda
> in
> > places where the lifetimes it produces are wrong.
> > 
> > And the reason that the C++ lambda produces an incorrect lifetime is that
> > sweeping is lazy and so the message loop may run in between the object being
> > marked unreachable and it being destroyed. Otherwise the destruction of the
> Mojo
> > InterfacePtrs held as members of USB and USBDevice would make the raw
pointer
> > lifetime correct.
> 
> Which could have been addressed by making the objects be eagerly finalized,
but
> "weakifiying" the callbacks as you did here works just as well, if not better.

C++ lambda functions can cause this kind of issue, so I think we should forbid
C++ lambda functions and instead use WTF::Function + WTF::bind for binding
callback functions.