Widget-Visibility: Out of Memory leads to 500 Error on Several Simple Sites #56526
Comments
donalirl
added
[Type] Bug
User Report
|
Another; 4322496-zd-woothemes (Lovecraft) |
|
Ticket: 4322096-zen Switching to another theme then back didn't work (still error). |
|
Another report: 31845088-hc Site was using the theme Blask, tried switching to couple of other themes, did not work. Follow up: 4322711-zen |
|
4321943-zen 4317107-zen Switching to a block based theme (worked) but switching back to previous theme (ie. Arcane, Revelar) didn't work. |
|
User report: 31766471-hc Changing the theme and switching back did not work. Helped them switch to a similar theme they selected - Twenty Eleven to avoid loss of business. Follow-up: 4322832-zen |
|
I've reverted a change we had made a few hours ago to try to solve this. Internal references:
Potentially related to Automattic/jetpack#20731 cc @mreishus |
|
@jeherve all the sites reported above are working now. Although about half of them switched themes, the ones that didn't are loading normally. |
mreishus
changed the title
|
It's definitely caused by Automattic/jetpack#20731, and we are seeing two failure states: Out of Memory and render->null(). I've turned this issue into Out Of Memory, and created #56546 for render->null(). First impressions of the Out of Memory error is that some of the errors occur on pages like sitemap.xml and ads.txt, which is not expected if the error is related to traversing large sets of blocks and some sort of interesting signal. (See: 2a4c4-pb ) |
|
With the revert clearing these errors, can this ticket be closed? |
Planning to use this to track the debugging of the issue; it contains relevant information and the widget visibility PR itself is getting hard to navigate. |
|
This is caused by an infinite loop. Here is a stack trace: The difference is that with legacy widgets, the
Block widgets add a new possibility:
The problem here is that running |
|
Is there a way to remove the filter ahead of rendering |
This (1) works in my testing but I don't feel great about: diff --git a/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php b/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
index 669fedf56..cf5efdf52 100644
--- a/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
+++ b/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
@@ -789,11 +789,15 @@ class Jetpack_Widget_Conditions {
}
$output = '';
+ remove_filter( 'widget_display_callback', array( __CLASS__, 'filter_widget' ) );
+ remove_filter( 'sidebars_widgets', array( __CLASS__, 'sidebars_widgets' ) );
foreach ( $blocks as $block ) {
if ( ! empty( $block ) ) {
$output .= render_block( $block );
}
}
+ add_filter( 'widget_display_callback', array( __CLASS__, 'filter_widget' ) );
+ add_filter( 'sidebars_widgets', array( __CLASS__, 'sidebars_widgets' ) );
$instance['content'] = $output;
return $instance;I think it's too dependent upon the filters that we set up around this function. That is, if something else starts hooking onto this function and using it a different way, the same problem could crop up again. This (2) also works in my testing and I feel a bit better about: diff --git a/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php b/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
index 669fedf56..a90dafde9 100644
--- a/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
+++ b/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
@@ -20,6 +20,7 @@ use Automattic\Jetpack\Assets;
*/
class Jetpack_Widget_Conditions {
static $passed_template_redirect = false;
+ static $is_filter_widget_rendering = false;
public static function init() {
global $pagenow;
@@ -759,6 +760,10 @@ class Jetpack_Widget_Conditions {
* @return array Settings to display or bool false to hide.
*/
public static function filter_widget( $instance ) {
+ if ( self::$is_filter_widget_rendering ) {
+ return $instance;
+ }
+
// Don't filter widgets from the REST API when it's called via the widgets admin page - otherwise they could get
// filtered out and become impossible to edit.
if ( strpos( wp_get_raw_referer(), '/wp-admin/widgets.php' ) && false !== strpos( $_SERVER['REQUEST_URI'], '/wp-json/' ) ) {
@@ -789,11 +794,13 @@ class Jetpack_Widget_Conditions {
}
$output = '';
+ self::$is_filter_widget_rendering = true;
foreach ( $blocks as $block ) {
if ( ! empty( $block ) ) {
$output .= render_block( $block );
}
}
+ self::$is_filter_widget_rendering = false;
$instance['content'] = $output;
return $instance;Another solution is to (3) rework the function so it doesn't render at all. I haven't tried this, but I think it would mean limiting the functionality. Instead of being allowed to set visibility rules on any block, anywhere in widgets (which admittedly, is pretty cool), it would only check the top level block for each widget, and either return them in their entirety or |
Yes. |
|
I have a fix for this issue (render inside the visibility feature inadvertently trigger an infinite loop). However, I don't have a fix for #56546 (render inside visibility feature tries to render null and crashes). I'm also not able to reproduce that issue. Additionally, there could be more issues out there that we don't know about. I propose scaling back the functionality offered by the feature. Instead of letting the user set visibility rules on any block inside a block widget, only allow the rules to be set on the outermost block of each block widget. In my opinion, this should cover the vast majority of user cases (I want this widget to display or not), while significantly reducing the complexity of the widget: The filter function will no longer have to change the content of blocks, and no longer have to call the render() function. Since #56546 is caused by a call to render(), if I can stop the filter from ever needing to render at all, then I can fix the issue. The following patch works in my testing. It does need additional changes on the front-end to only display the visibility options to the user on the outermost block of each widget. diff --git a/projects/plugins/jetpack/modules/widget-visibility/editor/index.jsx b/projects/plugins/jetpack/modules/widget-visibility/editor/index.jsx
index bf8fa4f692..20c86faff7 100644
--- a/projects/plugins/jetpack/modules/widget-visibility/editor/index.jsx
+++ b/projects/plugins/jetpack/modules/widget-visibility/editor/index.jsx
@@ -6,6 +6,7 @@ import { BaseControl, Button, SelectControl, ToggleControl } from '@wordpress/co
import { __, _x } from '@wordpress/i18n';
import { InspectorAdvancedControls } from '@wordpress/block-editor'; // eslint-disable-line import/no-unresolved
import { createHigherOrderComponent } from '@wordpress/compose';
+import { useSelect } from '@wordpress/data';
/* global widget_conditions_data, wpcom */
/* eslint-disable react/react-in-jsx-scope */
@@ -223,10 +224,20 @@ const maybeAddDefaultConditions = conditions => ( {
const visibilityAdvancedControls = createHigherOrderComponent(
BlockEdit => props => {
- const { attributes, setAttributes, isSelected } = props;
+ const { clientId, attributes, setAttributes, isSelected } = props;
const conditions = useMemo( () => attributes.conditions || {}, [ attributes ] );
const rules = useMemo( () => conditions.rules || [], [ conditions ] );
+ const isParentWidgetArea = useSelect(
+ select => {
+ const { getBlockParents, getBlock } = select( 'core/block-editor' );
+ const parents = getBlockParents( clientId, true );
+ const parentBlock = parents ? getBlock( parents[ 0 ] ) : undefined;
+ return parentBlock && parentBlock.name == 'core/widget-area';
+ },
+ [ clientId ]
+ );
+
const toggleMatchAll = useCallback(
() =>
setAttributes( {
@@ -382,9 +393,22 @@ const visibilityAdvancedControls = createHigherOrderComponent(
return (
<Fragment>
<BlockEdit { ...props } />
- { isSelected && blockHasVisibilitySettings( props.name ) && (
+ { isSelected && isParentWidgetArea && blockHasVisibilitySettings( props.name ) && (
<InspectorAdvancedControls>{ mainRender }</InspectorAdvancedControls>
) }
+ { isSelected && ! isParentWidgetArea && blockHasVisibilitySettings( props.name ) && (
+ <InspectorAdvancedControls>
+ <BaseControl
+ id="widget-vis__wrapper"
+ className="widget-vis__wrapper"
+ label={ __( 'Visibility', 'jetpack' ) }
+ help={ __(
+ 'Please select the top level block to apply widget visibility.',
+ 'jetpack'
+ ) }
+ ></BaseControl>
+ </InspectorAdvancedControls>
+ ) }
</Fragment>
);
},
diff --git a/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php b/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
index 669fedf560..b6b413a2f9 100644
--- a/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
+++ b/projects/plugins/jetpack/modules/widget-visibility/widget-conditions.php
@@ -781,90 +781,20 @@ class Jetpack_Widget_Conditions {
return false;
} elseif ( ! empty( $instance['content'] ) && has_blocks( $instance['content'] ) ) {
// Block-Based widgets: We have gutenberg blocks that could have the 'conditions' attribute
- // Note: These blocks can be nested, and we would have to apply these conditions at any level.
$blocks = parse_blocks( $instance['content'] );
- $blocks = self::recursively_filter_blocks( $blocks )[0];
- if ( empty( $blocks ) ) {
- return false;
+ if ( empty( $blocks[0]['attrs']['conditions']['rules'] ) ) {
+ return $instance;
}
-
- $output = '';
- foreach ( $blocks as $block ) {
- if ( ! empty( $block ) ) {
- $output .= render_block( $block );
- }
+ if ( self::filter_widget_check_conditions( $blocks[0]['attrs']['conditions'] ) ) {
+ return $instance;
}
- $instance['content'] = $output;
-
- return $instance;
+ return false;
}
// No visibility found.
return $instance;
}
- /**
- * Recursively check the visibility conditions in blocks' attr.conditions.rules.
- * Any that fail the check will be removed.
- *
- * @param array $blocks (By reference; will be modified).
- * @return array [$blocks, $indexes_removed]
- */
- public static function recursively_filter_blocks( &$blocks ) {
- $indexes_removed = array();
-
- foreach ( $blocks as $i => $block ) {
- $keep = true;
- if ( ! empty( $block['attrs']['conditions']['rules'] ) ) {
- $keep = self::filter_widget_check_conditions( $block['attrs']['conditions'] );
- }
-
- if ( ! $keep ) {
- unset( $blocks[ $i ] );
- array_push( $indexes_removed, $i );
- } elseif ( ! empty( $block['innerBlocks'] ) && is_array( $block['innerBlocks'] ) ) {
- $result = self::recursively_filter_blocks( $blocks[ $i ]['innerBlocks'] );
- $blocks[ $i ]['innerBlocks'] = $result[0];
-
- // For each block we removed in innerBlocks, we must remove a corresponding 'null' in innerContent
- // if $sub_indexes_removed = [0, 2], we removed the 0th and 2nd innerBlocks, and need to remove
- // the 0th and 2nd nulls in innerContent.
- $sub_indexes_removed = $result[1];
- if ( ! empty( $sub_indexes_removed ) ) {
- $null_locations = array_keys( $blocks[ $i ]['innerContent'], null, true );
- $null_count = count( $null_locations );
-
- // phpcs:disable Squiz.PHP.CommentedOutCode.Found
-
- /*
- Example:
- null_locations = [ 1, 3, 5 ] "Nulls exist at j=1, j=3, j=5".
- null_count = 3. "There are 3 nulls".
- sub_indexes_removed = [ 0, 2 ]. "We want to remove the 0th and 2nd null".
- We need to remove the 0th null (at j = 1) and the 2nd null (at j = 5).
- [ 'abc', null, 'def', null, 'ghi', null ].
- ..........^ j = 1, k = 0 ^.
- .......................^ j = 3, k = 1 ^.
- ......................................^ j = 5, k = 2.
- */
-
- // phpcs:enable Squiz.PHP.CommentedOutCode.Found
-
- $k = $null_count - 1;
- // Work backwards so as we delete items, then the other indexes don't change.
- foreach ( array_reverse( $null_locations ) as $j ) {
- // We are looking at a null, which is the $j index of the array and the $kth null in total (k is also 0 indexed).
- if ( in_array( $k, $sub_indexes_removed, true ) ) {
- array_splice( $blocks[ $i ]['innerContent'], $j, 1 );
- }
- $k--;
- }
- }
- }
- }
- return array( $blocks, $indexes_removed );
- }
-
/**
* Determine whether the widget should be displayed based on conditions set by the user.
*
1
Edit 9/28: Updated with front-end changes. I'm not entirely satisfied with this solution - I thought that specifying rules at any block level was a powerful, conceptually elegant solution - but I'm leaning towards it being a necessary compromise given the complexity of WP filters, existing user setups, and limited resources we have for this project. |
|
@jeherve We're in an odd spot development wise with JP and dotcom being unsynced. The changes are on JP, merged, but manually reverted on dotcom. I've been using |
|
As we discussed in Slack, at this point it may be easier to open a Jetpack PR, and then manually open a diff on WordPress.com Simple to bring everything over later on. We will not be able to rely on Fusion for this one unfortunately. |
|
Created Automattic/jetpack#21222 |
|
We've launched widget visibility, reworked a bit, so this error can no longer occur. Please re-open if you are seeing any other issues like this. |
Quick summary
Several simple sites are producing a 500 error and not loading. All sites are using a classic theme, and they load if they are switched to a newer theme.
See Also
Steps to reproduce
Unable to reproduce so far, but we have received at least 4 reports of this.
#29555158-hc (Lovecraft)
#30342258-hc (Toujours)
#4322338-zen (Colinear)
#4314833-zen (Baskerville 2)
Switching to a newer theme makes the site load. In some cases, switching back to the original classic theme works fine. Other times, the site goes down again.
Memory Errors
I have redacted the post URLs from the above error messages but they can be seen here: p1632446966171200/1632443368.154100-slack-C03TY6J1A
What you expected to happen
The sites to load.
What actually happened
The sites produced a 500 error and did not load.
Simple, Atomic or both?
Simple
Severity
Some (< 50%)
Available workarounds?
Sometimes switching the theme and then changing it back to the classic theme works, but not consistently in all cases.
The text was updated successfully, but these errors were encountered: