503230 - Dragging a live bookmark folder within another folder causes all items to be duplicated

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Will Roberts [:bws42]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090616 Minefield/3.6a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090616 Minefield/3.6a1pre

See STR.

Reproducible: Always

Steps to Reproduce:
1. Create a folder (folder1) on the bookmark toolbar.
2. Inside folder1 create another folder (folder2).
3. Drag a live bookmark into folder1 above folder2.
4. Drag the live bookmark within folder1 to below folder2.
5. All the items within the live bookmark are now listed twice.



Minefield nightly from 20090615 works, hg revision: eac99a38d8d9
Minefield nightly from 20090616 broken, hg revision: ca8799e74642

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eac99a38d8d9&tochange=ca8799e74642

Possible regression from bug 324430 ?

Comment 1

[Nochum Sossonko [:Natch]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090708 Minefield/3.6a1pre
Confirmed.

Comment 2

[Marco Bonardo [:mak]]

i can't see a clear culprit in the above patch, but i cannot also see another patch that could have caused this.
btw, after the duplication if you drag the livemark out of the folder it will come back to show the right number of entries, so this is a view bug, the bookmarks are not really duplicated. Something in toolbar.xml must be avoiding the popup cleanup

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Perhaps related to one of the focus bugs that landed in that range?

Comment 4

[Neil Deakin]

Don't see what it would have to do with focus.

Comment 5

[Dietrich Ayala (:dietrich)]

Yeah, nothing obvious in that range. Marco, can you try a backout first? If it doesn't resolve, then we'll have Neil backout next.

Comment 6

[Dietrich Ayala (:dietrich)]

assign over to Neil if it's not your regression.

Comment 7

[Marco Bonardo [:mak]]

i can't backout, we rely on that stuff, and without it we would perform really really ugly.
I think this is our fault though, needs to be debugged, but apart being a regression, i'm not sure why a view bug that happens only when dragging a livemark around and is temporary is blocking.

Comment 8

[Marco Bonardo [:mak]]

uh, instead looks like has something to do with some other change, my change was just touching a result rootNode, and we are not doing anything similar here.

What i see (Still have to further check) is that when you move the folder the view receives an itemMoved notification, that rebuilds the popup, but the popup flickers (close/open) and i see an invalidateContainer notification.

so something must be changed about popups (but i've excluded Bug 415791). still doesn't look like we are doing the right thing.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.0

invalidateContainer could be re-entrant in some case, this is already handled in menu view, but not in toolbar view.
if we enter invalidateContainer with a closed resultNode it will open it causing another call to invalidateContainer that will fill popup's children. But then the first invalidateContainer would move on and fill children again.

I don't bother writing a test for this because: would me complex involving many events, could most likely cause randomness due to the complexity, it would be too much implementation bind, won't add much value ad a general view test.

Comment 10

[Marco Bonardo [:mak]]

this bug is also fixed in patch for bug 498130. so we can take either one.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 399842 [details] [diff] [review]
patch v1.0

Fixed by my work in bug 498130, I think.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get a verification of comment 11, please?

Comment 13

[Marco Bonardo [:mak]]

i already checked that in comment 10 while testing Mano's patch, now we only need to know if we are going to approve 498130 for 1.9.2, otherwise we will need the previous patch for 1.9.2

Comment 14

[Marco Bonardo [:mak]]

confirmed fixed in
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090927 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Comment 15

[Marco Bonardo [:mak]]

no more fixed due to backout

Comment 16

[Asaf Romano (gone)]

Comment on attachment 399842 [details] [diff] [review]
patch v1.0

go ahead and land it.

Comment 17

[Marco Bonardo [:mak]]

relanded

Comment 18

[Marco Bonardo [:mak]]

fixed on 1.9.2 by landing of bug 498130

Comment 19

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv