Usb backend refactor step 2: Separate Usb Device Handle and Usb Device (deprecate)

Usb backend refactor step 2: Separate Usb Device Handle and Usb Device.

This change will allow UsbService to provide unique device ids. It will fix all the threading problems as well.

BUG=223817

[pfeldman, 2013-07-21 08:48:23]

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_dev...
File chrome/browser/usb/usb_device.h (right):

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_dev...
chrome/browser/usb/usb_device.h:23: typedef libusb_device* PlatformUsbDevice;
All these typedefs were here to encapsulate dependency from libusb.h. You are
now including it in public header, so call sites are effectively getting this
dependency. Is that intentional? Would you like to hide libusb behind this API
long term or do you allow clients to use both APIs?

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_dev...
chrome/browser/usb/usb_device.h:67: // FILE thread:
This requires a more comprehensive documentation. I.e. which API is used on
which thread any why.

If there is a need for half of this class to function on one thread and another
half on the other, it suggests that there are two different entities operating
on different threads, potentially having different lifetimes. Like I'd expect
ClaimInterface to return UsbTransfer* (raw pointer with ownership, re-using
net::Socket model) that would have Control, Bulk, etc. transfer methods. Client
needs to claim interface prior to using endpoints and such separation would make
both: workflow and threading perspectives clear.

To be more consistent with chrome threading model,
- UsbService and UsbDevice would operate on UI thread
- UsbTransfer would operate on single IO thread.

or

- UsbService would operate on UI thread
- UsbDevice and UsbTransfer would operate on single IO thread

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_ser...
File chrome/browser/usb/usb_service.h (right):

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_ser...
chrome/browser/usb/usb_service.h:53: scoped_refptr<UsbDevice> OpenDevice(int
device);
So I call this _only_ on UI thread and all subsequent UsbDevice use is either on
FILE or on my IO thread?

[Bei Zhang, 2013-07-21 21:31:52]

Thanks for the comment. I'll start experiment on introducing UsbTransfer now.

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_dev...
File chrome/browser/usb/usb_device.h (right):

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_dev...
chrome/browser/usb/usb_device.h:23: typedef libusb_device* PlatformUsbDevice;
I noticed earlier that this will make your code hard to compile due to the
conflict of winsock.h and winsock2.h. If you are using sockets, you cannot
include libusb.h any more in in 1.0.16. The introduction of libusb.h is for the
LIBUSB_CALL below. I will use API_CALL in base/port.h instead.

On 2013/07/21 08:48:23, pfeldman wrote:
> All these typedefs were here to encapsulate dependency from libusb.h. You are
> now including it in public header, so call sites are effectively getting this
> dependency. Is that intentional? Would you like to hide libusb behind this API
> long term or do you allow clients to use both APIs?

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_dev...
chrome/browser/usb/usb_device.h:67: // FILE thread:
The reason I allow those Async IO to be called on any thread is that it has way
more arguments than we can support in base::Callback. Basically both UsbDevice
and UsbService are not thread safe and can only be used on the FILE thread.

Introducing a UsbTransfer is a good idea and might be essential for exposing an
API to cancel a current async transfer. The down side is that it will make the
life cycle management of usb transfers even harder.

On 2013/07/21 08:48:23, pfeldman wrote:
> This requires a more comprehensive documentation. I.e. which API is used on
> which thread any why.
> 
> If there is a need for half of this class to function on one thread and
another
> half on the other, it suggests that there are two different entities operating
> on different threads, potentially having different lifetimes. Like I'd expect
> ClaimInterface to return UsbTransfer* (raw pointer with ownership, re-using
> net::Socket model) that would have Control, Bulk, etc. transfer methods.
Client
> needs to claim interface prior to using endpoints and such separation would
make
> both: workflow and threading perspectives clear.
> 
> To be more consistent with chrome threading model,
> - UsbService and UsbDevice would operate on UI thread
> - UsbTransfer would operate on single IO thread.
> 
> or
> 
> - UsbService would operate on UI thread
> - UsbDevice and UsbTransfer would operate on single IO thread

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_ser...
File chrome/browser/usb/usb_service.h (right):

https://codereview.chromium.org/18593002/diff/3007/chrome/browser/usb/usb_ser...
chrome/browser/usb/usb_service.h:53: scoped_refptr<UsbDevice> OpenDevice(int
device);
These UsbService is not thread safe. Methods can only be called on FILE thread.

On 2013/07/21 08:48:23, pfeldman wrote:
> So I call this _only_ on UI thread and all subsequent UsbDevice use is either
on
> FILE or on my IO thread?

[pfeldman, 2013-07-22 08:39:45]

I know I'm throwing a lot of suggestions below, but I think they eventually make
things better and easier to understand. I'm also offering some coding help so
easy your pain, so we could split it up. Wdyt?

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
File chrome/browser/usb/usb_device.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:103: TransferInfo();
Constructor should go first

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
File chrome/browser/usb/usb_service.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:72: class UsbContext : public
base::RefCountedThreadSafe<UsbContext> {
I think this tiny bit is worth landing separately.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
File chrome/browser/usb/usb_service.h (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.h:34: // The following methods must be called on
FILE thread:
UsbService is created and deleted on UI thread. You can't call its methods on
FILE thread since you are introducing a race condition.

You either need to make it ref-counted + wrap it for use with
BrowserContextKeyedService (as I do in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...)
or you need to make it only work UI thread only.

I actually think that latter is preferred. You would essentially need to
marshall its public methods to FILE thread first in usb_service.cc, but it
should not be a big deal given that your API has callbacks for return values. As
a benefit, all clients become simpler - they no longer need to do marshalling as
you do in usb_api.cc.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.h:47: void GetDevices(std::vector<int>* devices);
I don't think this integer handle makes sense. There is a number of operations
available on the device (on libusb_device) without opening it.
libusb_get_device_descriptor and libusb_get_active_config_descriptor would be
good examples. Your UsbDeviceStub captures (not yet opened) device well and I
think you should expose it to the API. I.e. I think your API could reflect
libusb API better:

UsbService -> context, singleton
UsbDevice -> libusb_device
UsbDeviceHandle -> libusb_device_handle
UsbInterface -> libusb_device_handle with claimed interface

those types would use RAII well.

Another idea is that although UsbDevice is not thread-safe, it could be operated
on any IO thread. Like WeakPtr - client picks thread with its first call and
sticks to it. libusb_device is thread-safe, so it should be doable.

[pfeldman, 2013-07-22 18:23:13]

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/extensions...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_api.cc:455: base::Unretained(service_),
service_ might have been deleted by now. You should make it RefCountedThreadSafe
(and wrap with another object for browsercontextgetter).

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/extensions...
File chrome/browser/extensions/api/usb/usb_device_resource.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_device_resource.cc:58: void
UsbDeviceResource::ListInterfaces(UsbConfigDescriptor* config,
Unfortunately, all your clients would need to have wrappers like this. It would
be great if longer term, your UsbDevice would allow operation on any thread
while doing things on FILE under the hood.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
File chrome/browser/usb/usb_device.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:231: void
UsbDevice::ListInterfaces(UsbConfigDescriptor* config,
Why is this using callback? It returns synchronously. If you require it to be
running on FILE, you can drop the callback.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:252:
DCHECK(thread_checker_.CalledOnValidThread());
ditto

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:264:
DCHECK(thread_checker_.CalledOnValidThread());
ditto

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:279:
DCHECK(thread_checker_.CalledOnValidThread());
ditto

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:316: base::Bind(&UsbDevice::SubmitTransfer,
Why scheduling submit? libusb_submit_transfer claims to be returning immediately
as a part of async io.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:340: base::Bind(&UsbDevice::SubmitTransfer,
ditto

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
File chrome/browser/usb/usb_device.h (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.h:130: static void API_CALL
HandleTransferCompletion(
Why API_CALL?

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
File chrome/browser/usb/usb_service.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:137: return scoped_refptr<UsbDevice>();
You could return NULL.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:199: client->RequestUsbAccess(vendor_id,
Can you call it on FILE?

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:202: base::Bind(&UsbService::FindDevicesImpl,
On what thread will it be called?

[Bei Zhang, 2013-07-23 17:58:44]

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/extensions...
File chrome/browser/extensions/api/usb/usb_api.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/extensions...
chrome/browser/extensions/api/usb/usb_api.cc:455: base::Unretained(service_),
I will make it Singleton and it will never shows up in the PostTask. Also,
libusb_context is designed to have only one instance as well.

On 2013/07/22 18:23:13, pfeldman wrote:
> service_ might have been deleted by now. You should make it
RefCountedThreadSafe

> (and wrap with another object for browsercontextgetter).

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
File chrome/browser/usb/usb_device.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:103: TransferInfo();
On 2013/07/22 08:39:45, pfeldman wrote:
> Constructor should go first

Done.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:231: void
UsbDevice::ListInterfaces(UsbConfigDescriptor* config,
I see... That'll do.

On 2013/07/22 18:23:13, pfeldman wrote:
> Why is this using callback? It returns synchronously. If you require it to be
> running on FILE, you can drop the callback.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.cc:316: base::Bind(&UsbDevice::SubmitTransfer,
There is a map to the transfers which is not thread safe. I'm trying to avoid
the lock.

On 2013/07/22 18:23:13, pfeldman wrote:
> Why scheduling submit? libusb_submit_transfer claims to be returning
immediately
> as a part of async io.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
File chrome/browser/usb/usb_device.h (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device.h:130: static void API_CALL
HandleTransferCompletion(
This callback needs __stdcall on windows.

On 2013/07/22 18:23:13, pfeldman wrote:
> Why API_CALL?

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
File chrome/browser/usb/usb_service.cc (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:72: class UsbContext : public
base::RefCountedThreadSafe<UsbContext> {
On 2013/07/22 08:39:45, pfeldman wrote:
> I think this tiny bit is worth landing separately.

Done.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:199: client->RequestUsbAccess(vendor_id,
Nope, this is a bug.

On 2013/07/22 18:23:13, pfeldman wrote:
> Can you call it on FILE?

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:202: base::Bind(&UsbService::FindDevicesImpl,
If it was working, which I doubt, then it shall be called on IO thread.

On 2013/07/22 18:23:13, pfeldman wrote:
> On what thread will it be called?

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
File chrome/browser/usb/usb_service.h (right):

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.h:34: // The following methods must be called on
FILE thread:
This is the summary of our discussion about this:
1. We determined that the enumeration of the usb devices will require blocking
IOs that is not allowed on UI thread or IO thread.
2. We shall not have more than one UsbService because libusb does not actually
support that.

In conclusion, we will make UsbService a Singleton that works mostly on FILE
thread but construct/destructs on UI thread.


On 2013/07/22 08:39:45, pfeldman wrote:
> UsbService is created and deleted on UI thread. You can't call its methods on
> FILE thread since you are introducing a race condition.
> 
> You either need to make it ref-counted + wrap it for use with
> BrowserContextKeyedService (as I do in
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dev...)
> or you need to make it only work UI thread only.
> 
> I actually think that latter is preferred. You would essentially need to
> marshall its public methods to FILE thread first in usb_service.cc, but it
> should not be a big deal given that your API has callbacks for return values.
As
> a benefit, all clients become simpler - they no longer need to do marshalling
as
> you do in usb_api.cc.

https://codereview.chromium.org/18593002/diff/69002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.h:47: void GetDevices(std::vector<int>* devices);
Yes, that will be exactly what the step 3 does.

On 2013/07/22 08:39:45, pfeldman wrote:
> I don't think this integer handle makes sense. There is a number of operations
> available on the device (on libusb_device) without opening it.
> libusb_get_device_descriptor and libusb_get_active_config_descriptor would be
> good examples. Your UsbDeviceStub captures (not yet opened) device well and I
> think you should expose it to the API. I.e. I think your API could reflect
> libusb API better:
> 
> UsbService -> context, singleton
> UsbDevice -> libusb_device
> UsbDeviceHandle -> libusb_device_handle
> UsbInterface -> libusb_device_handle with claimed interface
> 
> those types would use RAII well.
> 
> Another idea is that although UsbDevice is not thread-safe, it could be
operated
> on any IO thread. Like WeakPtr - client picks thread with its first call and
> sticks to it. libusb_device is thread-safe, so it should be doable.