Separate usb device handle from usb device. (deprecate)

Separate usb device handle from usb device.

*This CL has been splitted to four steps*:
  1. Upgrade libusb and introduce a mechanism to interrupt libusb_handle_events. _Done_
  2. Separate UsbDevice and UsbDeviceHandle.
  3. Introduce UsbTransfer and improve life cycle management of transfers.
  4. Introduce `chrome.usb.getDevices` and `chrome.usb.openDevice` API.

BUG=223817

[Bei Zhang, 2013-06-05 23:38:19]

Hi Antoney,

Please review.

Thanks,
Bei

[asargent_no_longer_on_chrome, 2013-06-06 21:57:29]

https://codereview.chromium.org/16316004/diff/24001/chrome/common/extensions/...
File chrome/common/extensions/api/usb.idl (right):

https://codereview.chromium.org/16316004/diff/24001/chrome/common/extensions/...
chrome/common/extensions/api/usb.idl:33: };
Because this API is now released to stable, we can't change the input arguments
or structure of result data we pass to callbacks because it would break
backwards compatibility. 

We can add functions with new names, or add new fields to existing dictionaries,
etc. and mark old functions or field names as nodoc to deprecate them, but an
invariant is that code written against the currently specified API before your
change must continue to run correctly.

[akalin, 2013-06-13 21:53:12]

Drive-by

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_de...
File chrome/browser/usb/usb_device_handle.h (right):

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_de...
chrome/browser/usb/usb_device_handle.h:47: class UsbDeviceHandle : public
base::RefCountedThreadSafe<UsbDeviceHandle>,
This doesn't make sense.

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

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:56: volatile bool running_;
volatile isn't sufficient -- you need a memory barrier if you're using this
across threads (i.e., protect it with a lock)

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:105: class UsbDevice : public
base::RefCountedThreadSafe<UsbDevice>,
this also doesn't make sense. Can you use DeleteSoon instead?

[Bei Zhang, 2013-06-13 22:36:22]

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

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:56: volatile bool running_;
I thought about this very carefully and yes you're right, this must be protected
with a lock.

At first I thought this was OK because libusb_handle_events and libusb_exit are
synced with an internal mutex. But after reading their code I found out that
they are not.

On 2013/06/13 21:53:12, akalin wrote:
> volatile isn't sufficient -- you need a memory barrier if you're using this
> across threads (i.e., protect it with a lock)

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:105: class UsbDevice : public
base::RefCountedThreadSafe<UsbDevice>,
Both ReleaseSoon and DeleteSoon requires RefCountedThreadSafe<T>.

Is it that weird? I can find several examples in the code base but indeed not
many of them.

Maybe I should use base::ThreadChecker instead?

On 2013/06/13 21:53:12, akalin wrote:
> this also doesn't make sense. Can you use DeleteSoon instead?

[akalin, 2013-06-14 00:40:05]

On 2013/06/13 22:36:22, Bei Zhang wrote:
>
https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
> File chrome/browser/usb/usb_service.cc (right):
> 
>
https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
> chrome/browser/usb/usb_service.cc:56: volatile bool running_;
> I thought about this very carefully and yes you're right, this must be
protected
> with a lock.
> 
> At first I thought this was OK because libusb_handle_events and libusb_exit
are
> synced with an internal mutex. But after reading their code I found out that
> they are not.

Even if libusb_{handle_events,exit} were synchronized, you'd still need a
lock/memory barrier for running_. Remember that you're not just fighting against
the CPU, but the compiler as well (which can reorder reads/writes).

> Is it that weird? I can find several examples in the code base but indeed not
> many of them.

Those other examples are buggy, then. :/ Inheriting from RCTS implies that the
thread is used across multiple threads. Inheriting from NonThreadSafe implies
that the thread is used on only one thread. Surely both of them can't be true.

> Maybe I should use base::ThreadChecker instead?

The ideal solution would be to remove ref-counting altogether. Why do you need
it?

[akalin, 2013-06-14 00:43:24]

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

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:200: to_release.swap(it->second);
What's going on here? You shouldn't have to do this.

In particular, calling AddRef() explicitly causes a leak; ReleaseSoon() takes a
ref if you pass it a pointer to a ref-counted class.

I think you should really find a way to remove ref-counting. :/

[Bei Zhang, 2013-06-14 04:59:07]

On 2013/06/14 00:40:05, akalin wrote:
> On 2013/06/13 22:36:22, Bei Zhang wrote:
> >
>
https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
> > File chrome/browser/usb/usb_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
> > chrome/browser/usb/usb_service.cc:56: volatile bool running_;
> > I thought about this very carefully and yes you're right, this must be
> protected
> > with a lock.
> > 
> > At first I thought this was OK because libusb_handle_events and libusb_exit
> are
> > synced with an internal mutex. But after reading their code I found out that
> > they are not.
> 
> Even if libusb_{handle_events,exit} were synchronized, you'd still need a
> lock/memory barrier for running_. Remember that you're not just fighting
against
> the CPU, but the compiler as well (which can reorder reads/writes).
> 
> > Is it that weird? I can find several examples in the code base but indeed
not
> > many of them.
> 
> Those other examples are buggy, then. :/ Inheriting from RCTS implies that the
> thread is used across multiple threads. Inheriting from NonThreadSafe implies
> that the thread is used on only one thread. Surely both of them can't be true.

I believe RCTS means that the scoped_refptr can be share among threads and 
NonThreadSafe is used to indicate non-thread-safe objects that are 

> 
> > Maybe I should use base::ThreadChecker instead?
> 
> The ideal solution would be to remove ref-counting altogether. Why do you need
> it?

[Bei Zhang, 2013-06-14 05:06:01]

Whoops. Mis-clicked the button.
... NonThreadSafe is used to indicate non-thread-safe objects that are appears
to be thread safe:

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/non...

RCTS makes AddRef and Release thread safe but not effecting any other methods. I
think an otherwise non-thread-safe object can have thread-safe methods, and
which doesn't make it thread-safe. On in other words, RCTS is an orthogonal to
NonThreadSafe.

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

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:200: to_release.swap(it->second);
On 2013/06/14 00:43:24, akalin wrote:
> What's going on here? You shouldn't have to do this.
> 
> In particular, calling AddRef() explicitly causes a leak; ReleaseSoon() takes
a
> ref if you pass it a pointer to a ref-counted class.
> 
> I think you should really find a way to remove ref-counting. :/

1. I carefully read the code of BrowserThread::ReleaseSoon and I'm positive that
BrowserThread::ReleaseSoon() will not manually increase the ref counter.
Specifically, it converts the pointer to void* when calling base::Bind:

https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas...

Then it simply Releases it on the thread: 

https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas...

As I tested, if I remove the AddRef, it will be deleted immediately and crashes
soon.

2. I swapped the scoped_refptr in case ReleaseSoon fires before I clear the
devices_ which will end up deleting the object on the current thread -- well, I
did this with a bug. I shall first exit the scope of to_release then use the
pointer to register the ReleaseSoon. But I believe 

I agree that is part of code is horrible :(. I will change the class to use
DeleteOnFileThread deletor instead, which will save all these code.

Your advice about removing the ref-counting is great and I tried very hard to do
it at the beginning. The problem is that I'm trying to hold a pointer to the
object and use it as the value of a map, which requires a operator= of the
object. I really need a map because otherwise an recurring enumeration will
become O(n^2) time. I can control the life-cycle using plain pointers but it'll
be more problematic AFAIK.

[Bei Zhang, 2013-06-14 05:58:24]

Hi Akalin,

I managed to remove the swap-and-ReleaseSoon code (among another fix on the
running_ thanks to your observation) and it looks better now. If possible I
would like to talk to you about the NonThreadSafe/RefCountedThreadSafe thing
tomorrow. I also used the NonThreadSafe in another partially thread safe class.

I really appreciate your kind help!

Thank you,
Bei

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

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:56: volatile bool running_;
On 2013/06/13 21:53:12, akalin wrote:
> volatile isn't sufficient -- you need a memory barrier if you're using this
> across threads (i.e., protect it with a lock)

Done.

[akalin, 2013-06-14 06:24:38]

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

https://codereview.chromium.org/16316004/diff/97002/chrome/browser/usb/usb_se...
chrome/browser/usb/usb_service.cc:200: to_release.swap(it->second);
On 2013/06/14 05:06:02, Bei Zhang wrote:
> On 2013/06/14 00:43:24, akalin wrote:
> > What's going on here? You shouldn't have to do this.
> > 
> > In particular, calling AddRef() explicitly causes a leak; ReleaseSoon()
takes
> a
> > ref if you pass it a pointer to a ref-counted class.
> > 
> > I think you should really find a way to remove ref-counting. :/
> 
> 1. I carefully read the code of BrowserThread::ReleaseSoon and I'm positive
that
> BrowserThread::ReleaseSoon() will not manually increase the ref counter.
> Specifically, it converts the pointer to void* when calling base::Bind:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas...
> 
> Then it simply Releases it on the thread: 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas...
> 
> As I tested, if I remove the AddRef, it will be deleted immediately and
crashes
> soon.
> 
> 2. I swapped the scoped_refptr in case ReleaseSoon fires before I clear the
> devices_ which will end up deleting the object on the current thread -- well,
I
> did this with a bug. I shall first exit the scope of to_release then use the
> pointer to register the ReleaseSoon. But I believe 
> 
> I agree that is part of code is horrible :(. I will change the class to use
> DeleteOnFileThread deletor instead, which will save all these code.
> 
> Your advice about removing the ref-counting is great and I tried very hard to
do
> it at the beginning. The problem is that I'm trying to hold a pointer to the
> object and use it as the value of a map, which requires a operator= of the
> object. I really need a map because otherwise an recurring enumeration will
> become O(n^2) time. I can control the life-cycle using plain pointers but
it'll
> be more problematic AFAIK.

Ah, you're right in that ReleaseSoon doesn't take a ref. However, the code is
still problematic. There is still a race between the two threads as to which
thread will release the last ref. You encountered this problem when you didn't
do the swap, but it still exists even with the swap. It is possible that as soon
as you call ReleaseSoon, execution switches to the FILE thread, releases the
object, then switches back to this thread (the UI thread?) before to_release is
destroyed.

If you really have to stick with ref-counting, what you want is:

BrowserThread::ReleaseSoon(...::FILE, FROM_HERE, it->second->release());

Manual memory management is problematic, but trying to solve it with
ref-counting only introduces more problems, especially with multiple threads. I
think you're better off just storing a map with raw pointers, writing a utility
function to iterate through the map and call DeleteSoon on all objects, clearing
the map, and call that utility function from the destructor as well as any other
place you need it.

[akalin, 2013-06-14 06:35:26]

On 2013/06/14 05:06:01, Bei Zhang wrote:
> Whoops. Mis-clicked the button.
> ... NonThreadSafe is used to indicate non-thread-safe objects that are appears
> to be thread safe:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/non...
> 
> RCTS makes AddRef and Release thread safe but not effecting any other methods.
I
> think an otherwise non-thread-safe object can have thread-safe methods, and
> which doesn't make it thread-safe. On in other words, RCTS is an orthogonal to
> NonThreadSafe.

What you say is technically true, but it's still a code smell. There is a strong
convention/implication that a class inheriting from NonThreadSafe is completely
non-thread-safe, i.e. that CalledOnValidThread() is asserted for each member
function.

On the other hand, inheriting from RCTS means that AddRef/Release are
thread-safe. Either you pass around pointers to your objects from multiple
threads or you don't. If you don't, then you don't need RCTS. If you do pass
them around, then you either use them from multiple threads or you don't. If you
do, then that's an error. If you don't, i.e. you always end up using the object
on the correct thread, then you can almost always find an owner for your object
on that thread, and you don't need ref-counting at all (because the owner can
manage your object's lifetime by itself).

I hope that clears things up.