Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Instead of subclassing the stubClass type of the Mojo service being
implemented we should call connection.bindHandleToStub to create a stub
and then set our implementation as its delegate. The existing code
instead unintentionally overrode the generated stub method bodies which
was mostly harmless but broke passing interface pointers.

While making this change it was discovered that if the DeviceManager
connection is closed a call to requestDevices() would still attempt to
access m_deviceManager.

BUG=None

Committed: https://crrev.com/9b73f1d2dcf4a5c7669fafffb8e670b5d421e4f8
Cr-Commit-Position: refs/heads/master@{#392190}

[Reilly Grant (use Gerrit), 2016-05-04 00:16:41]

Description was changed from

==========
Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Instead of subclassing the stubClass type of the Mojo service being
implemented we should call connection.bindHandleToStub to create a stub
and then set our implementation as its delegate. Without this code in
the generates stub method bodies is not run.

BUG=None
==========

to

==========
Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Instead of subclassing the stubClass type of the Mojo service being
implemented we should call connection.bindHandleToStub to create a stub
and then set our implementation as its delegate. Without this code in
the generates stub method bodies is not run.

While making this change it was discovered that if the DeviceManager
connection is closed a call to requestDevices() would still attempt to
access m_deviceManager.

BUG=None
==========

[Reilly Grant (use Gerrit), 2016-05-04 00:16:41]

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

[Reilly Grant (use Gerrit), 2016-05-04 00:24:30]

Please take a look.

[Reilly Grant (use Gerrit), 2016-05-04 00:26:17]

Description was changed from

==========
Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Instead of subclassing the stubClass type of the Mojo service being
implemented we should call connection.bindHandleToStub to create a stub
and then set our implementation as its delegate. Without this code in
the generates stub method bodies is not run.

While making this change it was discovered that if the DeviceManager
connection is closed a call to requestDevices() would still attempt to
access m_deviceManager.

BUG=None
==========

to

==========
Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Instead of subclassing the stubClass type of the Mojo service being
implemented we should call connection.bindHandleToStub to create a stub
and then set our implementation as its delegate. The existing code
instead unintentionally overrode the generated stub method bodies which
was mostly harmless but broke passing interface pointers.

While making this change it was discovered that if the DeviceManager
connection is closed a call to requestDevices() would still attempt to
access m_deviceManager.

BUG=None
==========

[haraken, 2016-05-04 03:26:15]

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

[haraken, 2016-05-04 03:26:15]

https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USB.cpp (right):

https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:167: if (!m_deviceManager) {

Does this mean that USB::onGetPermission can be called after the execution
context is destroyed?

https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:183:
m_deviceManager->GetDeviceChanges(createBaseCallback(bind<usb::DeviceChangeNotificationPtr>(&USB::onDeviceChanges,
WeakPersistentThisPointer<USB>(this))));

I'm wondering why you don't need the if(!m_deviceManager) check here.

[Reilly Grant (use Gerrit), 2016-05-04 18:20:41]

https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USB.cpp (right):

https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:167: if (!m_deviceManager) {
On 2016/05/04 at 03:26:15, haraken wrote:
> Does this mean that USB::onGetPermission can be called after the execution
context is destroyed?

No, the issue is that m_deviceManager and m_chooserService are different pipes.
If one is closed by the browser process (for any reason, in this case it was a
bug in the test script) then a request may still complete on the other. This is
not likely to happen in production but is still worth guarding again.

[Reilly Grant (use Gerrit), 2016-05-04 18:22:32]

https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webusb/USB.cpp (right):

https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webusb/USB.cpp:183:
m_deviceManager->GetDeviceChanges(createBaseCallback(bind<usb::DeviceChangeNotificationPtr>(&USB::onDeviceChanges,
WeakPersistentThisPointer<USB>(this))));
On 2016/05/04 at 03:26:15, haraken wrote:
> I'm wondering why you don't need the if(!m_deviceManager) check here.

Since this method is called to handle a message arriving on the pipe held by
m_deviceManager it is guaranteed that m_deviceManager will not be null. Note
that in my next change this stops being the case and I have to add the check.

[haraken, 2016-05-05 01:13:10]

On 2016/05/04 18:22:32, Reilly Grant wrote:
>
https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webusb/USB.cpp (right):
> 
>
https://codereview.chromium.org/1951723002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webusb/USB.cpp:183:
>
m_deviceManager->GetDeviceChanges(createBaseCallback(bind<usb::DeviceChangeNotificationPtr>(&USB::onDeviceChanges,
> WeakPersistentThisPointer<USB>(this))));
> On 2016/05/04 at 03:26:15, haraken wrote:
> > I'm wondering why you don't need the if(!m_deviceManager) check here.
> 
> Since this method is called to handle a message arriving on the pipe held by
> m_deviceManager it is guaranteed that m_deviceManager will not be null. Note
> that in my next change this stops being the case and I have to add the check.

Thanks, makes sense! LGTM.

[Ken Rockot(use gerrit already), 2016-05-05 01:44:18]

lgtm

[Reilly Grant (use Gerrit), 2016-05-05 06:15:52]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-05-05 06:16:09]

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

[commit-bot: I haz the power, 2016-05-05 08:31:23]

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

[commit-bot: I haz the power, 2016-05-05 08:31:24]

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

[Reilly Grant (use Gerrit), 2016-05-05 19:57:10]

Check for undefined better.

[Reilly Grant (use Gerrit), 2016-05-05 20:05:18]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-05-05 20:05:19]

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/1951723002/#ps40001
(title: "Check for undefined better.")

[commit-bot: I haz the power, 2016-05-05 20:06:09]

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

[commit-bot: I haz the power, 2016-05-06 00:21:42]

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

[commit-bot: I haz the power, 2016-05-06 00:21:43]

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

[Reilly Grant (use Gerrit), 2016-05-06 00:35:48]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2016-05-06 00:36:08]

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

[commit-bot: I haz the power, 2016-05-06 04:14:35]

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

[commit-bot: I haz the power, 2016-05-06 04:14:36]

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

[Reilly Grant (use Gerrit), 2016-05-06 22:37:02]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2016-05-06 22:37:02]

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/1951723002/#ps60001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-05-06 22:37:40]

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

[commit-bot: I haz the power, 2016-05-06 22:48:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-06 22:50:02]

Description was changed from

==========
Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Instead of subclassing the stubClass type of the Mojo service being
implemented we should call connection.bindHandleToStub to create a stub
and then set our implementation as its delegate. The existing code
instead unintentionally overrode the generated stub method bodies which
was mostly harmless but broke passing interface pointers.

While making this change it was discovered that if the DeviceManager
connection is closed a call to requestDevices() would still attempt to
access m_deviceManager.

BUG=None
==========

to

==========
Use the Mojo StubBindings interface correctly in WebUSB LayoutTests.

Instead of subclassing the stubClass type of the Mojo service being
implemented we should call connection.bindHandleToStub to create a stub
and then set our implementation as its delegate. The existing code
instead unintentionally overrode the generated stub method bodies which
was mostly harmless but broke passing interface pointers.

While making this change it was discovered that if the DeviceManager
connection is closed a call to requestDevices() would still attempt to
access m_deviceManager.

BUG=None

Committed: https://crrev.com/9b73f1d2dcf4a5c7669fafffb8e670b5d421e4f8
Cr-Commit-Position: refs/heads/master@{#392190}
==========

[commit-bot: I haz the power, 2016-05-06 22:50:04]

Patchset 4 (id:??) landed as
https://crrev.com/9b73f1d2dcf4a5c7669fafffb8e670b5d421e4f8
Cr-Commit-Position: refs/heads/master@{#392190}