nsICameraGetCameraCallback addref-ed on non-Main thread

RESOLVED FIXED

Status

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sotaro, Assigned: mikeh)

Tracking

({crash, topcrash})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2g-crash], crash signature)

Attachments

(1 attachment, 5 obsolete attachments)

When camera app is started on master Firefox OS unagi, the app crashed on the app start up.
Posted file Stacktrace of the crash (obsolete) —
By debugging by using GDB, it seems addref of mOnSuccessCb needs to be on main thread.

> nsCOMPtr<nsICameraGetCameraCallback> mOnSuccessCb;
I confirmed the patch works on master FirefoxOS unagi.
Comment on attachment 761557 [details] [diff] [review]
Call InitGonkCameraControl::Run() on main thread

The point of running Init off the main thread is to not block the UI while the camera is initializing, something we've seen can take an arbitrarily long time.
Attachment #761557 - Flags: feedback-
(In reply to Mike Habicher [:mikeh] from comment #5)
> The correct solution is to hold references to the callbacks in
> nsMainThreadPtrHandle/Holder objects.
> 
> See
> https://mxr.mozilla.org/mozilla-central/source/dom/camera/CameraControlImpl.
> h#213
> and
> https://mxr.mozilla.org/mozilla-central/source/dom/camera/CameraControlImpl.
> h#224

Thanks for the info. I tried it, though it did not help for addref case.
Summary: nsDOMCameraControl::Result() is called on non-Main thread → nsICameraGetCameraCallback addref-ed on non-Main thread
Confirmed the patch on master buri.
Attachment #761557 - Attachment is obsolete: true
Assignee: nobody → sotaro.ikeda.g
Duplicate of this bug: 882637
Blocks: 770840, 773610
sotaro, do you have any idea when was this started being a problem? The original changes went in ages ago; it seems strange we're just starting to see crashes now.
I might be effect of Bug 770840.
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> 
> Thanks for the info. I tried it, though it did not help for addref case.

What is the addref case?
Attachment #761802 - Flags: review?(mhabicher)
Severity: normal → critical
Crash Signature: [@ nsXPCWrappedJS::AddRef()]
Keywords: crash
Whiteboard: [b2g-crash]
Comment on attachment 761802 [details] [diff] [review]
addref nsICameraGetCameraCallback on Main thread

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

sotaro, this looks like it would work, and I'll give it a more thorough review in a moment; but first I'd like to understand why using nsMainThreadPtrHolder/Handle objects won't work, and the 'addref' case you mentioned earlier.
Duplicate of this bug: 882481
(In reply to Mike Habicher [:mikeh] from comment #11)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > 
> > Thanks for the info. I tried it, though it did not help for addref case.
> 
> What is the addref case?

At first I just added nsMainThreadPtrHandle to GetCameraResult. In this case, GetCameraResult was still instantiated on off-Main thread. A reference count of nsICameraGetCameraCallback was incremented on off-main Thread. Then the camera app was still crashed on camera app start-up.

nsMainThreadPtrHolder does not have a capability forcibly increment the reference count on main thread. But it have a capability of forcibly decrement the reference count on main thread. Therefore I move the creation of GetCameraResult to InitGonkCameraControl's constructor to increment the reference count on main-thread. And the problem was fixed.
The nsMainThreadPtrHolder is created on the Main Thread to wrap the Main-Thread-only objects, and it increments the references to those wrapped objects; then the Holders are passed around in nsMainThreadPtrHandles, which can increment and decrement references to the nsMainThreadPtrHolder on and off the Main Thread.

So both InitGonkCameraControl and GetCameraResult would need to have mOnSuccessCb and mOnErrorCb changed to nsMainThreadPtrHandles.

The reference to the nsDOMCameraControl object is/should be only incremented on the Main Thread, in nsDOMCameraControl::nsDOMCameraControl() and in GetCameraResult::Run().
It's #5 top crasher in B2G 24.0a1.
Keywords: topcrash
Assignee: sotaro.ikeda.g → mhabicher
I haven't had a chance to test this yet, but it builds.
Attachment #761550 - Attachment is obsolete: true
Attachment #761802 - Attachment is obsolete: true
Attachment #761802 - Flags: review?(mhabicher)
Attachment #762795 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 762795 [details] [diff] [review]
WIP - don't twiddle JS-wrapping nsCOMPtr's off of the main thread

This looks right to me. Maybe pass const nsMainThreadPtrHandle references around.
Comment on attachment 762795 [details] [diff] [review]
WIP - don't twiddle JS-wrapping nsCOMPtr's off of the main thread

I checked the patch works on mater hamachi.
Attachment #762795 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Comme ça?
Attachment #762795 - Attachment is obsolete: true
Attachment #762916 - Flags: review?(josh)
Comment on attachment 762916 [details] [diff] [review]
WIP - don't twiddle JS-wrapping nsCOMPtr's off of the main thread, v2

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

::: dom/camera/DOMCameraControl.cpp
@@ +451,5 @@
>    uint64_t mWindowId;
>  };
>  
>  nsresult
> +nsDOMCameraControl::Result(nsresult aResult, const nsMainThreadPtrHandle<nsICameraGetCameraCallback>& onSuccess, const nsMainThreadPtrHandle<nsICameraErrorCallback>& onError, uint64_t aWindowId)

Long line is long?

::: dom/camera/GonkCameraControl.cpp
@@ +195,5 @@
>    nsRefPtr<nsGonkCameraControl> mCameraControl;
>    // Raw pointer to DOM-facing camera control--it must NS_ADDREF itself for us
>    nsDOMCameraControl* mDOMCameraControl;
> +  const nsMainThreadPtrHandle<nsICameraGetCameraCallback> mOnSuccessCb;
> +  const nsMainThreadPtrHandle<nsICameraErrorCallback> mOnErrorCb;

These don't need to be const.
Attachment #762916 - Flags: review?(josh) → review+
Incorporate feedback and carry r+ forward.
Attachment #762916 - Attachment is obsolete: true
Attachment #763601 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/99761867d59c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 885133
Duplicate of this bug: 883873
You need to log in before you can comment on or make changes to this bug.