1008483 - Camera ReadWrite locks are not hooked up to the deadlock detector

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I'm a bit skeptical that our old-as-the-hills NSPR RW locks are a) well-tested, or b) performant, but in any case the current RwLockAutoEnterRead/RwLockAutoEnterWrite are not hooked up to the deadlock detector.

Comment 1

[(no longer active)]

Mike, it seems like bug 909542 added RwLockAutoEnterRead and RwLockAutoEnterWrite, but the patch is huge and it's not clear to me why these locks are used and what properties of them you're relying on.  Can you please help me understand what these are trying to achieve?

Thanks!

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Ehsan, they're being used for two purposes:
1. to protect the parameters database at the heart of GonkCameraParameters; and
2. to protect the array of CameraControlListeners in CameraControlImpl.

In the latter case, multiple listener-bound events may be fired from multiple threads within the camera driver, triggering callbacks that could try to modify the listener list. The camera driver threading model is not well documented or even standard across platforms, and using a RW-lock makes sure the readers don't deadlock.

Comment 3

[(no longer active)]

(In reply to Mike Habicher [:mikeh] from comment #2)
> Ehsan, they're being used for two purposes:
> 1. to protect the parameters database at the heart of GonkCameraParameters;
> and
> 2. to protect the array of CameraControlListeners in CameraControlImpl.
> 
> In the latter case, multiple listener-bound events may be fired from
> multiple threads within the camera driver, triggering callbacks that could
> try to modify the listener list. The camera driver threading model is not
> well documented or even standard across platforms, and using a RW-lock makes
> sure the readers don't deadlock.

Hmm, my question was more specifically about the usage of RW-locks versus normal locks.  What do the RW-locks buy you?  As far as I can see, you would be able to use normal locks for both #1 and #2 above...

Comment 4

[(no longer active)]

ping?

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Bug 1139027 removes the RW lock around GonkCameraParameters and replaces it with a standard mutex. The RW lock around the listeners in CameraControlImpl remains.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

royang: Can you take a look at this?

Comment 7

[royang51]

Attachment: bug1008483, v1

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8631931 [details] [diff] [review]
bug1008483, v1

Review of attachment 8631931 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the delay on this. Tried it out, looks good to me!

Comment 9

[royang51]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e96a2a90c2

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/291ae45aa7ce

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/b2g-inbound/rev/291ae45aa7ce

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/291ae45aa7ce

Comment 13

[Jan Varga [:janv]]

Comment on attachment 8631931 [details] [diff] [review]
bug1008483, v1

Review of attachment 8631931 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/camera/CameraControlImpl.cpp
@@ -5,5 @@
>  #include "CameraControlImpl.h"
>  #include "base/basictypes.h"
>  #include "mozilla/Assertions.h"
>  #include "mozilla/unused.h"
> -#include "nsPrintfCString.h"

Why did you remove it ?

My local build fails:

In file included from /Users/varga/Sources/Mozilla/obj-ff-dbg/dom/camera/Unified_cpp_dom_camera0.cpp:2:
/Users/varga/Sources/Mozilla/dom/camera/CameraControlImpl.cpp:350:7: error: 
      unknown type name 'nsPrintfCString'
      nsPrintfCString msg("Camera control API(%d) failed with 0x%x", mCo...
      ^
/Users/varga/Sources/Mozilla/dom/camera/CameraControlImpl.cpp:373:3: error: 
      unknown type name 'nsPrintfCString'
  nsPrintfCString msg("Failed to dispatch camera control message (0x%x)", rv);
  ^
2 errors generated.

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Reopened due to build break (although not locally for me on the try servers, I can see in theory how it might be a problem...).

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/ecdaffeae76f

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ecdaffeae76f