Block moves to "Inactive widgets" after saving

[htmgarcia]

Description

After adding a block to widgets Footer sidebar and saving changes, the widget moves to Inactive widgets area.
This happens randomly. Can be tricky to reproduce.

Step-by-step reproduction instructions

• Be sure to use Twenty Twenty-One theme
• Go to Appearance > Widgets
• Add a block to Footer sidebar. In my last test "Gallery" block.
• Save changes
• Refresh

Expected behaviour

The widget should remain in Footer sidebar.

Actual behaviour

The widget is moved to Inactive widgets area.

Screenshots or screen recording (optional)

WordPress information

• WordPress version: 5.8-RC2
• Gutenberg version: 11.0.0
• Are all plugins except Gutenberg deactivated? Yes
• Are you using a default theme (e.g. Twenty Twenty-One)? Yes

Device information

• Device: Desktop
• Operating system: MacOS Big Sur 11.4
• Browser: Firefox 89.0.2

[adamziel]

I managed to reproduce it – it really does seem to occur at random! So far the most reliable reproduction plan for me is this:

• Open the widgets editor
• Delete everything from the "footer" sidebar and hit save
• Refresh the page
• Add a new gallery block and hit save
• Refresh the page

[draganescu]

It is not easy to repro for me. Can it be a browser thing? I am using Edge and could not get this to happen, with existing or new galleries.

[adamziel]

@draganescu unfortunately this doesn't seem to be a client issue. I debugged the HTTP requests triggered when you save the widgets and it goes like this:

• POST /batch/v1 starts
• GET /wp/v2/widgets starts
• POST /batch/v1 finishes & triggers POST /wp/v2/sidebars/sidebar-1 and GET /wp/v2/sidebars

What I noticed is that, when it breaks, both POST requests return the response you would expect, e.g.

(notice the "sidebar: "" part)

What I find interesting though, is that both GET requests return the "broken state" response:

This leaves us with three options:

1. One of the POST requests stored one thing (the broken state) and returned another thing (the correct state)
2. Something alters sidebars_widgets between the POST and GET requests
3. One of the GET requests alters the state and moves the block widget back to wp_inactive_widgets

I am currently exploring option 3 as both GET endpoints call retrieve_widgets which may or may not call wp_set_sidebars_widgets in turn. My hypothesis is that because these GETs and POSTs run concurrently, things sometimes line up in just the right order for retrieve_widgets to operate on a partially saved state.

[adamziel]

It may be locking all over again. There are two possible GETs requests /wp/v2/widgets:

• index.php?rest_route=/wp/v2/widgets&context=edit&per_page=100&_embed=about..
• index.php?rest_route=%2Fwp%2Fv2%2Fwidgets%2F&context=edit& locale=user

The former only starts when the POST /batch/v1 finishes, this is good and I don't anticipate any problems there.

The latter starts immediately after POST /batch/v1 starts, but it is only ever sent if the page was just freshly reloaded, a new widget was added, and we are performing our very first save. This neatly fits the test plan I proposed above and the timing issues hypothesis.

[adamziel]

This is getting weird! I reproduced it again, but this time I also added two legacy widgets. These widgets were preserved, but the gallery widget block was moved to inactive widgets:

I am less convinced now that the /wp/v2/widgets plays any role here.

[adamziel]

For the record, the exact opposite happened too (legacy widget was moved over to the wp_inactive_widgets while the gallery block stayed in the sidebar):

It seems like it only moves either zero widgets or one widget, but never two, three etc.

[adamziel]

I still did not manage to capture an active breakpoint exactly when this issue happens organically, but I found that adding a retrieve_widgets call after the following line enables me to reproduce this problem consistently:

https://github.com/WordPress/wordpress-develop/blob/efb1b5946d22453c1ffe64920f5c501b341884fa/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php#L209

I wonder if this could be a batch processing issue where the next batch request calls retrieve_widgets and overrides something the previous request did.

[adamziel]

Reproducing this by hand is tedious and sometimes takes a long, long time. I created a quick&dirty script that sends the same sequence of requests as the widgets editor until it generates the broken state. For me, it happens on my first attempt sometimes, and some other times it needs dozens of attempts. I am not sure what to make of it yet.

https://gist.github.com/adamziel/a45b5906700425aab0336a6f1b3ce175

[adamziel]

I should also mention this is reproducible on WP trunk both with and without the gutenberg plugin.

[adamziel]

I finally managed to figure this one out, it was an interesting one, that's for sure :-)

tl;dr

The problem is caused by:

• Not properly refreshing global variables between handling batch API calls (cc @TimothyBJacobs, nothing directly actionable but I thought you might be interested)
• Mutating things in GET handlers (via retrieve_widgets)
• An inconsistent backport from gutenberg to WP core
• An unfortunate timing of things that fixes the problem most of the time. We only observe those rare instances when the timing restores the corrupted option after a subsequent POST request fixed it.

full problem description:

1. Clicking "update" in the widgets editor triggers a batch POST request with a series of create, update, and delete "sub-requests" representing all the changes made by the user. Let's use the same batch as in the gist above, namely: 3x create and 3x delete sub-requests.
2. Each batched WP_REST_Widgets_Controller::create_item() performs some checks and eventually creates the widget and assigns it to a sidebar using wp_set_sidebars_widgets().
3. wp_set_sidebars_widgets() only interacts with global $_wp_sidebars_widgets, but not with global $sidebars_widgets. This is the key part here.
4. Once all the batched WP_REST_Widgets_Controller::create_item() calls are done with, it's time to delete three stale widgets using WP_REST_Widgets_Controller::delete_item().
5. If we look at the WordPress trunk version of that method, the first thing it does, is calling retrieve_widgets(). Interestingly, this isn't the case in the Gutenberg plugin version – did something go wrong with backporting? cc @noisysocks @youknowriad @draganescu
6. Anyway, retrieve_widgets() runs some logic to move "hidden/lost" widgets to wp_inactive_widgets sidebar. It does so based on the global $sidebars_widgets that wasn't touched earlier by WP_REST_Widgets_Controller::create_item() – only global $_wp_sidebars_widgets has been changed. So, as far as retrieve_widgets() is concerned, the last created widget isn't assigned to any sidebar and so it is moved to the wp_inactive_widgets sidebar.

Let's take a pause here to find out why does it only affect the last created widget and not all the widgets in the batch. The answer is: WP_REST_Widgets_Controller::create_item calls wp_find_widgets_sidebar() which calls wp_get_sidebars_widgets() which updates the global $sidebar_widgets variable. The only problem is that it does so before it assigns a newly created widget to a sidebar. So now we have a global variable that has all the right widget-to-sidebar assignments, except for the very last one that was just created.

Now, so far we're in a fully deterministic world. Request in, bug out. Why is this problem so hard to reproduce then? I'll answer that with a diagram:

1. The widgets editor triggers the first two orange requests first. Once the POST request is finished it triggers another requests pair (green).
2. The first batch request always results in a broken state, but then there a green POST /wp/v2/sidebars request updates the widgets-to-sidebars assignment to a valid one. In parallel with that POST request, it issues a green GET /wp/v2/widgets request.
3. At the time the green requests are issued, we are in a broken state. The POST always starts from a broken state and results in a valid state. The GET starts from a broken state most of the time (unless the POST was handled first), and, depending on the timing, it results in either a valid or a broken state.

I didn't spend any time on cracking the exact sequence of events in that last point, but something mutable is going on in the GET handler so I assume that retrieve_widgets() is at fault here as well. What we know for sure is that things rarely line up in just the right way to trigger the problem, hence the difficult reproduction process.

Anyway, if I remove POST /wp/v2/sidebars request, I always end up in a broken state. If I remove the last GET request, I always end up in a valid state.

[adamziel]

As for a fix – I think each request handler should update all the relevant global variables. With that in mind, the fix here would be as simple as refreshing global $sidebars_widgets in wp_set_sidebars_widgets as well as $_wp_sidebars_widgets. This could be done as simply as calling wp_get_sidebars_widgets(), although it's unintuitive and I don't like that too much. Perhaps we could refactor another function like wp_refresh_widgets_globals() and then use in in both wp_get_sidebars_widgets() and wp_set_sidebars_widgets().

In addition to that, retrieve_widgets() could just call wp_get_sidebars_widgets() instead of using a global variable. This alone would fix the problem here. I'll spin some PRs soon and let's discuss the code directly there.

As a side note, I would absolutely love to get rid of any mutability from retrieve_widgets. It is confusing and causes hard to fix issues such as this one. Unfortunately it's been in the core since 2.8.0 so there's likely plenty of code depending on it. Might we at least deprecate it? I'll be happy to create a new issues on core Trac.

[adamziel]

Let me also surface this from WordPress.org slack:

@adamziel
Does anyone know why we have both $_wp_sidebars_widgets AND $sidebars_widgets global variables? (edited)

@hellofromtonya
Great question @zieladam!
Core uses $_wp_sidebars_widgets private global as a cache of the sidebars_widgets option to initialize $sidebars_widgets global when not in the admin. (Note: The cached version can be modified in memory to switch from single to multi widget.)
$sidebars_widgets is the in memory working state of the widgets.

@adamziel
thank you @hellofromtonya! do you think we could refactor that to have just one? I just finished debugging #33335 and a big part of it was that, after updating things, we refreshed only $_wp_sidebars_widgets but not $sidebars_widgets. I will patch it up and it will just work, but I think we’re in a non-future-proof situation and may introduce similar errors again (edited)

[adamziel]

Let's continue the discussion in in https://core.trac.wordpress.org/ticket/53657

[adamziel]

Actually I'm re-opening this one. We merged a fix for WP core but we may still need to patch the Gutenberg plugin. I initially assumed we don't because there are no retrieve_widgets calls in sight in handlers we care about, but it's still something we need to check.

@noisysocks do you recall why the retrieve_widgets calls were added to core but not to the plugin?

[noisysocks]

@adamziel: Just a mistake on my part. I think that the plugin PHP and Core PHP has diverged quite a bit, unfortunately. I wonder if we could make the plugin require WordPress 5.8 sooner.

[adamziel]

I propose we rename retrieve_widgets to recover_lost_widgets: https://core.trac.wordpress.org/ticket/53811

[adamziel]

I poked around and it doesn't seem like the Gutenberg plugin is affected by the problem. The call to retrieve_widgets was never added to update and delete endpoints in the WordPress/gutenberg repo. We're good to close this specific issue.

The logic of storing and reading widgets is still likely to cause more problems like this one so I created an epic in Trac to sort it out.