Closed Bug 882328 Opened 7 years ago Closed 7 years ago
ICamera Get Camera Callback addref-ed on non-Main thread
When camera app is started on master Firefox OS unagi, the app crashed on the app start up.
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-
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
(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
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?
Severity: normal → critical
Crash Signature: [@ nsXPCWrappedJS::AddRef()]
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.
(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.
Assignee: sotaro.ikeda.g → mhabicher
I haven't had a chance to test this yet, but it builds.
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+
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.
try-server push: https://tbpl.mozilla.org/?tree=Try&rev=c992b312ed03
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 885133
You need to log in before you can comment on or make changes to this bug.