fix(client): avoid race between execute and clearContext (#3452)

johnjbarton · web-flow · commit 8bc5b468393b · 2020-08-10T16:37:49.000-07:00
Add a delay in execute to ensure that reload events and clear context events are completed first. Fixes #3424
diff --git a/client/karma.js b/client/karma.js
@@ -131,6 +131,13 @@ function Karma (socket, iframe, opener, navigator, location, document) {
       // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL)
       self.error('Some of your tests did a full page reload!')
     }
+    reloadingContext = false
+  }
+
+  function clearContext () {
+    reloadingContext = true
+
+    navigateContextTo('about:blank')
   }
 
   this.log = function (type, args) {
@@ -145,12 +152,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {
 
   this.stringify = stringify
 
-  function clearContext () {
-    reloadingContext = true
-
-    navigateContextTo('about:blank')
-  }
-
   function getLocation (url, lineno, colno) {
     var location = ''
 
@@ -234,11 +235,10 @@ function Karma (socket, iframe, opener, navigator, location, document) {
     }
 
     if (self.config.clearContext) {
-      // give the browser some time to breath, there could be a page reload, but because a bunch of
-      // tests could run in the same event loop, we wouldn't notice.
-      setTimeout(function () {
-        clearContext()
-      }, 0)
+      // A test could have incorrectly issued a navigate. To clear the context
+      // we will navigate the iframe. Delay ours to ensure the error from an
+      // incorrect navigate is processed.
+      setTimeout(clearContext)
     }
 
     socket.emit('complete', result || {}, function () {
@@ -259,24 +259,26 @@ function Karma (socket, iframe, opener, navigator, location, document) {
   }
 
   socket.on('execute', function (cfg) {
-    // reset startEmitted and reload the iframe
-    startEmitted = false
-    self.config = cfg
-    // if not clearing context, reloadingContext always true to prevent beforeUnload error
-    reloadingContext = !self.config.clearContext
-    navigateContextTo(constant.CONTEXT_URL)
-
-    if (self.config.clientDisplayNone) {
-      [].forEach.call(document.querySelectorAll('#banner, #browsers'), function (el) {
-        el.style.display = 'none'
-      })
-    }
+    // Delay our navigation to the next event in case the clearContext has not completed.
+    setTimeout(function allowClearContextToComplete () {
+      // reset startEmitted and reload the iframe
+      startEmitted = false
+      self.config = cfg
+
+      navigateContextTo(constant.CONTEXT_URL)
+
+      if (self.config.clientDisplayNone) {
+        [].forEach.call(document.querySelectorAll('#banner, #browsers'), function (el) {
+          el.style.display = 'none'
+        })
+      }
 
-    // clear the console before run
-    // works only on FF (Safari, Chrome do not allow to clear console from js source)
-    if (window.console && window.console.clear) {
-      window.console.clear()
-    }
+      // clear the console before run
+      // works only on FF (Safari, Chrome do not allow to clear console from js source)
+      if (window.console && window.console.clear) {
+        window.console.clear()
+      }
+    })
   })
   socket.on('stop', function () {
     this.complete()
diff --git a/static/karma.js b/static/karma.js
@@ -141,6 +141,13 @@ function Karma (socket, iframe, opener, navigator, location, document) {
       // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL)
       self.error('Some of your tests did a full page reload!')
     }
+    reloadingContext = false
+  }
+
+  function clearContext () {
+    reloadingContext = true
+
+    navigateContextTo('about:blank')
   }
 
   this.log = function (type, args) {
@@ -155,12 +162,6 @@ function Karma (socket, iframe, opener, navigator, location, document) {
 
   this.stringify = stringify
 
-  function clearContext () {
-    reloadingContext = true
-
-    navigateContextTo('about:blank')
-  }
-
   function getLocation (url, lineno, colno) {
     var location = ''
 
@@ -244,11 +245,10 @@ function Karma (socket, iframe, opener, navigator, location, document) {
     }
 
     if (self.config.clearContext) {
-      // give the browser some time to breath, there could be a page reload, but because a bunch of
-      // tests could run in the same event loop, we wouldn't notice.
-      setTimeout(function () {
-        clearContext()
-      }, 0)
+      // A test could have incorrectly issued a navigate. To clear the context
+      // we will navigate the iframe. Delay ours to ensure the error from an
+      // incorrect navigate is processed.
+      setTimeout(clearContext)
     }
 
     socket.emit('complete', result || {}, function () {
@@ -269,24 +269,26 @@ function Karma (socket, iframe, opener, navigator, location, document) {
   }
 
   socket.on('execute', function (cfg) {
-    // reset startEmitted and reload the iframe
-    startEmitted = false
-    self.config = cfg
-    // if not clearing context, reloadingContext always true to prevent beforeUnload error
-    reloadingContext = !self.config.clearContext
-    navigateContextTo(constant.CONTEXT_URL)
-
-    if (self.config.clientDisplayNone) {
-      [].forEach.call(document.querySelectorAll('#banner, #browsers'), function (el) {
-        el.style.display = 'none'
-      })
-    }
+    // Delay our navigation to the next event in case the clearContext has not completed.
+    setTimeout(function allowClearContextToComplete () {
+      // reset startEmitted and reload the iframe
+      startEmitted = false
+      self.config = cfg
+
+      navigateContextTo(constant.CONTEXT_URL)
+
+      if (self.config.clientDisplayNone) {
+        [].forEach.call(document.querySelectorAll('#banner, #browsers'), function (el) {
+          el.style.display = 'none'
+        })
+      }
 
-    // clear the console before run
-    // works only on FF (Safari, Chrome do not allow to clear console from js source)
-    if (window.console && window.console.clear) {
-      window.console.clear()
-    }
+      // clear the console before run
+      // works only on FF (Safari, Chrome do not allow to clear console from js source)
+      if (window.console && window.console.clear) {
+        window.console.clear()
+      }
+    })
   })
   socket.on('stop', function () {
     this.complete()
diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js
@@ -44,31 +44,40 @@ describe('Karma', function () {
     assert(startSpy.calledWith(config))
   })
 
-  it('should open a new window when useIFrame is false', function () {
+  it('should open a new window when useIFrame is false', function (done) {
     var config = ck.config = {
       useIframe: false,
       runInParent: false
     }
 
     socket.emit('execute', config)
-    assert(!ck.start.called)
+    setTimeout(function nextEventLoop () {
+      assert(!ck.start.called)
 
-    ck.loaded()
-    assert(startSpy.calledWith(config))
-    assert(windowStub.calledWith('context.html'))
+      ck.loaded()
+      assert(startSpy.calledWith(config))
+      assert(windowStub.calledWith('context.html'))
+      done()
+    })
   })
 
-  it('should not set style on elements', function () {
+  it('should not set style on elements', function (done) {
     var config = {}
     socket.emit('execute', config)
-    assert(Object.keys(elements[0].style).length === 0)
+    setTimeout(function nextEventLoop () {
+      assert(Object.keys(elements[0].style).length === 0)
+      done()
+    })
   })
 
-  it('should set display none on elements if clientDisplayNone', function () {
+  it('should set display none on elements if clientDisplayNone', function (done) {
     var config = { clientDisplayNone: true }
     socket.emit('execute', config)
-    assert(elements[0].style.display === 'none')
-    assert(elements[1].style.display === 'none')
+    setTimeout(function nextEventLoop () {
+      assert(elements[0].style.display === 'none')
+      assert(elements[1].style.display === 'none')
+      done()
+    })
   })
 
   it('should stop execution', function () {
@@ -97,55 +106,65 @@ describe('Karma', function () {
     assert.notStrictEqual(k.start, ADAPTER_START_FN)
   })
 
-  it('should not set up context if there was an error', function () {
+  it('should not set up context if there was an error', function (done) {
     var config = ck.config = {
       clearContext: true
     }
 
     socket.emit('execute', config)
 
-    var mockWindow = {}
+    setTimeout(function nextEventLoop () {
+      var mockWindow = {}
 
-    ck.error('page reload')
-    ck.setupContext(mockWindow)
+      ck.error('page reload')
+      ck.setupContext(mockWindow)
 
-    assert(mockWindow.onbeforeunload == null)
-    assert(mockWindow.onerror == null)
+      assert(mockWindow.onbeforeunload == null)
+      assert(mockWindow.onerror == null)
+      done()
+    })
   })
 
-  it('should setup context if there was error but clearContext config is false', function () {
+  it('should setup context if there was error but clearContext config is false', function (done) {
     var config = ck.config = {
       clearContext: false
     }
 
     socket.emit('execute', config)
 
-    var mockWindow = {}
+    setTimeout(function nextEventLoop () {
+      var mockWindow = {}
 
-    ck.error('page reload')
-    ck.setupContext(mockWindow)
+      ck.error('page reload')
+      ck.setupContext(mockWindow)
 
-    assert(mockWindow.onbeforeunload != null)
-    assert(mockWindow.onerror != null)
+      assert(mockWindow.onbeforeunload != null)
+      assert(mockWindow.onerror != null)
+      done()
+    })
   })
 
-  it('should error out if a script attempted to reload the browser after setup', function () {
+  it('should error out if a script attempted to reload the browser after setup', function (done) {
     // Perform setup
     var config = ck.config = {
       clearContext: true
     }
     socket.emit('execute', config)
-    var mockWindow = {}
-    ck.setupContext(mockWindow)
 
-    // Spy on our error handler
-    sinon.spy(k, 'error')
+    setTimeout(function nextEventLoop () {
+      var mockWindow = {}
+      ck.setupContext(mockWindow)
+
+      // Spy on our error handler
+      sinon.spy(k, 'error')
 
-    // Emulate an unload event
-    mockWindow.onbeforeunload()
+      // Emulate an unload event
+      mockWindow.onbeforeunload()
 
-    // Assert our spy was called
-    assert(k.error.calledWith('Some of your tests did a full page reload!'))
+      // Assert our spy was called
+      assert(k.error.calledWith('Some of your tests did a full page reload!'))
+      done()
+    })
   })
 
   it('should report navigator name', function () {
@@ -439,12 +458,10 @@ describe('Karma', function () {
       }
 
       socket.emit('execute', config)
+      clock.tick(1)
       var CURRENT_URL = iframe.src
-
       ck.complete()
-
       clock.tick(1)
-
       assert.strictEqual(iframe.src, CURRENT_URL)
     })
 
@@ -455,6 +472,7 @@ describe('Karma', function () {
       }
 
       socket.emit('execute', config)
+      clock.tick(1)
       assert(!startSpy.called)
 
       ck.loaded()
