Memory access out of bounds with WooCommerce development plugin
samueljseay opened this issue · 20 comments
I had a look through the issues before raising this, I am aware it's quite similar to #1131 (comment)
I recently started implementing branch previews for WooCommerce with Playground and when WooCommerce loads I get request time out errors and memory out of bounds errors like:
Uncaught (in promise) Error: memory access out of bounds
at #handleRequest (worker-thread-8ad48778.js?wpVersion=6.5&phpVersion=8.0&php-extension=iconv&php-extension=mbstring&p…:48:3538)
at async WebPHP.run (worker-thread-8ad48778.js?wpVersion=6.5&phpVersion=8.0&php-extension=iconv&php-extension=mbstring&p…:37:15194)
at async #u (worker-thread-8ad48778.js?wpVersion=6.5&phpVersion=8.0&php-extension=iconv&php-extension=mbstring&p…:54:2744)
at async #l (worker-thread-8ad48778.js?wpVersion=6.5&phpVersion=8.0&php-extension=iconv&php-extension=mbstring&p…:54:2315)
at async PlaygroundWorkerEndpoint.request (worker-thread-8ad48778.js?wpVersion=6.5&phpVersion=8.0&php-extension=iconv&php-extension=mbstring&p…:58:9271)
You can reproduce with this playground link:
Pinging @brandonpayton to take a look as requested by Adam.
Thanks all 🙇
Hi @samueljseay, I am planning to take a look at this next.
Actually, it sounds like we are going to wait a bit on #134 for now because JSPI should hopefully solve this class of issue.
@brandonpayton is this the same issue, though? This one isn’t „unreachable statement” but „memory access out of bounds”. Let’s give it a try with JSPI and see if it’s still happening.
@adamziel, that makes sense. I made a note to try this with your JSPI branch on Monday.
The repro link is not currently working for me. I'm encountering the error:
GET https://playground.wordpress.net/plugin-proxy.php?org=woocommerce&repo=woocommerce&workflow=Build%20Live%20Branch&artifact=plugins-8797272447&pr=46761 400 (Invalid request)
I'm not yet sure if this is an issue with Playground, the website, or the repro URL and plan to look into it more tomorrow.
The current artifact expiry in the above Woo PR is 7 days, and the original artifact is now expired. I re-ran the workflow to obtain a new artifact. The following link should work for the next week:
I tested this with a JSPI build in the hope that we could avoid possible asyncify issues which may be red herrings.
When I build PHP 8.0 with debug info and debug in Chrome Canary, it looks like Playground is trying to run /wordpress/wp-content/plugins/plugin-proxy.php. This seems wrong, and I believe the artifact downloaded from plugin-proxy.php should have been saved to a different zip file name. And if not, shouldn't it be trying to run something from within the zip file rather than a file named "plugin-proxy.php"?
Separately, I manually downloaded the artifact zip file and installed the plugin in Playground by uploading the zip. This worked fine, and there were no apparent memory-related errors.
Currently, I am wondering whether this is a case where Playground is trying to run a non-existent PHP file or trying to run a non-PHP file as PHP. And the early error might be running afoul of the assumptions of our wasm_memory_storage extension. Perhaps the extension is asked to free memory that was allocated a different way before wasm_memory_storage was installed.
Next I am planning to test Playground without the wasm_memory_storage extension to see if this is the case or whether we have another kind of problem.
An error occurs with this blueprint, regardless of whether wasm_memory_storage is enabled -- tested in both Chrome and Chrome Canary.
There is a mix of the following messages occurring multiple times:
ExitStatus: Program terminated with exit(1)Error: memory access out of bounds
When testing with the JSPI branch, there is just a mix of exit-related messages like:
Error: Program terminated with exit(1)Error: PHP.run() failed with exit code -1 and the following output:-- note: actual output appears to be empty
I think the difference between the JSPI and Asyncify branches may indicate this is more of a PHP execution issue. A next step is to continue debugging with the JSPI branch to learn more.
When I build PHP 8.0 with debug info and debug in Chrome Canary, it looks like Playground is trying to run /wordpress/wp-content/plugins/plugin-proxy.php. This seems wrong, and I believe the artifact downloaded from plugin-proxy.php should have been saved to a different zip file name. And if not, shouldn't it be trying to run something from within the zip file rather than a file named "plugin-proxy.php"?
Yes I noticed this too, all the assets are loaded under this file path.
@adamziel @samueljseay This issue seems to be related to the plugin folder being named like it is a PHP file plugin-proxy.php.
I changed packages/playground/blueprints/src/lib/steps/install-asset.ts to use a different name, and Playground loaded without crashing.
/**
* Install asset: Extract folder from zip file and move it to target
*/
export async function installAsset(
playground: UniversalPHP,
{
targetPath,
zipFile,
ifAlreadyInstalled = 'overwrite',
}: InstallAssetOptions
): Promise<{
assetFolderPath: string;
assetFolderName: string;
}> {
// Extract to temporary folder so we can find asset folder name
const zipFileName = zipFile.name;
- const assetNameGuess = zipFileName.replace(/\.zip$/, '');
+ let assetNameGuess = zipFileName.replace(/\.zip$/, '');
+ if ( assetNameGuess === 'plugin-proxy.php') {
+ assetNameGuess = 'PluginFromProxy';
+ }@samueljseay, it looks like there is a workaround that may unblock you before we fix this. If the artifact zip has a single root folder, that is the name used by Playground instead of "plugin-proxy.php". This is based on the code here:
@adamziel I think this is probably where we are running into trouble:
The asset paths are like:
http://127.0.0.1:5400/scope:0.4309312241391197/wp-content/plugins/plugin-proxy.php/assets/client/admin/components/style.css?ver=25bd75adba173819bfd7
And IIUC Playground will try to run such non-PHP files as PHP because they have .php/ in their path:
Thank you @brandonpayton that workaround works great! 🎉
@samueljseay, that's great to hear!
I'm going to leave this issue open for now because the "Memory access out of bounds" error in non-JSPI builds might indicate a need for better asyncify-related cleanup. It's worth looking into IMO.
Logged a bug for running static files as PHP: #1365
@brandonpayton It would also be great if PHP.wasm wouldn't crash in this scenario, but fail gracefully. Can that efree() call be avoided if it's deemed to panic?
@brandonpayton It would also be great if PHP.wasm wouldn't crash in this scenario, but fail gracefully. Can that efree() call be avoided if it's deemed to panic?
@adamziel I spent some time looking this evening for ways to handle this and so far don't have anything to suggest.
Emscripten provides an onAbort callback that we already use. It notifies of an ongoing abort but doesn't allow stopping the program from terminating. Perhaps we could terminate in onAbort to do so on our terms if that helps.
What does "fail gracefully" mean to you?
I'm going to leave this issue open for now because the "Memory access out of bounds" error in non-JSPI builds might indicate a need for better asyncify-related cleanup. It's worth looking into IMO.
Until we fix #1365, this Playground zip is an easy way to reproduce the memory out of bounds errors in the Asyncify builds:
playground-with-woo-plugin-dir-named-as-php-file.zip
Hopefully, there is something we can learn here to make Asyncify more stable before moving to JSPI.
What does "fail gracefully" mean to you?
I meant wrapping up the request without crashing so that the runtime can remain functional and handle more requests. That could mean not calling efree() at all if we know it would fail.
Also, let's update rotatePHPRuntime to also rotate the runtime on crash – this way users would be able to continue using Playground. after reporting the error.
I meant wrapping up the request without crashing so that the runtime can remain functional and handle more requests. That could mean not calling efree() at all if we know it would fail.
I considered this and investigated a bit a couple of weeks ago but didn't identify any promising avenues for failing gracefully like that.
Also, let's update rotatePHPRuntime to also rotate the runtime on crash – this way users would be able to continue using Playground. after reporting the error.
That's a great idea. I created #1453 to track that.
I haven't had time to do more testing with Asyncify and this crash, so I'm going to close this and let that that go for now.