Skip to content

Commit

Permalink
fix(client): fix a false positive page reload error in Safari (#3643)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
devoto13 authored Feb 1, 2021
1 parent 9c755e0 commit 2a57b23
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 31 deletions.
18 changes: 6 additions & 12 deletions client/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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-')
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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!')
}
}

Expand Down
6 changes: 3 additions & 3 deletions context/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
6 changes: 3 additions & 3 deletions static/context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
18 changes: 6 additions & 12 deletions static/karma.js
Original file line number Diff line number Diff line change
Expand Up @@ -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-')
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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!')
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/client/karma.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('Karma', function () {
}
}
socket = new MockSocket()
iframe = {}
iframe = { contentWindow: {} }
windowNavigator = {}
windowLocation = { search: '' }
windowStub = sinon.stub().returns({})
Expand Down

0 comments on commit 2a57b23

Please sign in to comment.