Repaired support for multiple timeouts in FrameManager (#945)

* Added normal frame manager test

* Caught and fixed an edge case

* Completed more tests

* Added cleanup

* Caught edge case

* Completed test/fixes

* Removed dev .only

* Consolidated code into a single definition

* Removed dev .only

lib/frame-manager.js

@@ -23,7 +23,6 @@ function FrameManager(window) {
   if (!(this instanceof FrameManager)) return new FrameManager(window);
 
   EventEmitter.call(this);
-  var subscribed = false;
   var requestedFrame = false;
   var frameRequestTimeout;
   var self = this;
@@ -32,7 +31,7 @@ function FrameManager(window) {
   this.on('removeListener', unsubscribe);
 
   function subscribe(eventName) {
-    if (!subscribed && eventName === 'data') {
+    if (!self.listenerCount('data') && eventName === 'data') {
       parent.emit('log', 'subscribing to browser window frames');
       window.webContents.beginFrameSubscription(receiveFrame);
     }
@@ -42,10 +41,15 @@ function FrameManager(window) {
     if (!self.listenerCount('data')) {
       parent.emit('log', 'unsubscribing from browser window frames')
       window.webContents.endFrameSubscription();
-      subscribed = false;
     }
   }
 
+  function cancelFrame() {
+    requestedFrame = false;
+    clearTimeout(frameRequestTimeout);
+    self.emit('data', null);
+  }
+
   function receiveFrame(buffer) {
     requestedFrame = false;
     clearTimeout(frameRequestTimeout);
@@ -81,15 +85,15 @@ function FrameManager(window) {
         }
         catch (error) {
           parent.emit('log', `Failed to attach to debugger for frame subscriptions: ${error}`);
-          this.emit('data', null);
+          cancelFrame();
           return;
         }
       }
 
       if (timeout) {
         frameRequestTimeout = setTimeout(function() {
           parent.emit('log', `FrameManager timing out after ${timeout} ms with no new rendered frames`);
-          self.emit('data', null)
+          cancelFrame();
         }, timeout);
       }
 

package.json

@@ -32,6 +32,7 @@
     "split2": "^2.0.1"
   },
   "devDependencies": {
+    "async": "~2.1.4",
     "basic-auth": "^1.0.3",
     "basic-auth-connect": "^1.0.0",
     "bluebird": "^3.4.0",

test/index.js

@@ -1463,6 +1463,139 @@ describe('Nightmare', function () {
       didSubscribe.should.be.false;
     });
 
+    it('should subscribe to frames when requested necessary', function(done) {
+      var didSubscribe = false;
+      var didUnsubscribe = false;
+      var FrameManager = require('../lib/frame-manager.js');
+      var fn;
+      var manager = FrameManager({
+        webContents: {
+          debugger: {
+            isAttached: function() { return true; },
+            sendCommand: function(command) { if (command === 'DOM.highlightRect') { fn('mock-data'); }}
+          },
+          beginFrameSubscription: function(_fn) { didSubscribe = true; fn = _fn; },
+          endFrameSubscription: function() { didUnsubscribe = true; },
+          executeJavaScript: function() {}
+        }
+      });
+      manager.requestFrame(function (data) {
+        didSubscribe.should.be.true;
+        didUnsubscribe.should.be.true;
+        data.should.equal('mock-data');
+        done();
+      });
+    });
+
+    it('should support multiple concurrent frame subscriptions', function(done) {
+      var subscribeCount = 0;
+      var unsubscribeCount = 0;
+      var FrameManager = require('../lib/frame-manager.js');
+      var fn = null;
+      var assert = require('assert');
+      var async = require('async');
+      var manager = FrameManager({
+        webContents: {
+          debugger: {
+            isAttached: function() { return true; },
+            sendCommand: function(command) {
+              if (command === 'DOM.highlightRect') {
+                setTimeout(function () {
+                  fn('mock-data');
+                }, 100);
+              }
+            }
+          },
+          beginFrameSubscription: function(_fn) { subscribeCount += 1; assert.strictEqual(fn, null); fn = _fn; },
+          endFrameSubscription: function() { unsubscribeCount += 1; fn = null; },
+          executeJavaScript: function() {}
+        }
+      });
+      async.times(2, function requestFrameFn (i, cb) {
+        manager.requestFrame(function handleFrame (data) {
+          cb(null, data);
+        });
+      }, function handleResults (err, results) {
+        if (err) { done(err); }
+        subscribeCount.should.equal(1);
+        unsubscribeCount.should.equal(1);
+        results[0].should.equal('mock-data');
+        results[1].should.equal('mock-data');
+        done();
+      });
+    });
+
+    it('should support multiple series frame subscriptions', function(done) {
+      var subscribeCount = 0;
+      var unsubscribeCount = 0;
+      var FrameManager = require('../lib/frame-manager.js');
+      var fn = null;
+      var assert = require('assert');
+      var async = require('async');
+      var manager = FrameManager({
+        webContents: {
+          debugger: {
+            isAttached: function() { return true; },
+            sendCommand: function(command) {
+              if (command === 'DOM.highlightRect') {
+                setTimeout(function () {
+                  fn('mock-data');
+                }, 100);
+              }
+            }
+          },
+          beginFrameSubscription: function(_fn) { subscribeCount += 1; assert.strictEqual(fn, null); fn = _fn; },
+          endFrameSubscription: function() { unsubscribeCount += 1; fn = null; },
+          executeJavaScript: function() {}
+        }
+      });
+      async.timesSeries(2, function requestFrameFn (i, cb) {
+        manager.requestFrame(function handleFrame (data) {
+          cb(null, data);
+        });
+      }, function handleResults (err, results) {
+        if (err) { done(err); }
+        subscribeCount.should.equal(2);
+        unsubscribeCount.should.equal(2);
+        results[0].should.equal('mock-data');
+        results[1].should.equal('mock-data');
+        done();
+      });
+    });
+
+    // DEV: We can have multiple timeouts if page is static
+    it('should support multiple series timing out frame subscriptions', function(done) {
+      var subscribeCount = 0;
+      var unsubscribeCount = 0;
+      var FrameManager = require('../lib/frame-manager.js');
+      var fn = null;
+      var assert = require('assert');
+      var async = require('async');
+      var manager = FrameManager({
+        webContents: {
+          debugger: {
+            isAttached: function() { return true; },
+            sendCommand: function() { /* Ignore command so it times out */ }
+          },
+          beginFrameSubscription: function(_fn) { subscribeCount += 1; assert.strictEqual(fn, null); fn = _fn; },
+          endFrameSubscription: function() { unsubscribeCount += 1; fn = null; },
+          executeJavaScript: function() {}
+        }
+      });
+      async.timesSeries(2, function requestFrameFn (i, cb) {
+        manager.requestFrame(function handleFrame (data) {
+          cb(null, data);
+        }, 100);
+      }, function handleResults (err, results) {
+        if (err) { done(err); }
+        subscribeCount.should.equal(2);
+        unsubscribeCount.should.equal(2);
+        should.equal(results[0], null);
+        should.equal(results[1], null);
+        done();
+      });
+    });
+
     it('should load jquery correctly', function*() {
       var loaded = yield nightmare
         .goto(fixture('rendering'))