-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wp-now: environment crashes when I try to save post #416
Comments
Maybe it's Jetpack making outbound HTTP requests? Potentially related #396 |
@danielbachhuber, it may be, but I'm not sure how to debug it more. I've just imported the database dump to have the Jetpack connection active, started wp-now, accepted the Jetpack safe mode, saved the post, and it crashes again. |
A stack trace would be so super helpful here. Building with Assertions, no optimizations, and MM error reporting could yield a stack trace. If not, then here's the next best thing:
With the stack trace, we could poke around and figure out what to do next. |
@adamziel thanks for providing more details. I've made those changes in Dockerfile, recompiled PHP and ran build:
Then, I started the server using
I've checked paths from the stack trace but can't see anything obvious. |
const rethrown = new Error(message);
rethrown.cause = err;
throw rethrown; The console message doesn't seem to give us the original one with WASM details in it. What would it say if you added an explicit |
@adamziel it looks like it's this one, so it was included as
|
Oh, I see! So it has no |
@adamziel no, just the ExitStatus object I included in previous comments. I searched more for the I thought it might be caused by I also noticed that I don't need to save the post to crash it. It crashes as soon as I load the post edit form and refresh it. I can refresh any other page normally, and I reproduce the crash only on post and page forms. |
@wojtekn oh, it seems like Emscripten throws an instance of a custom
We should patch it to expose a stack trace. Perhaps overriding Either way, try manually replacing
PHP may not provide any, but we’re running it as WebAssembly in JavaScript program and have extra powers. Emscripten JS module controls the entire execution flow. We could e.g. make it log every PHP function called. We should be able to get the trace when it crashes. The fact that we’re not getting one now seems like a bug, not a limitation. |
@adamziel I've replaced that ExitStatus class with the following:
And logged exception using
|
@adamziel I've checked emscripten repository for that, and it looks like this change was made deliberately to fix some memory leak issue in Chrome: |
@wojtekn excellent find! We may ignore that tbh – Emscripten, the framework, made the right call here, but for us the entire runtime is about PHP. If the PHP dies, we may kill the entire process and reclaim any memory that leaked in the process of shutting down the WebAssembly module. Exposing the stack trace by default will make debugging all such issues much easier. Probably even the pthreads one. I filed a separate issue here: #450 |
The stack trace is extremely helpful, thank you for taking the time to provide it @wojtekn! The call to The |
…race attached (#470) ## Description Overrides Emscripten's default ExitStatus object which gets thrown on failure. Unfortunately, the default object is not a subclass of Error and does not provide any stack trace. This is a deliberate behavior on Emscripten's end to prevent memory leaks after the program exits. See: emscripten-core/emscripten#9108 In case of WordPress Playground, the worker in which the PHP runs will typically exit after the PHP program finishes, so we don't have to worry about memory leaks. As for assigning to a previously undeclared ExitStatus variable here, the Emscripten module declares `ExitStatus` as `function ExitStatus` which means it gets hoisted to the top of the scope and can be reassigned here – before the actual declaration is reached. If that sounds weird, try this example: ```js ExitStatus = () => { console.log("reassigned"); } function ExitStatus() {} ExitStatus(); // logs "reassigned" ``` ## Testing instructions Confirm the CI tests passed Related: #416 cc @wojtekn
@adamziel I found an open PHP bug https://bugs.php.net/bug.php?id=79781 which ends up with a slightly similar result. I ran the test script using wp-now and reproduced it. The stack trace looked as follows:
And one for my blog is:
|
@wojtekn Great find! I'm seeing a similar result, but including a memory exhausted error from PHP:
We set |
I'm leaving this issue at this time as I don't have enough bandwidth. |
Thanks @wojtekn for your dedication here; totally understandable. I struggle myself with getting the bandwidth to follow-up on stuff like this issue. We'll get to it at some point. |
…race attached (#470) ## Description Overrides Emscripten's default ExitStatus object which gets thrown on failure. Unfortunately, the default object is not a subclass of Error and does not provide any stack trace. This is a deliberate behavior on Emscripten's end to prevent memory leaks after the program exits. See: emscripten-core/emscripten#9108 In case of WordPress Playground, the worker in which the PHP runs will typically exit after the PHP program finishes, so we don't have to worry about memory leaks. As for assigning to a previously undeclared ExitStatus variable here, the Emscripten module declares `ExitStatus` as `function ExitStatus` which means it gets hoisted to the top of the scope and can be reassigned here – before the actual declaration is reached. If that sounds weird, try this example: ```js ExitStatus = () => { console.log("reassigned"); } function ExitStatus() {} ExitStatus(); // logs "reassigned" ``` ## Testing instructions Confirm the CI tests passed Related: WordPress/wordpress-playground#416 cc @wojtekn
I tested WordPress/playground-tools#110 along with #639, and unfortunately it still crashes. |
Can you try this one: #870 ? It looks like these are actually two separate issues. |
I couldn't reproduce my site crashing with the #870 anymore. However, now it doesn't crash on the latest |
@wojtekn oh, interesting! So did WordPress/playground-tools@29dba5d fix it? As in, does it crash one commit before that? I wonder if some change in the Playground repo helped here, or was it perhaps a new Node.js release. |
Actually no, that shouldn't fix it because it did not change any Playground dependencies. So weird! |
I fixed the stack overflow error related to the POST request: I still don't have any updates about the the crash related to simply loading the website. |
@adamziel as I no longer reproduce it, should we close? |
@wojtekn Yeah, let's close and reopen if this problem ever returns. I went through the logs in this issue once again just to be sure and some of them are familiar and seem related to some WASM issues we've solved lately. |
I tried using wp-now for a blog, but it kept crashing when I saved the post. It preserved changes in the post being saved, though.
The output looked as follows:
The site has the following plugins installed:
Google Authenticator 0.54I tried to identify which plugin may cause it. I disabled all and was able to save posts without a crash. Then I started enabling them one by one to see when it starts crashing. It hasn't started crashing again, but I couldn't enable Jetpack as it displays "An error occurred. Please try again." when I click "Set up Jetpack", so I think it may be connected to Jetpack.
The text was updated successfully, but these errors were encountered: