Better error reporting for window.location probably changed (onbeforeunload handler) #173
Assignees
Labels
Comments
|
@jonathanolson any worries about the following patch? Subject: [PATCH] I think we want this.
---
Index: js/server/Test.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/server/Test.js b/js/server/Test.js
--- a/js/server/Test.js (revision 9d01c74386ae4318b820325b3bd7db0ab294786d)
+++ b/js/server/Test.js (date 1682709370398)
@@ -202,7 +202,7 @@
let url;
if ( this.type === 'sim-test' ) {
- url = `sim-test.html?url=${encodeURIComponent( `${baseURL}/${this.url}` )}`;
+ url = `sim-test.html?failIfNoLoad&url=${encodeURIComponent( `${baseURL}/${this.url}` )}`;
if ( this.queryParameters ) {
url += `&simQueryParameters=${encodeURIComponent( this.queryParameters )}`;
@@ -215,7 +215,7 @@
url = `pageload-test.html?url=${encodeURIComponent( `${baseURL}/${this.url}` )}`;
}
else if ( this.type === 'wrapper-test' ) {
- url = `wrapper-test.html?url=${encodeURIComponent( `${baseURL}/${this.url}` )}`;
+ url = `wrapper-test.html?failIfNoLoad&url=${encodeURIComponent( `${baseURL}/${this.url}` )}`;
}
if ( this.testQueryParameters ) {
url = `${url}&${this.testQueryParameters}`;
|
|
@jonathanolson and I talked about two more potential sources of this onbeforeunload error. Let's investigate both approaches.
|
|
This test shows that we are in fact not getting the onbeforeunload from puppeteer whatsoever: It isn't until I add this patch that I begin receiving any onbeforeunload events both in the parent and in the iframe. So perhaps the puppeteer closing as the problem was a false assumption on my part. Subject: [PATCH] do that unload
---
Index: js/common/browserPageLoad.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/browserPageLoad.js b/js/common/browserPageLoad.js
--- a/js/common/browserPageLoad.js (revision f07ceb27412350e2c82214dd329f63c260d0f16f)
+++ b/js/common/browserPageLoad.js (date 1683048882674)
@@ -108,7 +108,7 @@
timeout: options.gotoTimeout
} );
const result = await promise;
- !page.isClosed() && await page.close();
+ !page.isClosed() && await page.close( { runBeforeUnload: true } );
// If we created a temporary browser, close it
ownsBrowser && await browser.close(); |
zepumph
added a commit
to phetsims/joist
that referenced
this issue
May 2, 2023
zepumph
added a commit
to phetsims/perennial
that referenced
this issue
May 2, 2023
zepumph
added a commit
to phetsims/scenery
that referenced
this issue
May 2, 2023
jonathanolson
added a commit
to phetsims/perennial
that referenced
this issue
May 3, 2023
zepumph
added a commit
that referenced
this issue
May 3, 2023
|
@jonathanolson, after #177 and a pull, you can use |
|
In 1117c60 I changed it so that by default we don't see window.location errors. Feel free to change the default of that error, but in general that seems like an error for you and me to note, and not any others. |
|
We finally got our first logging from out new node client (#178): |
|
@jonathanolson, I don't see a lot from the above. Do you think it is related to the "Context Lost" logging? Would you like to put a couple more logs in here to try to understand the problem a bit more? |
zepumph
added a commit
that referenced
this issue
May 10, 2023
zepumph
added a commit
that referenced
this issue
May 11, 2023
jonathanolson
added a commit
to phetsims/mobius
that referenced
this issue
May 11, 2023
|
phetsims/mobius@d38c78d will most likely fix the ThreeJS WebGL for density and buoyancy. Those are the only window.location errors that we are seeing on the new Node client, which feels like a good reason to go all in on it over in #178. |
|
|
There have been no window.location errors on the new node client since phetsims/mobius@d38c78d. |
zepumph
added a commit
that referenced
this issue
May 12, 2023
|
Alright! the gross hacky ?showBeforeUnloadErrors is gone now. Closing |
zepumph
pushed a commit
to phetsims/mobius
that referenced
this issue
Oct 19, 2023
…ms/aqua#173 (cherry picked from commit d38c78d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
From https://github.com/phetsims/special-ops/issues/234, @jonathanolson and I want to investigate improving the experience for this.
A few ideas:
Report the whole console logging with the errorSee ifconsole.error()occurs and treat those as Errors.failIfNoLoadhelps here.We think that much of this is from a puppeteer client closing after 15 minutes, what if instead it was every hour or 2?The text was updated successfully, but these errors were encountered: