Closed Bug 1129051 Opened 10 years ago Closed 10 years ago

[B2G][Camera] Multiple camera control listeners cause double free for take picture callbacks

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
2.2 S11 (1may)
Tracking Status
firefox40 --- fixed

People

(Reporter: aosmond, Assigned: royang51)

References

Details

Attachments

(1 file, 4 obsolete files)

Latent issue (not presently triggered) in camera control code: nsGonkCameraControl::OnTakePictureComplete copies the picture blob data into new buffer CameraControlImpl::OnTakePictureComplete forwards buffer to listeners DOMCameraControlListener::OnTakePictureComplete posts to the main thread and wraps the buffer via File::CreateMemoryFile The copy should happen for each listener instead of once in nsGonkCameraControl, because File::CreateMemoryFile assumes that it owns the buffer pointer and will free it when it goes out of scope.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee: aosmond → royang51
Attached patch bug1129051.patch, v1 (obsolete) — Splinter Review
Attachment #8597730 - Flags: review?(aosmond)
Attached patch bug1129051_2.patch (obsolete) — Splinter Review
Attachment #8597732 - Flags: review?(aosmond)
Attachment #8597730 - Attachment is obsolete: true
Attachment #8597730 - Flags: review?(aosmond)
Attachment #8597732 - Attachment is obsolete: true
Attachment #8597732 - Flags: review?(aosmond)
Attached patch bug1129051.patch, V2 (obsolete) — Splinter Review
Attachment #8597733 - Flags: review?(aosmond)
Comment on attachment 8597733 [details] [diff] [review] bug1129051.patch, V2 Review of attachment 8597733 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af61ffd252a
Attachment #8597733 - Flags: review?(aosmond) → review+
royang, could you update the patch with a description? See one of the patches in bug 994912 as an example. It should look something like: > Bug XXX - Description. r=aosmond > > diff --git ... > ... Also please request Commit Access (Level 1) so that you can push to the try server yourself. Follow the instructions at https://www.mozilla.org/en-US/about/governance/policies/commit/. Add a needinfo for me and I will vouch for you.
Comment on attachment 8597733 [details] [diff] [review] bug1129051.patch, V2 From: Roger Yang <royang51@gmail.com> Bug 1129051 - Fix Camera Control Listener double free. Fix webrtc memory leak. >diff --git a/dom/camera/CameraControlListener.h b/dom/camera/CameraControlListener.h >index 532fdaf..e489c11 100644 >--- a/dom/camera/CameraControlListener.h >+++ b/dom/camera/CameraControlListener.h >@@ -84,17 +84,17 @@ public: > public: > uint32_t mMaxMeteringAreas; > uint32_t mMaxFocusAreas; > }; > virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) { } > > virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) { } > virtual void OnAutoFocusMoving(bool aIsMoving) { } >- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { } >+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { } > virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) { } > > enum UserContext > { > kInStartCamera, > kInStopCamera, > kInAutoFocus, > kInStartFaceDetection, >diff --git a/dom/camera/DOMCameraControlListener.cpp b/dom/camera/DOMCameraControlListener.cpp >index 96f55ab..7a586c4 100644 >--- a/dom/camera/DOMCameraControlListener.cpp >+++ b/dom/camera/DOMCameraControlListener.cpp >@@ -336,28 +336,30 @@ DOMCameraControlListener::OnAutoFocusComplete(bool aAutoFocusSucceeded) > protected: > bool mAutoFocusSucceeded; > }; > > NS_DispatchToMainThread(new Callback(mDOMCameraControl, aAutoFocusSucceeded)); > } > > void >-DOMCameraControlListener::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+DOMCameraControlListener::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > { > class Callback : public DOMCallback > { > public: > Callback(nsMainThreadPtrHandle<nsISupports> aDOMCameraControl, >- uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+ const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > : DOMCallback(aDOMCameraControl) >- , mData(aData) > , mLength(aLength) > , mMimeType(aMimeType) >- { } >+ { >+ mData = (uint8_t*) malloc(aLength); >+ memcpy(mData, aData, aLength); >+ } > > void > RunCallback(nsDOMCameraControl* aDOMCameraControl) override > { > nsCOMPtr<nsIDOMBlob> picture = > File::CreateMemoryFile(mDOMCameraControl.get(), > static_cast<void*>(mData), > static_cast<uint64_t>(mLength), >diff --git a/dom/camera/DOMCameraControlListener.h b/dom/camera/DOMCameraControlListener.h >index a5c30c6..c1a1be8 100644 >--- a/dom/camera/DOMCameraControlListener.h >+++ b/dom/camera/DOMCameraControlListener.h >@@ -16,17 +16,17 @@ class CameraPreviewMediaStream; > class DOMCameraControlListener : public CameraControlListener > { > public: > DOMCameraControlListener(nsDOMCameraControl* aDOMCameraControl, CameraPreviewMediaStream* aStream); > > virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) override; > virtual void OnAutoFocusMoving(bool aIsMoving) override; > virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) override; >- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; >+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; > > virtual void OnHardwareStateChange(HardwareState aState, nsresult aReason) override; > virtual void OnPreviewStateChange(PreviewState aState) override; > virtual void OnRecorderStateChange(RecorderState aState, int32_t aStatus, int32_t aTrackNum) override; > virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) override; > virtual void OnShutter() override; > virtual void OnRateLimitPreview(bool aLimit) override; > virtual bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override; >diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp >index 3e1eee7..41a15e5 100644 >--- a/dom/camera/GonkCameraControl.cpp >+++ b/dom/camera/GonkCameraControl.cpp >@@ -1529,24 +1529,20 @@ nsGonkCameraControl::OnFacesDetected(camera_frame_metadata_t* aMetaData) > CameraControlImpl::OnFacesDetected(faces); > } > > void > nsGonkCameraControl::OnTakePictureComplete(uint8_t* aData, uint32_t aLength) > { > ReentrantMonitorAutoEnter mon(mReentrantMonitor); > >- uint8_t* data = new uint8_t[aLength]; >- >- memcpy(data, aData, aLength); >- > nsString s(NS_LITERAL_STRING("image/")); > s.Append(mFileFormat); > DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength); >- OnTakePictureComplete(data, aLength, s); >+ OnTakePictureComplete(aData, aLength, s); > > if (mResumePreviewAfterTakingPicture) { > nsresult rv = StartPreview(); > if (NS_FAILED(rv)) { > DOM_CAMERA_LOGE("Failed to restart camera preview (%x)\n", rv); > OnPreviewStateChange(CameraControlListener::kPreviewStopped); > } > } >diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >index 3243807..141e60b 100644 >--- a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >@@ -585,28 +585,28 @@ MediaEngineGonkVideoSource::OnUserError(UserContext aContext, nsresult aError) > MonitorAutoLock lock(mMonitor); > if (mPhotoCallbacks.Length()) { > NS_DispatchToMainThread(new TakePhotoError(mPhotoCallbacks, aError)); > } > } > } > > void >-MediaEngineGonkVideoSource::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+MediaEngineGonkVideoSource::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > { > // It needs to start preview because Gonk camera will stop preview while > // taking picture. > mCameraControl->StartPreview(); > > // Create a main thread runnable to generate a blob and call all current queued > // PhotoCallbacks. > class GenerateBlobRunnable : public nsRunnable { > public: > GenerateBlobRunnable(nsTArray<nsRefPtr<PhotoCallback>>& aCallbacks, >- uint8_t* aData, >+ const uint8_t* aData, > uint32_t aLength, > const nsAString& aMimeType) > : mPhotoDataLength(aLength) > { > mCallbacks.SwapElements(aCallbacks); > mPhotoData = (uint8_t*) malloc(aLength); > memcpy(mPhotoData, aData, mPhotoDataLength); > mMimeType = aMimeType; >diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.h b/dom/media/webrtc/MediaEngineGonkVideoSource.h >index 12d0372..b072f19 100644 >--- a/dom/media/webrtc/MediaEngineGonkVideoSource.h >+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.h >@@ -69,17 +69,17 @@ public: > SourceMediaStream* aSource, > TrackID aId, > StreamTime aDesiredTime) override; > > void OnHardwareStateChange(HardwareState aState, nsresult aReason) override; > void GetRotation(); > bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override; > void OnUserError(UserContext aContext, nsresult aError) override; >- void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; >+ void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; > > void AllocImpl(); > void DeallocImpl(); > void StartImpl(webrtc::CaptureCapability aCapability); > void StopImpl(); > uint32_t ConvertPixelFormatToFOURCC(int aFormat); > void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight); > void Notify(const mozilla::hal::ScreenConfiguration& aConfiguration);
Comment on attachment 8597733 [details] [diff] [review] bug1129051.patch, V2 From: Roger Yang <royang51@gmail.com> Bug 1129051 - Fix double free in Camera Control Listener. Fix webrtc memory leak. >diff --git a/dom/camera/CameraControlListener.h b/dom/camera/CameraControlListener.h >index 532fdaf..e489c11 100644 >--- a/dom/camera/CameraControlListener.h >+++ b/dom/camera/CameraControlListener.h >@@ -84,17 +84,17 @@ public: > public: > uint32_t mMaxMeteringAreas; > uint32_t mMaxFocusAreas; > }; > virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) { } > > virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) { } > virtual void OnAutoFocusMoving(bool aIsMoving) { } >- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { } >+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { } > virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) { } > > enum UserContext > { > kInStartCamera, > kInStopCamera, > kInAutoFocus, > kInStartFaceDetection, >diff --git a/dom/camera/DOMCameraControlListener.cpp b/dom/camera/DOMCameraControlListener.cpp >index 96f55ab..7a586c4 100644 >--- a/dom/camera/DOMCameraControlListener.cpp >+++ b/dom/camera/DOMCameraControlListener.cpp >@@ -336,28 +336,30 @@ DOMCameraControlListener::OnAutoFocusComplete(bool aAutoFocusSucceeded) > protected: > bool mAutoFocusSucceeded; > }; > > NS_DispatchToMainThread(new Callback(mDOMCameraControl, aAutoFocusSucceeded)); > } > > void >-DOMCameraControlListener::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+DOMCameraControlListener::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > { > class Callback : public DOMCallback > { > public: > Callback(nsMainThreadPtrHandle<nsISupports> aDOMCameraControl, >- uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+ const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > : DOMCallback(aDOMCameraControl) >- , mData(aData) > , mLength(aLength) > , mMimeType(aMimeType) >- { } >+ { >+ mData = (uint8_t*) malloc(aLength); >+ memcpy(mData, aData, aLength); >+ } > > void > RunCallback(nsDOMCameraControl* aDOMCameraControl) override > { > nsCOMPtr<nsIDOMBlob> picture = > File::CreateMemoryFile(mDOMCameraControl.get(), > static_cast<void*>(mData), > static_cast<uint64_t>(mLength), >diff --git a/dom/camera/DOMCameraControlListener.h b/dom/camera/DOMCameraControlListener.h >index a5c30c6..c1a1be8 100644 >--- a/dom/camera/DOMCameraControlListener.h >+++ b/dom/camera/DOMCameraControlListener.h >@@ -16,17 +16,17 @@ class CameraPreviewMediaStream; > class DOMCameraControlListener : public CameraControlListener > { > public: > DOMCameraControlListener(nsDOMCameraControl* aDOMCameraControl, CameraPreviewMediaStream* aStream); > > virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) override; > virtual void OnAutoFocusMoving(bool aIsMoving) override; > virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) override; >- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; >+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; > > virtual void OnHardwareStateChange(HardwareState aState, nsresult aReason) override; > virtual void OnPreviewStateChange(PreviewState aState) override; > virtual void OnRecorderStateChange(RecorderState aState, int32_t aStatus, int32_t aTrackNum) override; > virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) override; > virtual void OnShutter() override; > virtual void OnRateLimitPreview(bool aLimit) override; > virtual bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override; >diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp >index 3e1eee7..41a15e5 100644 >--- a/dom/camera/GonkCameraControl.cpp >+++ b/dom/camera/GonkCameraControl.cpp >@@ -1529,24 +1529,20 @@ nsGonkCameraControl::OnFacesDetected(camera_frame_metadata_t* aMetaData) > CameraControlImpl::OnFacesDetected(faces); > } > > void > nsGonkCameraControl::OnTakePictureComplete(uint8_t* aData, uint32_t aLength) > { > ReentrantMonitorAutoEnter mon(mReentrantMonitor); > >- uint8_t* data = new uint8_t[aLength]; >- >- memcpy(data, aData, aLength); >- > nsString s(NS_LITERAL_STRING("image/")); > s.Append(mFileFormat); > DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength); >- OnTakePictureComplete(data, aLength, s); >+ OnTakePictureComplete(aData, aLength, s); > > if (mResumePreviewAfterTakingPicture) { > nsresult rv = StartPreview(); > if (NS_FAILED(rv)) { > DOM_CAMERA_LOGE("Failed to restart camera preview (%x)\n", rv); > OnPreviewStateChange(CameraControlListener::kPreviewStopped); > } > } >diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >index 3243807..141e60b 100644 >--- a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >@@ -585,28 +585,28 @@ MediaEngineGonkVideoSource::OnUserError(UserContext aContext, nsresult aError) > MonitorAutoLock lock(mMonitor); > if (mPhotoCallbacks.Length()) { > NS_DispatchToMainThread(new TakePhotoError(mPhotoCallbacks, aError)); > } > } > } > > void >-MediaEngineGonkVideoSource::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+MediaEngineGonkVideoSource::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > { > // It needs to start preview because Gonk camera will stop preview while > // taking picture. > mCameraControl->StartPreview(); > > // Create a main thread runnable to generate a blob and call all current queued > // PhotoCallbacks. > class GenerateBlobRunnable : public nsRunnable { > public: > GenerateBlobRunnable(nsTArray<nsRefPtr<PhotoCallback>>& aCallbacks, >- uint8_t* aData, >+ const uint8_t* aData, > uint32_t aLength, > const nsAString& aMimeType) > : mPhotoDataLength(aLength) > { > mCallbacks.SwapElements(aCallbacks); > mPhotoData = (uint8_t*) malloc(aLength); > memcpy(mPhotoData, aData, mPhotoDataLength); > mMimeType = aMimeType; >diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.h b/dom/media/webrtc/MediaEngineGonkVideoSource.h >index 12d0372..b072f19 100644 >--- a/dom/media/webrtc/MediaEngineGonkVideoSource.h >+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.h >@@ -69,17 +69,17 @@ public: > SourceMediaStream* aSource, > TrackID aId, > StreamTime aDesiredTime) override; > > void OnHardwareStateChange(HardwareState aState, nsresult aReason) override; > void GetRotation(); > bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override; > void OnUserError(UserContext aContext, nsresult aError) override; >- void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; >+ void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; > > void AllocImpl(); > void DeallocImpl(); > void StartImpl(webrtc::CaptureCapability aCapability); > void StopImpl(); > uint32_t ConvertPixelFormatToFOURCC(int aFormat); > void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight); > void Notify(const mozilla::hal::ScreenConfiguration& aConfiguration);
Comment on attachment 8597733 [details] [diff] [review] bug1129051.patch, V2 >From: Roger Yang <royang51@gmail.com> > >Bug 1129051 - Fix Camera Control Listener double free. Fix webrtc memory leak r=royang51 > >diff --git a/dom/camera/CameraControlListener.h b/dom/camera/CameraControlListener.h >index 532fdaf..e489c11 100644 >--- a/dom/camera/CameraControlListener.h >+++ b/dom/camera/CameraControlListener.h >@@ -84,17 +84,17 @@ public: > public: > uint32_t mMaxMeteringAreas; > uint32_t mMaxFocusAreas; > }; > virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) { } > > virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) { } > virtual void OnAutoFocusMoving(bool aIsMoving) { } >- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { } >+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { } > virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) { } > > enum UserContext > { > kInStartCamera, > kInStopCamera, > kInAutoFocus, > kInStartFaceDetection, >diff --git a/dom/camera/DOMCameraControlListener.cpp b/dom/camera/DOMCameraControlListener.cpp >index 96f55ab..7a586c4 100644 >--- a/dom/camera/DOMCameraControlListener.cpp >+++ b/dom/camera/DOMCameraControlListener.cpp >@@ -336,28 +336,30 @@ DOMCameraControlListener::OnAutoFocusComplete(bool aAutoFocusSucceeded) > protected: > bool mAutoFocusSucceeded; > }; > > NS_DispatchToMainThread(new Callback(mDOMCameraControl, aAutoFocusSucceeded)); > } > > void >-DOMCameraControlListener::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+DOMCameraControlListener::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > { > class Callback : public DOMCallback > { > public: > Callback(nsMainThreadPtrHandle<nsISupports> aDOMCameraControl, >- uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+ const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > : DOMCallback(aDOMCameraControl) >- , mData(aData) > , mLength(aLength) > , mMimeType(aMimeType) >- { } >+ { >+ mData = (uint8_t*) malloc(aLength); >+ memcpy(mData, aData, aLength); >+ } > > void > RunCallback(nsDOMCameraControl* aDOMCameraControl) override > { > nsCOMPtr<nsIDOMBlob> picture = > File::CreateMemoryFile(mDOMCameraControl.get(), > static_cast<void*>(mData), > static_cast<uint64_t>(mLength), >diff --git a/dom/camera/DOMCameraControlListener.h b/dom/camera/DOMCameraControlListener.h >index a5c30c6..c1a1be8 100644 >--- a/dom/camera/DOMCameraControlListener.h >+++ b/dom/camera/DOMCameraControlListener.h >@@ -16,17 +16,17 @@ class CameraPreviewMediaStream; > class DOMCameraControlListener : public CameraControlListener > { > public: > DOMCameraControlListener(nsDOMCameraControl* aDOMCameraControl, CameraPreviewMediaStream* aStream); > > virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) override; > virtual void OnAutoFocusMoving(bool aIsMoving) override; > virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) override; >- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; >+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; > > virtual void OnHardwareStateChange(HardwareState aState, nsresult aReason) override; > virtual void OnPreviewStateChange(PreviewState aState) override; > virtual void OnRecorderStateChange(RecorderState aState, int32_t aStatus, int32_t aTrackNum) override; > virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) override; > virtual void OnShutter() override; > virtual void OnRateLimitPreview(bool aLimit) override; > virtual bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override; >diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp >index 3e1eee7..41a15e5 100644 >--- a/dom/camera/GonkCameraControl.cpp >+++ b/dom/camera/GonkCameraControl.cpp >@@ -1529,24 +1529,20 @@ nsGonkCameraControl::OnFacesDetected(camera_frame_metadata_t* aMetaData) > CameraControlImpl::OnFacesDetected(faces); > } > > void > nsGonkCameraControl::OnTakePictureComplete(uint8_t* aData, uint32_t aLength) > { > ReentrantMonitorAutoEnter mon(mReentrantMonitor); > >- uint8_t* data = new uint8_t[aLength]; >- >- memcpy(data, aData, aLength); >- > nsString s(NS_LITERAL_STRING("image/")); > s.Append(mFileFormat); > DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength); >- OnTakePictureComplete(data, aLength, s); >+ OnTakePictureComplete(aData, aLength, s); > > if (mResumePreviewAfterTakingPicture) { > nsresult rv = StartPreview(); > if (NS_FAILED(rv)) { > DOM_CAMERA_LOGE("Failed to restart camera preview (%x)\n", rv); > OnPreviewStateChange(CameraControlListener::kPreviewStopped); > } > } >diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >index 3243807..141e60b 100644 >--- a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp >@@ -585,28 +585,28 @@ MediaEngineGonkVideoSource::OnUserError(UserContext aContext, nsresult aError) > MonitorAutoLock lock(mMonitor); > if (mPhotoCallbacks.Length()) { > NS_DispatchToMainThread(new TakePhotoError(mPhotoCallbacks, aError)); > } > } > } > > void >-MediaEngineGonkVideoSource::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) >+MediaEngineGonkVideoSource::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) > { > // It needs to start preview because Gonk camera will stop preview while > // taking picture. > mCameraControl->StartPreview(); > > // Create a main thread runnable to generate a blob and call all current queued > // PhotoCallbacks. > class GenerateBlobRunnable : public nsRunnable { > public: > GenerateBlobRunnable(nsTArray<nsRefPtr<PhotoCallback>>& aCallbacks, >- uint8_t* aData, >+ const uint8_t* aData, > uint32_t aLength, > const nsAString& aMimeType) > : mPhotoDataLength(aLength) > { > mCallbacks.SwapElements(aCallbacks); > mPhotoData = (uint8_t*) malloc(aLength); > memcpy(mPhotoData, aData, mPhotoDataLength); > mMimeType = aMimeType; >diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.h b/dom/media/webrtc/MediaEngineGonkVideoSource.h >index 12d0372..b072f19 100644 >--- a/dom/media/webrtc/MediaEngineGonkVideoSource.h >+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.h >@@ -69,17 +69,17 @@ public: > SourceMediaStream* aSource, > TrackID aId, > StreamTime aDesiredTime) override; > > void OnHardwareStateChange(HardwareState aState, nsresult aReason) override; > void GetRotation(); > bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override; > void OnUserError(UserContext aContext, nsresult aError) override; >- void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; >+ void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override; > > void AllocImpl(); > void DeallocImpl(); > void StartImpl(webrtc::CaptureCapability aCapability); > void StopImpl(); > uint32_t ConvertPixelFormatToFOURCC(int aFormat); > void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight); > void Notify(const mozilla::hal::ScreenConfiguration& aConfiguration);
Comment on attachment 8597733 [details] [diff] [review] bug1129051.patch, V2 Review of attachment 8597733 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/GonkCameraControl.cpp @@ +1537,4 @@ > nsString s(NS_LITERAL_STRING("image/")); > s.Append(mFileFormat); > DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength); > + OnTakePictureComplete(aData, aLength, s); I just realized this is missing a change in CameraControlImpl.h and CameraControlImpl.cpp to add const to CameraControlImpl::OnTakePictureComplete (called here). That should generate a compiler warning (apparently non-fatal, that's weird).
Attachment #8597733 - Flags: review+ → review-
Comment on attachment 8597733 [details] [diff] [review] bug1129051.patch, V2 Review of attachment 8597733 [details] [diff] [review]: ----------------------------------------------------------------- you're correct. I will update the patch. Thanks,
Attachment #8597733 - Attachment is obsolete: true
Attached patch bug1129051.patch, V3 (obsolete) — Splinter Review
From: Roger Yang <royang51@gmail.com> Bug 1129051 - Fix Camera Control Listener double free. Fix webrtc memory leak r=royang51
Attachment #8598351 - Flags: review?(aosmond)
Attachment #8598351 - Attachment is obsolete: true
Attachment #8598351 - Flags: review?(aosmond)
Attachment #8598364 - Flags: review?(aosmond)
Comment on attachment 8598364 [details] [diff] [review] bug1129051.patch, V3 Review of attachment 8598364 [details] [diff] [review]: ----------------------------------------------------------------- Much better, thanks. I'll try landing this again today.
Attachment #8598364 - Flags: review?(aosmond) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
Depends on: 1160428
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: