display_page_row loops forever

[hailin]

display_page_row() in wp-admin/includes/template.php could loop forever, causing wp-admin/edit-pages.php not able to load at all.

The root cause is that when a few pages are corrupted, such that they form a loop in a disconnected sub-graph. The disconnected sub-graph will be treated as orphans in page_rows since none of its elements points to a top level page.

When one orphan page is passed to display_page_row( $op, 0 ), display_page_row will try to find out its ancestors. It will hit an infinite loop, if the chain of ancestors pages form a loop.

We should try to detect, break the loop, and fix the corrupted pages post_parent.

The loop happens very rarely. But it does happen. We’ve seen it happening about once a week on a large scale WPMU installation. I tried to reproduce it by editing and changing page parents with no avail. Possible causes are cache pollution, and simultaneous operation by two admins, causing race condition in writing to DB.

Nevertheless, it’s better to detect and fix the loops, otherwise, users can not access edit-pages.php.

I further conclude that we only need to handle loop conditions in display_page_row. Given our current algorithm, loops are only possible in disconnected sub-graphs, thus we don’t need to worry about it in page_rows or _page_rows.

[hailin]

patch

[hailin]

tested it with a live loop example, it fixes the pages correctly, and pages are displayed correctly.

[westi]

Ok.

Easy to reproduce the bug!

Create a page hierarchy with:

• Root
• * Level One

Add a Page called Level Two

Open edit on Root and Level Two in two browser tabs
Set Root to be Level Twos child in one tab
Set Level Two to be Level Ones child in other tab.

Boom!

[westi]

I'm not sure this patch is the best fix for the overall issue.

I think we need to approach this from two angles:

• Stopping it happening by validating the parent is valid when the request to set it happens.
• Ensure if it still happens we recover in a meaningful way.

Also it looks like the code patched here should use get_post_ancestors which would have already built the tree for us and we can just count() it.

[hailin]

I agree that we should try to prevent the loop from forming in the first place.
where is the code that handles page parent_id update?

_get_post_ancestors() can not detect loops involving multiple hops.
It only makes sure post ID and parent_id are not the same. So we can not rely on it.
I think the loop breaking logic in the original patch is solid.

[westi]

Replying to hailin:

I agree that we should try to prevent the loop from forming in the first place.
where is the code that handles page parent_id update?

wp-includes/post.php:wp_insert_post()

Probably need a function to validate the passed in post_parent (also check the supplied parent exists I guess)

Replying to hailin:

_get_post_ancestors() can not detect loops involving multiple hops.
It only makes sure post ID and parent_id are not the same. So we can not rely on it.
I think the loop breaking logic in the original patch is solid.

Ok. But the change should go into _get_post_ancestors as we have the same bug there and then we can call that and use count() to get the depth we are at.

[hailin]

Thanks for the hints, Westi.

Here is a patch that fixes the root cause, and prevents the loop from happening in the first place. The calling sequence for editing page parent is:
edit_post() => wp_update_post() => wp_insert_post()

I think it's better to prevent and fix it earlier in the sequence, since wp_insert_post are pretty low level function, which should be agnostic of the logic from upper layer.

I've reproduced the loop issues involving one hop, two hops or three hops. And verified that the patch handles every case successfully, and it prevents the loop from happening in the first place.

As for the second part:
how to break the loop in case it still happens, I'm not convinced that _get_post_ancestors is the best place to be, because _get_post_ancestors is intended as a private function. Let me open another ticket for breaking the loop since it's a different issue.

[hailin]

created #11047 to keep track of loop breaking.

[hailin]

updated patch

[hailin]

the updated patch handles all possible loops (post_parent = self ID), and multi-hop loops. it returns right away when post_parent is the same value as the current one in the db.

[hakre]

Related: #10277

[hakre]

Related: #11424

[nacin]

#13611 closed as a duplicate. Some good information there, also a patch, also a potential importer issue the reporter is looking into.

[nacin]

Closing as a duplicate of #11047 and the tickets it was in turn closed as a duplicate of.

Odd that this ticket and #11047 were the same OP.