-
Notifications
You must be signed in to change notification settings - Fork 237
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
Memory access out of bounds with WooCommerce development plugin #1332
Comments
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:
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: |
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:
When testing with the JSPI branch, there is just a mix of exit-related messages like:
|
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. |
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 I changed /**
* 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: wordpress-playground/packages/playground/blueprints/src/lib/steps/install-asset.ts Lines 72 to 79 in b951c05
|
@adamziel I think this is probably where we are running into trouble: wordpress-playground/packages/php-wasm/universal/src/lib/php-request-handler.ts Lines 595 to 597 in 7d62755
The asset paths are like:
And IIUC Playground will try to run such non-PHP files as PHP because they have wordpress-playground/packages/php-wasm/universal/src/lib/php-request-handler.ts Lines 315 to 321 in 7d62755
|
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 |
@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? |
Until we fix #1365, this Playground zip is an easy way to reproduce the memory out of bounds errors in the Asyncify builds: Hopefully, there is something we can learn here to make Asyncify more stable before moving to JSPI. |
I meant wrapping up the request without crashing so that the runtime can remain functional and handle more requests. That could mean not calling Also, let's update |
I considered this and investigated a bit a couple of weeks ago but didn't identify any promising avenues for failing gracefully like that.
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. |
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:
You can reproduce with this playground link:
https://playground.wordpress.net/#%7B%22landingPage%22:%22/wp-admin/admin.php?page=wc-admin%22,%22preferredVersions%22:%7B%22php%22:%228.0%22,%22wp%22:%22latest%22%7D,%22phpExtensionBundles%22:%5B%22kitchen-sink%22%5D,%22features%22:%7B%22networking%22:true%7D,%22steps%22:%5B%7B%22step%22:%22installPlugin%22,%22pluginZipFile%22:%7B%22resource%22:%22url%22,%22url%22:%22https://playground.wordpress.net/plugin-proxy.php?org=woocommerce&repo=woocommerce&workflow=Build%20Live%20Branch&artifact=plugins-8797272447&pr=46761%22%7D,%22options%22:%7B%22activate%22:true%7D%7D,%7B%22step%22:%22login%22,%22username%22:%22admin%22,%22password%22:%22password%22%7D%5D,%22plugins%22:%5B%5D%7D
Pinging @brandonpayton to take a look as requested by Adam.
Thanks all 🙇
The text was updated successfully, but these errors were encountered: