WordPress.org

Make WordPress Core

Opened 12 years ago

Closed 11 years ago

#11018 closed defect (bug) (duplicate)

display_page_row loops forever

Reported by: hailin Owned by: hailin
Milestone: Priority: normal
Severity: normal Version: 2.7
Component: Template Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (2)

11018_page_loop.diff (1.7 KB) - added by hailin 12 years ago.
patch
11018_prevent_loop.diff (2.0 KB) - added by hailin 12 years ago.
updated patch

Download all attachments as: .zip

Change History (21)

@hailin
12 years ago

patch

#1 @hailin
12 years ago

  • Component changed from General to Template

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

#2 @hailin
12 years ago

  • Keywords page_rows added
  • Milestone changed from Unassigned to 2.9
  • Version set to 2.7

#3 @scribu
12 years ago

  • Keywords has-patch added; page_rows removed

#4 @westi
12 years ago

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!

#5 @westi
12 years ago

  • Keywords needs-patch added; has-patch removed

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.

#6 follow-up: @hailin
12 years ago

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.

#7 in reply to: ↑ 6 @westi
12 years ago

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.

#8 @hailin
12 years ago

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.

#9 @hailin
12 years ago

created #11047 to keep track of loop breaking.

@hailin
12 years ago

updated patch

#10 @hailin
12 years ago

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.

#11 @hailin
12 years ago

  • Keywords has-patch added; needs-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

#12 @hailin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#13 @ryan
12 years ago

  • Milestone changed from 2.9 to 3.0

#14 @Denis-de-Bernardy
12 years ago

  • Cc Denis-de-Bernardy added

#15 @hakre
12 years ago

Related: #10277

#16 @hakre
12 years ago

Related: #11424

#17 @ryan
11 years ago

  • Milestone changed from 3.0 to 3.1

#18 @nacin
11 years ago

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

#19 @nacin
11 years ago

  • Milestone Awaiting Triage deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.