nsICameraGetCameraCallback addref-ed on non-Main thread

RESOLVED FIXED

Status

Firefox OS
General
--
critical
RESOLVED FIXED
5 years ago
5 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)

(Reporter)

Description

5 years ago
When camera app is started on master Firefox OS unagi, the app crashed on the app start up.
(Reporter)

Comment 1

5 years ago
Created attachment 761550 [details]
Stacktrace of the crash
(Reporter)

Comment 2

5 years ago
By debugging by using GDB, it seems addref of mOnSuccessCb needs to be on main thread.

> nsCOMPtr<nsICameraGetCameraCallback> mOnSuccessCb;
(Reporter)

Comment 3

5 years ago
Created attachment 761557 [details] [diff] [review]
Call InitGonkCameraControl::Run() on main thread

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-
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
(Reporter)

Comment 6

5 years ago
(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.
(Reporter)

Updated

5 years ago
Summary: nsDOMCameraControl::Result() is called on non-Main thread → nsICameraGetCameraCallback addref-ed on non-Main thread
(Reporter)

Comment 7

5 years ago
Created attachment 761802 [details] [diff] [review]
addref nsICameraGetCameraCallback on Main thread

Confirmed the patch on master buri.
Attachment #761557 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Assignee: nobody → sotaro.ikeda.g

Updated

5 years ago
Duplicate of this bug: 882637

Updated

5 years ago
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.
(Reporter)

Comment 10

5 years ago
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?
(Reporter)

Updated

5 years ago
Attachment #761802 - Flags: review?(mhabicher)

Updated

5 years ago
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.

Updated

5 years ago
Duplicate of this bug: 882481
(Reporter)

Comment 14

5 years ago
(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().

Comment 16

5 years ago
It's #5 top crasher in B2G 24.0a1.
Keywords: topcrash
Assignee: sotaro.ikeda.g → mhabicher
Created attachment 762795 [details] [diff] [review]
WIP - don't twiddle JS-wrapping nsCOMPtr's off of the main thread

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.
(Reporter)

Comment 19

5 years ago
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+
Created attachment 762916 [details] [diff] [review]
WIP - don't twiddle JS-wrapping nsCOMPtr's off of the main thread, v2

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+
Created attachment 763601 [details] [diff] [review]
don't twiddle JS-wrapping nsCOMPtr's off of the main thread, r=jdm, v3

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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 885133

Updated

5 years ago
Duplicate of this bug: 883873
You need to log in before you can comment on or make changes to this bug.