WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 5 months ago

#46089 new defect (bug)

Memory exhaustion when setting script translations on `wp-i18n`

Reported by: aduth Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch needs-refresh
Focuses: Cc:
PR Number:

Description

A memory exhaustion occurs given the following snippet of code:

<?php
add_action( 'admin_enqueue_scripts', function() {
        wp_set_script_translations( 'wp-i18n', '' );
} );
<b>Fatal error</b>:  Allowed memory size of 268435456 bytes exhausted (tried to allocate 262144 bytes) in <b>/srv/www/editor/htdocs/wp-includes/class.wp-dependencies.php</b> on line <b>324</b><br />

Memory exhaustion occurs here:

https://github.com/WordPress/WordPress/blob/4e3e9ca9cf893190609728848caf224034fd1ef0/wp-includes/class.wp-dependencies.php#L338

I think it stems from this line:

https://github.com/WordPress/WordPress/blob/4e3e9ca9cf893190609728848caf224034fd1ef0/wp-includes/class.wp-scripts.php#L517

Where wp-i18n is adding itself as a dependency.

Could probably do for a simple check to ensure that the incoming handle is not wp-i18n.

Attachments (2)

set-translations-i18n.diff (514 bytes) - added by aduth 9 months ago.
46089.2.patch (667 bytes) - added by aduth 6 months ago.

Download all attachments as: .zip

Change History (14)

#1 @aduth
9 months ago

  • Keywords has-patch added

#2 @aduth
9 months ago

It's a bit more complex unfortunately, since even with the patch, the same issue would occur when calling wp_set_script_translations with wp-polyfill, since wp-polyfill is itself a dependency of wp-i18n.

https://github.com/WordPress/wordpress-develop/blob/ad0a1a2f8280bb421d36a0f1080080e7c0f7c6d8/src/wp-includes/script-loader.php#L425

Handling this then becomes even more clumsy:

<?php

if ( 'wp-i18n' !== $handle && ! in_array( 'wp-i18n', $obj->deps, true ) &&
                ( ! isset( $this->registered['wp-i18n'] ) || ! in_array( $handle, $this->registered['wp-i18n']->deps ) ) ) {
        $obj->deps[] = 'wp-i18n';
}

Since context was omitted, the issue becomes most apparent from the Gutenberg plugin in trying to re-register all of the packaged scripts. A workaround will be implemented there in the meantime.

https://github.com/WordPress/gutenberg/pull/12559

#3 @swissspidy
9 months ago

  • Milestone changed from Awaiting Review to 5.1.1

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


8 months ago

#5 @lukecarbis
8 months ago

  • Milestone changed from 5.1.1 to 5.2

#6 @desrosj
6 months ago

  • Keywords needs-refresh added

@aduth are you able to refresh the patch based on everything learned in GB-1559?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 months ago

#8 @JeffPaul
6 months ago

  • Milestone changed from 5.2 to Future Release

Per input from @aduth in today's bug scrub and with minimal movement for this in the 5.2 release cycle, we're punting this to Future Release.

@aduth
6 months ago

#9 @aduth
6 months ago

The last patch aligns to what was implemented with GB-1599. There's still room for improvement:

  • The condition with 'wp-polyfill' ought to really be testing any of the dependencies of 'wp-i18n'
  • Generally speaking, I'm thinking this would be better served with an improvement to the lower-level resolution of circular dependencies of scripts, so as to avoid the possibility for this to occur for _any_ script enqueue, particularly since the error is quite cryptic (@iseulde had encountered a similar problem for a different script).

To clarify, there's not a huge urgency to fix this, as best I can tell. But the snippet from the original comment is definitely not something one would expect to cause a memory exhaustion.

This ticket was mentioned in Slack in #core-editor by aduth. View the logs.


5 months ago

#11 @VadimNicolai
5 months ago

I understand that this is not a huge urgency.
But for me, as a GB contributor, it is a real pain.
Can we have a slightly higher priority on this?

#12 @aduth
5 months ago

@VadimNicolai , as mentioned in Slack, this should not be an issue for out-of-the-box use of either WordPress or Gutenberg. It requires specific code to be run for the memory exhaustion to occur. If you're encountering it, it's likely there is some combination of plugins or local changes which is prompting the error.

The purpose of the ticket is more in providing safeguards in core to prevent this from ever occurring. It could also be expanded to the broader concern that the script loader allows for circular dependencies to be resolved recursively (vs. disallowing registration of these dependencies or bailing out upon detection of the circular dependency).

Note: See TracTickets for help on using tickets.