WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10092 closed defect (bug) (fixed)

widgets randomly get added in two or more sidebars

Reported by: Denis-de-Bernardy Owned by: azaozz
Milestone: 2.8.1 Priority: normal
Severity: normal Version: 2.8
Component: Widgets Keywords: has-patch tested commit
Focuses: Cc:

Description

we've yet to identify a means to reproduce, but here goes some IRC conversations:

[03:18] <ddebernardy> I haven't seen it occur in over a week or so, but I've no idea of whether it's due to my not testing widgets moving (apart from adding one and testing as I rewrite it)

[03:19] <junsuijin> well i was wondering if it had to do with settings from widgets saved into the db before the most recent 2.8 is all

[03:20] <junsuijin> and it inconsistently updating those ids

[03:21] <ddebernardy> I suspect, personally, that it may have to do with a jquery bug

[03:21] <junsuijin> i can meticulously go back and set up the exact widgets i had before clearing my db i suppose, or just grab the db option from the blog i have set up heh

[03:21] <ddebernardy> like, when you have several sidebars open

[03:21] <ddebernardy> and hover a widget over them all

[03:21] <junsuijin> yeah i was thinking that too

[03:21] <junsuijin> but i've got several open right now

[03:21] <ddebernardy> and erroneously drop the widget in one

[03:21] <junsuijin> i'm wondering maybe if you drag 2 widgets and they don't finish saving

[03:21] <ddebernardy> immediately click on it

[03:21] <ddebernardy> and move it in a separate sidebar

[03:22] <junsuijin> yeah

[03:22] <junsuijin> sort of a similar scenario

[03:22] <junsuijin> basically initiating another ajax request before the other finishes

[03:22] <ddebernardy> that scenario seems likely to end us with the same widget in two sidebars

Attachments (1)

10092.patch (639 bytes) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Denis-de-Bernardy5 years ago

it might be worth adding a check somewhere, if it doesn't exist already, to verify that a widget doesn't exist in a separate sidebar before adding it.

comment:2 follow-up: junsuijin5 years ago

  • Cc junsuijin@… added

Another note is that when the duplication happened it would sometimes overwrite other existing widgets with the duplicate widget, sometimes just add it to a different sidebar (not always one that was open at the time).

comment:3 junsuijin5 years ago

Just figured out a bit more info...
Starting from having no widgets, manually cleaning the db option.
I kept watch of the AJAX POST for widgets-order responses, and the widgets would number as expected, where each widget had widget-12_rig-html-16, widget-12_rig-html-17, etc., where the widget-12 corresponds to the type of widget, unique to each type of widget in the available widgets list.

I press F5, then click the Widgets admin menu link to totally reload the page.
I attempt to put a new rig-html widget into sidebar-1 and then quickly move it to sidebar-2.
I note 3 AJAX POSTs now, all numbering the widgets like widget-21_rig-html-16, widget-22_rig-html-17, where the widget-number corresponds to the order of active widgets from first registered sidebar to last registered sidebar. The widget did still get moved correctly, but this is inconsistent behavior, and could account for the widget overwriting/duping, since the new widget gets added as a widget-12 as expected, but amongst widgets of order determined by their place in the list of active widgets.

Example (note widget-12_rig-html-33):


RESPONSE 1


action
widgets-order
savewidgets
4ba6c52c16
sidebars[after-comments]
sidebars[after-content]
sidebars[after-title]
sidebars[after-welcome-content]
widget-44_pages-2,widget-45_links-2,widget-46_tag_cloud-2
sidebars[backgrounds]
widget-49_rig-html-20
sidebars[before-content]
sidebars[before-welcome-content]
sidebars[features]
widget-40_text-7,widget-41_rig-recent-posts-3,widget-42_rig-recent-comments-3,widget-43_categories-2
sidebars[footer]
widget-47_rig-seo-footer,widget-48_rig-html-21
sidebars[header]
widget-38_rig-seo-header,widget-39_rig-html-22
sidebars[scripts]
sidebars[sidebar-1]
widget-21_rig-html-15,widget-22_rig-seo-sidebar,widget-23_rig-html-23,widget-24_rig-html-24,widget-25_rig-html-28
,widget-26_rig-html-29,widget-27_rig-html-30,widget-28_rig-html-32,widget-29_rig-html-31,widget-30_rig-html-16
,widget-12_rig-html-33
sidebars[sidebar-2]
widget-31_rig-html-17,widget-32_rig-html-25,widget-33_rig-html-19,widget-34_rig-html-26,widget-35_text-6
,widget-36_rig-html-27,widget-37_rig-html-18
sidebars[sidebar-top]
sidebars[wp_inactive_widgets]


RESPONSE 2


action
widgets-order
savewidgets
4ba6c52c16
sidebars[after-comments]
sidebars[after-content]
sidebars[after-title]
sidebars[after-welcome-content]
widget-44_pages-2,widget-45_links-2,widget-46_tag_cloud-2
sidebars[backgrounds]
widget-49_rig-html-20
sidebars[before-content]
sidebars[before-welcome-content]
sidebars[features]
widget-40_text-7,widget-41_rig-recent-posts-3,widget-42_rig-recent-comments-3,widget-43_categories-2
sidebars[footer]
widget-47_rig-seo-footer,widget-48_rig-html-21
sidebars[header]
widget-38_rig-seo-header,widget-39_rig-html-22
sidebars[scripts]
sidebars[sidebar-1]
widget-21_rig-html-15,widget-22_rig-seo-sidebar,widget-23_rig-html-23,widget-24_rig-html-24,widget-25_rig-html-28
,widget-26_rig-html-29,widget-27_rig-html-30,widget-28_rig-html-32,widget-29_rig-html-31,widget-30_r
ig-html-16
sidebars[sidebar-2]
widget-31_rig-html-17,widget-32_rig-html-25,widget-33_rig-html-19,widget-34_rig-html-26,widget-35_text-6
,widget-36_rig-html-27,widget-37_rig-html-18
sidebars[sidebar-top]
sidebars[wp_inactive_widgets]


RESPONSE 3


action
widgets-order
savewidgets
4ba6c52c16
sidebars[after-comments]
sidebars[after-content]
sidebars[after-title]
sidebars[after-welcome-content]
widget-44_pages-2,widget-45_links-2,widget-46_tag_cloud-2
sidebars[backgrounds]
widget-49_rig-html-20
sidebars[before-content]
sidebars[before-welcome-content]
sidebars[features]
widget-40_text-7,widget-41_rig-recent-posts-3,widget-42_rig-recent-comments-3,widget-43_categories-2
sidebars[footer]
widget-47_rig-seo-footer,widget-48_rig-html-21
sidebars[header]
widget-38_rig-seo-header,widget-39_rig-html-22
sidebars[scripts]
sidebars[sidebar-1]
widget-21_rig-html-15,widget-22_rig-seo-sidebar,widget-23_rig-html-23,widget-24_rig-html-24,widget-25_rig-html-28
,widget-26_rig-html-29,widget-27_rig-html-30,widget-28_rig-html-32,widget-29_rig-html-31,widget-30_r
ig-html-16
sidebars[sidebar-2]
widget-31_rig-html-17,widget-12_rig-html-33,widget-32_rig-html-25,widget-33_rig-html-19,widget-34_rig-html-26
,widget-35_text-6,widget-36_rig-html-27,widget-37_rig-html-18
sidebars[sidebar-top]
sidebars[wp_inactive_widgets]

comment:4 in reply to: ↑ 2 Denis-de-Bernardy5 years ago

Replying to junsuijin:

Another note is that when the duplication happened it would sometimes overwrite other existing widgets with the duplicate widget, sometimes just add it to a different sidebar (not always one that was open at the time).

that part is normal. it's the same widget, after all.

comment:5 Denis-de-Bernardy5 years ago

k, so based on your comments, the diagnosis is correct -- we might be missing a check, over in widgets, to double-check for existing widgets with the same ID in sidebars, to work around race conditions

comment:6 Denis-de-Bernardy5 years ago

  • Keywords needs-patch added

comment:7 junsuijin5 years ago

Well you probably have to look at the source of this page to be able to read those responses sensibly, but basically, the admin-ajax.php sets the widgets option every time it does this response. There would be a problem when adding say, the 21st available widget to sidebar-1 in the example, as it is not actually a widget-21.

comment:8 azaozz5 years ago

The widget-##_ part of the id is not used when saving the positions and sidebars, it's a temp id for the widget only. Think I have an idea what's going on there. See if the attached patch fixes it.

azaozz5 years ago

comment:9 Denis-de-Bernardy5 years ago

  • Keywords has-patch added; needs-patch removed

odd... didn't get any email as this ticket was being updated...

@junsuijin -- can you try the patch?

comment:10 junsuijin5 years ago

  • Keywords tested added

Not 100% sure this is the only means to reproduce the bug. It seems that if I load up several instances of any given widget, and then remove an instance with an id in the middle of the many others, then proceed to reload the admin widgets.php page, the id count for newly added instances of the given multi-widget starts from the now-unused id in the middle, which allows for any added instances past the first (or however many unused ids there are in the middle), to overwrite (duplicating themselves) the consecutive instances past the unused id(s), since additional new instance ids continue incrementing from the first added instance.

I patched the 2.8 release's wp-admin/includes/widgets.php with azaozz's patch. After doing so new instances are always given an id past the highest instance id, instead of trying to utilize the unused id(s) in the middle. This effectively eliminates the problem since new instance ids cannot increment over established instance ids.

I made a db backup at the point where the bug can be reproduced in case anyone needs it, but I think the described situation is the cause.

comment:11 Denis-de-Bernardy5 years ago

  • Keywords commit added

Aaaah, now I get it. So, to reproduce, do the following:

add three widgets. Delete the send one. Add yet another widget. Without the patch, we get a dup id, with the patch, we no longer do.

comment:12 Denis-de-Bernardy5 years ago

Delete he second one even.

comment:13 junsuijin5 years ago

Yes Denis; but to clarify slightly, after deleting the second widget instance, add two more instances (adding just one would not pose a problem). Or, add 1 instance and remove it, and add another. Both ways produce the same effect.

The key though, is that after removing that second widget instance, one must physically reload the widgets.php admin page, via the menu link or by pressing enter with the address bar selected (a simple F5 refresh will not do the job). Without 'leaving' the widgets.php page, the instance id count is unaffected (so in a case where a user would log out after deleting the middle widget instance, and then the user returns later, they could experience the problem, if they add more than one instance of that type of widget).

comment:14 azaozz5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [11576]) Do not reuse deleted widget instances IDs, fixes #10092 for 2.8.1

comment:15 azaozz5 years ago

(In [11577]) Do not reuse deleted widget instances IDs, fixes #10092 for trunk

Note: See TracTickets for help on using tickets.