fix(client): fix a false positive page reload error in Safari (#3643)

devoto13 · web-flow · commit 2a57b230cd6b · 2021-02-01T11:55:02.000-08:00
Previous code was susceptible to the race condition in Safari browser. Specifically below piece:

```
iframe.src = policy.createURL(url)
karmaNavigating = false
```

There is no guarantee that onbeforeunload event will be triggered synchronously after setting `iframe.src`. It was the case in Chrome and Firefox, but not in Safari, where this caused an error every test run. New approach resets the onbeforeunload handler before navigation is triggered by Karma itself to avoid the need for synchronization and this race condition altogether. The handler will be restored by the new context once it is loaded.

PS Code is a bit fragile as there is an implicit dependency on `onbeforeunload` from the context, but I can't think of a cleaner approach.
diff --git a/client/karma.js b/client/karma.js
@@ -5,7 +5,6 @@ var util = require('../common/util')
 function Karma (updater, socket, iframe, opener, navigator, location, document) {
   this.updater = updater
   var startEmitted = false
-  var karmaNavigating = false
   var self = this
   var queryParams = util.parseQueryParams(location.search)
   var browserId = queryParams.id || util.generateId('manual-')
@@ -83,21 +82,21 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
 
   var childWindow = null
   function navigateContextTo (url) {
-    karmaNavigating = true
     if (self.config.useIframe === false) {
       // run in new window
       if (self.config.runInParent === false) {
         // If there is a window already open, then close it
         // DEV: In some environments (e.g. Electron), we don't have setter access for location
         if (childWindow !== null && childWindow.closed !== true) {
+          // The onbeforeunload listener was added by context to catch
+          // unexpected navigations while running tests.
+          childWindow.onbeforeunload = undefined
           childWindow.close()
         }
         childWindow = opener(url)
-        karmaNavigating = false
       // run context on parent element (client_with_context)
       // using window.__karma__.scriptUrls to get the html element strings and load them dynamically
       } else if (url !== 'about:blank') {
-        karmaNavigating = false
         var loadScript = function (idx) {
           if (idx < window.__karma__.scriptUrls.length) {
             var parser = new DOMParser()
@@ -128,15 +127,10 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
       }
     // run in iframe
     } else {
+      // The onbeforeunload listener was added by the context to catch
+      // unexpected navigations while running tests.
+      iframe.contentWindow.onbeforeunload = undefined
       iframe.src = policy.createURL(url)
-      karmaNavigating = false
-    }
-  }
-
-  this.onbeforeunload = function () {
-    if (!karmaNavigating) {
-      // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL)
-      self.error('Some of your tests did a full page reload!')
     }
   }
 
diff --git a/context/karma.js b/context/karma.js
@@ -77,9 +77,9 @@ function ContextKarma (callParentKarmaMethod) {
     contextWindow.onerror = function () {
       return self.error.apply(self, arguments)
     }
-    // DEV: We must defined a function since we don't want to pass the event object
-    contextWindow.onbeforeunload = function (e, b) {
-      callParentKarmaMethod('onbeforeunload', [])
+
+    contextWindow.onbeforeunload = function () {
+      return self.error('Some of your tests did a full page reload!')
     }
 
     contextWindow.dump = function () {
diff --git a/static/context.js b/static/context.js
@@ -214,9 +214,9 @@ function ContextKarma (callParentKarmaMethod) {
     contextWindow.onerror = function () {
       return self.error.apply(self, arguments)
     }
-    // DEV: We must defined a function since we don't want to pass the event object
-    contextWindow.onbeforeunload = function (e, b) {
-      callParentKarmaMethod('onbeforeunload', [])
+
+    contextWindow.onbeforeunload = function () {
+      return self.error('Some of your tests did a full page reload!')
     }
 
     contextWindow.dump = function () {
diff --git a/static/karma.js b/static/karma.js
@@ -15,7 +15,6 @@ var util = require('../common/util')
 function Karma (updater, socket, iframe, opener, navigator, location, document) {
   this.updater = updater
   var startEmitted = false
-  var karmaNavigating = false
   var self = this
   var queryParams = util.parseQueryParams(location.search)
   var browserId = queryParams.id || util.generateId('manual-')
@@ -93,21 +92,21 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
 
   var childWindow = null
   function navigateContextTo (url) {
-    karmaNavigating = true
     if (self.config.useIframe === false) {
       // run in new window
       if (self.config.runInParent === false) {
         // If there is a window already open, then close it
         // DEV: In some environments (e.g. Electron), we don't have setter access for location
         if (childWindow !== null && childWindow.closed !== true) {
+          // The onbeforeunload listener was added by context to catch
+          // unexpected navigations while running tests.
+          childWindow.onbeforeunload = undefined
           childWindow.close()
         }
         childWindow = opener(url)
-        karmaNavigating = false
       // run context on parent element (client_with_context)
       // using window.__karma__.scriptUrls to get the html element strings and load them dynamically
       } else if (url !== 'about:blank') {
-        karmaNavigating = false
         var loadScript = function (idx) {
           if (idx < window.__karma__.scriptUrls.length) {
             var parser = new DOMParser()
@@ -138,15 +137,10 @@ function Karma (updater, socket, iframe, opener, navigator, location, document)
       }
     // run in iframe
     } else {
+      // The onbeforeunload listener was added by the context to catch
+      // unexpected navigations while running tests.
+      iframe.contentWindow.onbeforeunload = undefined
       iframe.src = policy.createURL(url)
-      karmaNavigating = false
-    }
-  }
-
-  this.onbeforeunload = function () {
-    if (!karmaNavigating) {
-      // TODO(vojta): show what test (with explanation about jasmine.UPDATE_INTERVAL)
-      self.error('Some of your tests did a full page reload!')
     }
   }
 
diff --git a/test/client/karma.spec.js b/test/client/karma.spec.js
@@ -22,7 +22,7 @@ describe('Karma', function () {
       }
     }
     socket = new MockSocket()
-    iframe = {}
+    iframe = { contentWindow: {} }
     windowNavigator = {}
     windowLocation = { search: '' }
     windowStub = sinon.stub().returns({})

Original file line number	Diff line number	Diff line change
`@@ -77,9 +77,9 @@ function ContextKarma (callParentKarmaMethod) {`
`77`	`77`	`contextWindow.onerror = function () {`
`78`	`78`	`return self.error.apply(self, arguments)`
`79`	`79`	`}`
`80`		`- // DEV: We must defined a function since we don't want to pass the event object`
`81`		`- contextWindow.onbeforeunload = function (e, b) {`
`82`		`- callParentKarmaMethod('onbeforeunload', [])`
	`80`	`+`
	`81`	`+ contextWindow.onbeforeunload = function () {`
	`82`	`+ return self.error('Some of your tests did a full page reload!')`
`83`	`83`	`}`
`84`	`84`
`85`	`85`	`contextWindow.dump = function () {`
Original file line number	Diff line number	Diff line change
`@@ -214,9 +214,9 @@ function ContextKarma (callParentKarmaMethod) {`
`214`	`214`	`contextWindow.onerror = function () {`
`215`	`215`	`return self.error.apply(self, arguments)`
`216`	`216`	`}`
`217`		`- // DEV: We must defined a function since we don't want to pass the event object`
`218`		`- contextWindow.onbeforeunload = function (e, b) {`
`219`		`- callParentKarmaMethod('onbeforeunload', [])`
	`217`	`+`
	`218`	`+ contextWindow.onbeforeunload = function () {`
	`219`	`+ return self.error('Some of your tests did a full page reload!')`
`220`	`220`	`}`
`221`	`221`
`222`	`222`	`contextWindow.dump = function () {`
Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@ describe('Karma', function () {`
`22`	`22`	`}`
`23`	`23`	`}`
`24`	`24`	`socket = new MockSocket()`
`25`		`- iframe = {}`
	`25`	`+ iframe = { contentWindow: {} }`
`26`	`26`	`windowNavigator = {}`
`27`	`27`	`windowLocation = { search: '' }`
`28`	`28`	`windowStub = sinon.stub().returns({})`