882328 - nsICameraGetCameraCallback addref-ed on non-Main thread

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

When camera app is started on master Firefox OS unagi, the app crashed on the app start up.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: Stacktrace of the crash

Comment 2

[Sotaro Ikeda [:sotaro]]

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

> nsCOMPtr<nsICameraGetCameraCallback> mOnSuccessCb;

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: Call InitGonkCameraControl::Run() on main thread

I confirmed the patch works on master FirefoxOS unagi.

Comment 4

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

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.

Comment 5

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

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

Comment 6

[Sotaro Ikeda [:sotaro]]

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

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: addref nsICameraGetCameraCallback on Main thread

Confirmed the patch on master buri.

Comment 9

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

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.

Comment 10

[Sotaro Ikeda [:sotaro]]

I might be effect of Bug 770840.

Comment 11

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

(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?

Comment 12

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

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.

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

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

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

[Scoobidiver (away)]

It's #5 top crasher in B2G 24.0a1.

Comment 17

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

Attachment: 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.

Comment 18

[Josh Matthews [:jdm]]

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 19

[Sotaro Ikeda [:sotaro]]

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.

Comment 20

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

Attachment: WIP - don't twiddle JS-wrapping nsCOMPtr's off of the main thread, v2

Comme ça?

Comment 21

[Josh Matthews [:jdm]]

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.

Comment 22

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

Attachment: don't twiddle JS-wrapping nsCOMPtr's off of the main thread, r=jdm, v3

Incorporate feedback and carry r+ forward.

Comment 23

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

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=c992b312ed03

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/99761867d59c

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/99761867d59c