fix(client): fix a false positive page reload error in Safari #3643
Conversation
|
✅ Build karma 511 completed (commit 57e96aab43 by @devoto13) |
|
✅ Build karma 510 completed (commit 57e96aab43 by @devoto13) |
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.
devoto13
force-pushed
the
fix-page-reload
branch
from
396b822
to
9155218
Compare
|
✅ Build karma 513 completed (commit 456e5e1f0d by @devoto13) |
|
✅ Build karma 512 completed (commit 456e5e1f0d by @devoto13) |
There was a problem hiding this comment.
This solution seems much cleaner but I don't follow the explanation.
The old code was like
karmaNavigating = true
iframe.src = policy.createURL(url)
karmaNavigating = false
So I guess you are saying that iframe.src = was triggering onbeforeunload but not synchronously on safari. Thus the trailing karmaNavigating = false ran before the handler could check it.
The new solution removes the onbeforeunload handler before iframe.src = then the iframe content (context) adds it back.
Ok I see. The new solution adds the onbeforeunload handler at the start of the context execution, then the client removes the handler whenever it is going to navigate an existing context window, thus avoiding the error on navigation without needing a flag.
| 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 |
There was a problem hiding this comment.
Why not delete childWindow.onbeforeunload?
There was a problem hiding this comment.
onbeforeunload is a property of the window object, i.e. it has getter and setter. window.onbeforeunload = undefined will change the underlying value, while delete window.onbeforeunload will remove the property descriptor. Consider the below screenshot:
It does not really matter in this case, but I think the current code is more correct.
| @@ -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 | |||
|
Merge once you consider my comments |
## [6.0.4](v6.0.3...v6.0.4) (2021-02-01) ### Bug Fixes * **cli:** temporarily disable strict parameters validation ([#3641](#3641)) ([9c755e0](9c755e0)), closes [#3625](#3625) * **client:** fix a false positive page reload error in Safari ([#3643](#3643)) ([2a57b23](2a57b23)) * ensure that Karma supports running tests on IE 11 ([#3642](#3642)) ([dbd1943](dbd1943))
|
🎉 This PR is included in version 6.0.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Thank you for this fix! We were hitting this on our v4-dev branch consistently, but on main it didn't happen even though we do test Safari there too, but a newer version. |
Includes a fix for a false positive reload page error in Safari: karma-runner/karma#3643
…runner#3643) 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.
## [6.0.4](karma-runner/karma@v6.0.3...v6.0.4) (2021-02-01) ### Bug Fixes * **cli:** temporarily disable strict parameters validation ([karma-runner#3641](karma-runner#3641)) ([9c755e0](karma-runner@9c755e0)), closes [karma-runner#3625](karma-runner#3625) * **client:** fix a false positive page reload error in Safari ([karma-runner#3643](karma-runner#3643)) ([2a57b23](karma-runner@2a57b23)) * ensure that Karma supports running tests on IE 11 ([karma-runner#3642](karma-runner#3642)) ([dbd1943](karma-runner@dbd1943))
Previous code was susceptible to the race condition in Safari browser. Specifically below piece:
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
onbeforeunloadfrom the context, but I can't think of a cleaner approach.