375677 - Deleting 5 or more bookmarks at once in Bookmark Manager (FF 2.0.0.3) seriously messes up the bookmark handling

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Xavier]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 XPCOMViewer/0.9.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3 XPCOMViewer/0.9.5

If you select 5 or more bookmarks in the bookmark Manager and use the context menu to delete them here is what happens:
- they do not disappear from the bookmark list.
- their context menu becomes truncated to the first 3 opening options
- no other bookmark can be added !

Everything goes back to normal when you relaunch Firefox, the deleted bookmarks are gone.

Selecting 4 or less works all right.

Reproducible: Always

Steps to Reproduce:
1. have at least 5 bookmarks
2. and open the bookmark manager (Organize Bookmark option from Bookmarks Menu)
3. Select 5 bookmarks
4. right click to get contextual menu, then Delete
Actual Results:  
The selected bookmarks do not disappear.
Their context menu is truncated.
They can't be opened.
No other bookmark can be added, either by Drag and Drop in the manager or from the Bookmarks menu.

Expected Results:  
The selected bookmarks are deleted.

Comment 1

[zug_treno]

Maybe related to bug 373800 and/or bug 347103 comment 4?

Comment 3

[Todd Agulnick]

I'm looking at the following code in the bookmark manager: http://lxr.mozilla.org/mozilla1.8/source/browser/components/bookmarks/content/bookmarks.js#1665

Shouldn't his be a call to BMDS.endUpdateBatch() instead of BMDS.beginUpdateBatch()?

Comment 4

[Todd Agulnick]

It looks like this was initially introduced by the fix to Bug 168411, but was masked by another bug in that patch that incorrectly referred to this.BATCH_LIMIT, an attribute that never existed or had been removed.

In Bug 361030, it was noticed that this bad reference was causing a Javascript error, so the code was amended to refer to kBATCH_LIMIT instead. Once was this was fixed, the Javascript error was cleared up, but that now allowed the beginUpdateBatch() call to execute, which is now causing more serious problems.

I'm updating my dev environment so I can produce a patch for this. Is there someone who'd be willing to test it?

Comment 5

[Todd Agulnick]

Attachment: beginUpdateBatch() -> endUpdateBatch()

I don't have a working development environment, so I can't test this. But it's likely the solution to this bug.

Comment 6

[(not reading, please use [e-mail] instead)]

confirming

Comment 7

[(not reading, please use [e-mail] instead)]

todd's suggestion looks correct, I've got a working MOZILLA_1_8 build that I can use to test it.

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 263660 [details] [diff] [review]
beginUpdateBatch() -> endUpdateBatch()

r=sspitzer.  I've reproduced the bug and verified Todd's fix.

thanks, Todd!

I'll go land on the trunk.

if we have a respin for 2.0.0.4, please consider this.  otherwise, this would be good for 2.0.0.5

Comment 9

[(not reading, please use [e-mail] instead)]

fixed on the trunk, thanks again Todd.

Checking in bookmarks.js;
/cvsroot/mozilla/browser/components/bookmarks/content/bookmarks.js,v  <--  bookm
arks.js
new revision: 1.129; previous revision: 1.128
done

Comment 10

[Jay Patel [:jay]]

This isn't really a dataloss bug if any new bookmarks show up after a restart.  Has anyone tested that? 

Also, this bug has been around for a while, so we should probably avoid adding more risk to 2.0.0.4 and get this in for 2.0.0.5.  Just my .02

Comment 11

[(not reading, please use [e-mail] instead)]

> This isn't really a dataloss bug if any new bookmarks show up after a restart. 
> Has anyone tested that? 

At least from some simple initial tests, Joey is right, that the changes show up after a restart.

Comment 12

[Todd Agulnick]

(In reply to comment #10)
> This isn't really a dataloss bug if any new bookmarks show up after a restart. 
> Has anyone tested that?

Things do indeed repair themselves after a restart. The problem is that before the restart happens, it's possible to delete X when you meant to delete Y. Is that dataloss? Reasonable people could disagree.

> 
> Also, this bug has been around for a while, so we should probably avoid adding
> more risk to 2.0.0.4 and get this in for 2.0.0.5.  Just my .02
> 

This bug has been around only since 2.0.0.2. I live and breathe bookmarks, so my perspective may be skewed, but I think the value of fixing a bug that can hork the entire bookmark UI is large. It's a less-than-one-line change and its risk is low.

Comment 13

[(not reading, please use [e-mail] instead)]

since we might be respining 2.0.0.4, setting blocker 1.8.1.4 for this less-than-one-line change.

Comment 14

[Jay Patel [:jay]]

(In reply to comment #12)
> (In reply to comment #10)
> > This isn't really a dataloss bug if any new bookmarks show up after a restart. 
> > Has anyone tested that?
> 
> Things do indeed repair themselves after a restart. The problem is that before
> the restart happens, it's possible to delete X when you meant to delete Y. Is
> that dataloss? Reasonable people could disagree.

Thanks for testing that Todd... based on your results, this is indeed a dataloss bug and we should get it fixed sooner than later.

> 
> > 
> > Also, this bug has been around for a while, so we should probably avoid adding
> > more risk to 2.0.0.4 and get this in for 2.0.0.5.  Just my .02
> > 
> 
> This bug has been around only since 2.0.0.2. I live and breathe bookmarks, so
> my perspective may be skewed, but I think the value of fixing a bug that can
> hork the entire bookmark UI is large. It's a less-than-one-line change and its
> risk is low.

Didn't realize this was a recent regression, and after looking at the patch, I think we should take this for the 2.0.0.4 rc2 respin.  I'll leave it to juanb or dveditz to give the approvals.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 263660 [details] [diff] [review]
beginUpdateBatch() -> endUpdateBatch()

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 16

[(not reading, please use [e-mail] instead)]

fixed on the MOZILLA_1_8_BRANCH

Checking in content/bookmarks.js;
/cvsroot/mozilla/browser/components/bookmarks/content/bookmarks.js,v  <--  bookm
arks.js
new revision: 1.104.2.23; previous revision: 1.104.2.22
done

thanks again to Todd for the fix.

Comment 18

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.4 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4pre) Gecko/2007050804 BonEcho/2.0.0.4pre and the steps to reproduce from this bug and the steps to reproduce from bug 378902 (a dupe of this one). 

The Bookmark manager is working fine also when i delete more than 5 Bookmarks. Adding verified keyword