Merge pull request #4451 from MaxWhere/framesubscriber-fix

Fixing FrameSubscriber memory issue

atom/browser/api/atom_api_web_contents.cc

@@ -1072,9 +1072,11 @@ void WebContents::BeginFrameSubscription(
     const FrameSubscriber::FrameCaptureCallback& callback) {
   const auto view = web_contents()->GetRenderWidgetHostView();
   if (view) {
-    scoped_ptr<FrameSubscriber> frame_subscriber(new FrameSubscriber(
-        isolate(), view->GetVisibleViewportSize(), callback));
-    view->BeginFrameSubscription(frame_subscriber.Pass());
+    FrameSubscriber* frame_subscriber = new FrameSubscriber(
+      isolate(), view->GetVisibleViewportSize(), callback);
+    scoped_ptr<FrameSubscriber::Subscriber> del_frame_subscriber(
+      frame_subscriber->GetSubscriber());
+    view->BeginFrameSubscription(del_frame_subscriber.Pass());
   }
 }
 

atom/browser/api/frame_subscriber.cc

@@ -13,28 +13,58 @@ namespace atom {
 
 namespace api {
 
+using Subscriber = FrameSubscriber::Subscriber;
+
 FrameSubscriber::FrameSubscriber(v8::Isolate* isolate,
                                  const gfx::Size& size,
                                  const FrameCaptureCallback& callback)
-    : isolate_(isolate), size_(size), callback_(callback) {
+    : isolate_(isolate), size_(size), callback_(callback), pending_frames(0) {
+  subscriber_ = new Subscriber(this);
+}
+
+Subscriber::Subscriber(
+  FrameSubscriber* frame_subscriber) : frame_subscriber_(frame_subscriber) {
 }
 
-bool FrameSubscriber::ShouldCaptureFrame(
+Subscriber::~Subscriber() {
+  frame_subscriber_->subscriber_ = NULL;
+  frame_subscriber_->RequestDestruct();
+}
+
+bool Subscriber::ShouldCaptureFrame(
     const gfx::Rect& damage_rect,
     base::TimeTicks present_time,
     scoped_refptr<media::VideoFrame>* storage,
     DeliverFrameCallback* callback) {
   *storage = media::VideoFrame::CreateFrame(
       media::PIXEL_FORMAT_YV12,
-      size_, gfx::Rect(size_), size_, base::TimeDelta());
+      frame_subscriber_->size_, gfx::Rect(frame_subscriber_->size_),
+      frame_subscriber_->size_, base::TimeDelta());
   *callback = base::Bind(&FrameSubscriber::OnFrameDelivered,
-                         base::Unretained(this), *storage);
+                         base::Unretained(frame_subscriber_), *storage);
+  frame_subscriber_->pending_frames++;
   return true;
 }
 
+Subscriber* FrameSubscriber::GetSubscriber() {
+  return subscriber_;
+}
+
+bool FrameSubscriber::RequestDestruct() {
+  bool deletable = (subscriber_ == NULL && pending_frames == 0);
+  // Destruct FrameSubscriber if we're not waiting for frames and the
+  // subscription has ended
+  if (deletable)
+    delete this;
+
+  return deletable;
+}
+
 void FrameSubscriber::OnFrameDelivered(
     scoped_refptr<media::VideoFrame> frame, base::TimeTicks, bool result) {
-  if (!result)
+  pending_frames--;
+
+  if (RequestDestruct() || subscriber_ == NULL || !result)
     return;
 
   v8::Locker locker(isolate_);

atom/browser/api/frame_subscriber.h

@@ -14,26 +14,43 @@ namespace atom {
 
 namespace api {
 
-class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber {
+class FrameSubscriber {
  public:
   using FrameCaptureCallback = base::Callback<void(v8::Local<v8::Value>)>;
 
+  // Inner class that is the actual subscriber sent to chromium
+  class Subscriber :
+    public content::RenderWidgetHostViewFrameSubscriber {
+   public:
+    explicit Subscriber(FrameSubscriber* frame_subscriber);
+
+    bool ShouldCaptureFrame(const gfx::Rect& damage_rect,
+                            base::TimeTicks present_time,
+                            scoped_refptr<media::VideoFrame>* storage,
+                            DeliverFrameCallback* callback) override;
+
+    ~Subscriber();
+   private:
+    FrameSubscriber* frame_subscriber_;
+  };
+
   FrameSubscriber(v8::Isolate* isolate,
                   const gfx::Size& size,
                   const FrameCaptureCallback& callback);
 
-  bool ShouldCaptureFrame(const gfx::Rect& damage_rect,
-                          base::TimeTicks present_time,
-                          scoped_refptr<media::VideoFrame>* storage,
-                          DeliverFrameCallback* callback) override;
+  Subscriber* GetSubscriber();
 
  private:
   void OnFrameDelivered(
       scoped_refptr<media::VideoFrame> frame, base::TimeTicks, bool);
 
+  bool RequestDestruct();
+
   v8::Isolate* isolate_;
   gfx::Size size_;
   FrameCaptureCallback callback_;
+  Subscriber* subscriber_;
+  int pending_frames;
 
   DISALLOW_COPY_AND_ASSIGN(FrameSubscriber);
 };