WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 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-unit-tests
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 (3)

set-translations-i18n.diff (514 bytes) - added by aduth 2 years ago.
46089.2.patch (667 bytes) - added by aduth 2 years ago.
46089.diff (1.2 KB) - added by noisysocks 4 months ago.

Download all attachments as: .zip

Change History (20)

#1 @aduth
2 years ago

  • Keywords has-patch added

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

  • Milestone changed from Awaiting Review to 5.1.1

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


2 years ago

#5 @lukecarbis
2 years ago

  • Milestone changed from 5.1.1 to 5.2

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


2 years ago

#8 @JeffPaul
2 years 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
2 years ago

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


2 years ago

#11 @VadimNicolai
2 years 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
2 years 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).

This ticket was mentioned in Slack in #core-js by ocean90. View the logs.


6 months ago

#14 @noisysocks
5 months ago

  • Milestone changed from Future Release to 5.7

It would be good for a proper fix here to land in 5.7 so that we don't need to ship a workaround. See GB28279.

#15 in reply to: ↑ description @noisysocks
5 months ago

Replying to aduth:

Memory exhaustion occurs here:

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

I tested this and it looks like now the memory exhaustion error now happens in all_deps and not recurse_deps. This is likely because, since this ticket was opened, recurse_deps was updated to no longer be recursive. See #46469.

Maybe we can update all_deps similarly.

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


5 months ago

@noisysocks
4 months ago

#17 @noisysocks
4 months ago

  • Keywords needs-unit-tests added; needs-refresh removed
  • Milestone changed from 5.7 to Future Release

I did some experimentation here and 46089.diff is a very rough proof of concept for what I described in my comment above. It fixes the infinite recursion but needs tests, testing, and to be prettied up.

It looks as though we need the workaround in GB28279 even if this bug is fixed, though. This is because WordPress will print a script’s translations before it prints the script. This means that wp-i18n’s translations will try to call wp.i18n.setLocaleData() before wp.i18n is defined.

For this reason I am moving this ticket out of 5.7 and will create a new ticket which implements the workaround in GB28279.

Last edited 4 months ago by noisysocks (previous) (diff)
Note: See TracTickets for help on using tickets.