Closed Bug 1129051 Opened 6 years ago Closed 6 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+
https://hg.mozilla.org/mozilla-central/rev/c5607755d8fa
Status: ASSIGNED → RESOLVED
Closed: 6 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.