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)

Product:
Firefox OS Graveyard
Component:
Gaia::Camera
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox40 fixed)

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

People

(Reporter: aosmond, Assigned: royang51)

References

Details

Attachments

(1 file, 4 obsolete files)

bug1129051.patch, V3
9.82 KB, patch
aosmond
: review+
Details | Diff | Splinter Review 
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
Blocks: camera-backlog
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: 10 years ago

          status-firefox40:
          --- → fixed
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: