WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 13 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:

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 16 months ago.
46089.2.patch (667 bytes) - added by aduth 13 months ago.

Download all attachments as: .zip

Change History (14)

#1 @aduth
16 months ago

  • Keywords has-patch added

#2 @aduth
16 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
16 months ago

  • Milestone changed from Awaiting Review to 5.1.1

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


15 months ago

#5 @lukecarbis
15 months ago

  • Milestone changed from 5.1.1 to 5.2

#6 @desrosj
14 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.


14 months ago

#8 @JeffPaul
14 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
13 months ago

#9 @aduth
13 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.


13 months ago

#11 @VadimNicolai
13 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
13 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.