Widget Customizer: Persistent object cache/Transients causes certain previewed widgets to leak outside of customizer

[westonruter]

With a persistent/external object cache plugin present (e.g. Memcached Object Cache), widgets which output buffer and cache their frontend output (the their widget() method)s will leak outside of the customizer preview onto the site frontend. Two core widgets exhibit this behavior: Recent Posts and Recent Comments.

1. Add a Recent Posts widget to a widget area in the customizer and then save.
2. Go to the frontend to see the widget as configured
3. Go into the customizer and change the title of the Recent Posts widget and preview the change
4. Leave the customizer and see that the changed widget title persists.

[westonruter]

Skip widget() caching in customizer for Recent Posts and Recent Comments widgets. See also ​https://github.com/x-team/wordpress-develop/pull/7

[westonruter]

Perhaps there should be a couple helper abstractions added to WP_Widget to encourage proper caching and to reduce redundancy. The WP_Widget constructor could accept the cache key as one of the arguments, and if it is provided, then before widget() is even called it could short-circuit with the cached output if it was supplied. Otherwise, it would start output buffering and then after widget() returns it could store the cached output and flush so that the next time the widget is to be rendered, the cached output could be served. When calling update() the supplied object cache key could be automatically flushed. All of this caching could be prevented if being run inside of the customizer.

[nacin]

This looks like something we'll need to discuss at the weekly developer meeting on April 2. Don't let this slow down any ideas or development here, though.

Is there a way for us to add a previewing() method to WP_Widget? That might be much cleaner and more portable.

[westonruter]

As discussed on IRC, we can invoke wp_suspend_cache_addition() before rendering each widget in the preview to prevent the cache from getting polluted. Also, wp_suspend_cache_addition() should also suspend setting transients.

[ocean90]

wp_suspend_cache_addition()

[nacin]

I noticed an issue here. wp_suspend_cache_addition() is designed to prevent cache *adds*, as in put something into cache that isn't there yet — but not cache *sets*, as in update existing stale cache. Suspending sets could be a problem as then caches would get out of sync with the raw data; that's not the case with suspending adds.

Transients only have a "set" method. They don't have an "add" version. We could block sets, but that would mean it is possible that a stale transient is not somehow updated. While it's probably better *in this case* to suspend transient sets, always suspending transient sets during the suspension of cache adds is likely not the blanket assumption we'd want to make.

I don't entirely know what to do here. Asking others to weigh in.

[David_Rothstein]

Isn't this bug actually caused by the Recent Posts and Recent Comments widgets using a poorly-chosen (and non-unique) cache key?

For example, you can reproduce a similar issue without using the Widget Customizer or a persistent cache plugin at all. Just run these two lines of code:

echo the_widget( 'WP_Widget_Recent_Posts', array( 'title' => 'First title' ), array() );
echo the_widget( 'WP_Widget_Recent_Posts', array( 'title' => 'Second title' ), array() );

The second one will be displayed with a title of "First title", not "Second title".

I wrote a patch that fixes this by using a unique hash for the cache key (and will attach it now). I tested it with the Memcached Object Cache plugin and the Widget Customizer and it seems to fix the bug. This is my first Wordpress patch, so go easy on the review :)

Also, based on the above it shouldn't be necessary to fix anything with setting transients here (since that's only a problem if there's code that sets a transient with a name that isn't unique to the data being cached).

[David_Rothstein]

Use a unique cache key for the Recent Posts and Recent Comments widgets, to prevent cache leaking bugs.

[David_Rothstein]

I was also thinking that it may be a good idea to combine this approach with the addition of the wp_suspend_cache_addition() call. This is for two reasons:

1. It looks like a good use of wp_suspend_cache_addition() anyway, due to the documented purpose of that function. (Adding a bunch of temporary previews to the cache is kind of a waste.)
2. If there are plugins out there that cache their output in the same way the WordPress core plugins did, they would still experience this bug until they fixed their code. Using wp_suspend_cache_addition() is a good backup for that.

However, for 2, I looked at the code in ​http://wordpress.org/plugins/memcached and it doesn't currently check wp_suspend_cache_addition() at all before putting things in the cache... so without updating that plugin that approach doesn't look like it will actually do anything to fix the original bug?

[westonruter]

Add WP_Widget::is_preview() for widgets to use to prevent caching; use in Recent Posts and Recent Comments widgets; also attempt to prevent caching with wp_suspend_cache_addition()

[westonruter]

In 27538.4.patch, I added WP_Widget::is_preview() which widgets should be using to determine whether they should cache or not, and this is now utilized on Recent Posts and Recent Comments. Like ocean90, I also tried adding wp_suspend_cache_addition() but in practice this will have no effect because the cache will have been populated already if the user had visited the site before, and wp_suspend_cache_addition() only blocks cache additions not cache updates, and it doesn't even block additions unless the object cache honors it, and neither Memcached Object Cache nor APC Object Cache do, so it really adds no practical benefit if it only blocks cache additions in the core non-persistent object cache.

[David_Rothstein]

In 27538.4.patch, the removal of the global $comments, $comment line is problematic (among other things, it causes all comment authors in the Recent Comments widget to be printed as "Anonymous").

Also, what's the rationale for this approach over something like 27538.3.patch? 27538.3.patch is simpler code for widgets to deal with, and by addressing the issue with the bad cache key directly, it also prevents other cache leaking bugs from occurring in the future (see for example the code snippet I pasted earlier in this ticket).

[westonruter]

Restore accidental removal of global $comments, $comment

[ocean90]

In 27966:

WP_Widget: Introduce is_preview() method.

With the Widget Customizer it's possible that previewed widgets can leak data outside of Customizer, when the widget uses the cache API.
The Customizer calls the regular update callback which should already refresh the cache. Since cache additions aren't blocked yet the cache can be filled with preview data.
To prevent this issue WP_Widget::is_preview() will return true, when $wp_customize->is_preview() returns true. If is_preview() is true, cache additions are suspended via wp_suspend_cache_addition(). Make sure your object cache drop-in has implemented wp_suspend_cache_addition().

is_preview() can/should also be used inside WP_Widget::widget(), see WP_Widget_Recent_Posts or WP_Widget_Recent_Comments for examples.

For more info see IRC logs: ​http://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2014-04-02&sort=asc#m824279

props westonruter.
fixes #27538.

[Denis-de-Bernardy]

Re-opening, because I think r27966 is can of worms that borders on crazy.

This should be fixed in the wp_cache API directly: you cannot realistically rely on WP widget authors who might try to use the cache in a way or another (be it using wp_cache_get() or a transient).

Methinks the cache API should get updated, with methods such as wp_cache_enable() and wp_cache_disable() introduced and getting called as appropriate. Perhaps use wp_switch_to_blog(-1) or something to that order if they're around, in an effort to keep some level of backwards compatibility. Or maybe unregister all buckets and make them temporarily non-persistent. Anything.

The point here is that existing widgets will not get updated any time soon to make use of WP_Widget#is_preview(). In contrast, object cache plugins will get updated and upgraded very quickly if using the theme or widget customizer yields fatal errors related to non-existing cache methods (which are no big deal here, really, since only admins would ever see them).

[nacin]

Replying to Denis-de-Bernardy:

Re-opening, because I think r27966 is can of worms that borders on crazy.

Read ​http://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2014-04-02&sort=asc#m824279. It goes on for an hour. Feel free to open a new ticket but I think we've done all we can for 3.9.

[Denis-de-Bernardy]

Hehe. It's quite refreshing to see WP core devs openly talking about how they think their own code base and APIs suck. (And agreed, the caching and widgets APIs do suck, but imo they're nowhere near as sucky as taxonomies, roles managed using meta, wp query or the biggest of all monstrosities aka the loop.)

For the transients, we could force the timeout to 1 second to avoid poisoning the cache. And frankly, the IRC log leaves me unconvinced. I understand the rational, mind you, but I still think a fatal error due to non-existing cache functions when previewing a theme is preferrable to the risk of seeing widgets poisoning a cache for many years to come.