Closed Bug 1008483 Opened 6 years ago Closed 5 years ago

Camera ReadWrite locks are not hooked up to the deadlock detector

Categories

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

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
FxOS-S5 (21Aug)
Tracking Status
firefox43 --- fixed

People

(Reporter: bent.mozilla, Assigned: royang51)

References

Details

Attachments

(1 file)

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.
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!
Assignee: nobody → ehsan
Flags: needinfo?(mhabicher)
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.
Flags: needinfo?(mhabicher)
(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...
Flags: needinfo?(mhabicher)
ping?
Bug 1139027 removes the RW lock around GonkCameraParameters and replaces it with a standard mutex. The RW lock around the listeners in CameraControlImpl remains.
Depends on: 1139027
royang: Can you take a look at this?
Assignee: ehsan → royang51
Flags: needinfo?(bugzilla)
Status: NEW → ASSIGNED
Component: DOM: Device Interfaces → Gaia::Camera
Product: Core → Firefox OS
Version: Trunk → unspecified
Attached patch bug1008483, v1Splinter Review
Attachment #8631931 - Flags: review?(aosmond)
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!
Attachment #8631931 - Flags: review?(aosmond) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/291ae45aa7ce
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
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.
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...).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/ecdaffeae76f
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.