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

RESOLVED FIXED in Firefox 2

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Xavier, Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

({regression, verified1.8.1.4})

unspecified
Firefox 2
x86
Windows XP
regression, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.4 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.06 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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

10 years ago
Maybe related to bug 373800 and/or bug 347103 comment 4?
Duplicate of this bug: 376309

Comment 3

10 years ago
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

10 years ago
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

10 years ago
Created attachment 263660 [details] [diff] [review]
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.
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true
todd's suggestion looks correct, I've got a working MOZILLA_1_8 build that I can use to test it.
Assignee: nobody → sspitzer
Keywords: regression
Status: NEW → ASSIGNED
Depends on: 168411

Updated

10 years ago
Flags: blocking1.8.1.5?
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
Attachment #263660 - Flags: review+
Attachment #263660 - Flags: approval1.8.1.5?
Attachment #263660 - Flags: approval1.8.1.4?
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 2

Comment 10

10 years ago
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
> 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

10 years ago
(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.
since we might be respining 2.0.0.4, setting blocker 1.8.1.4 for this less-than-one-line change.
Flags: blocking1.8.1.4?

Comment 14

10 years ago
(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 on attachment 263660 [details] [diff] [review]
beginUpdateBatch() -> endUpdateBatch()

approved for 1.8.1.4, a=dveditz for release-drivers
Attachment #263660 - Flags: approval1.8.1.5?
Attachment #263660 - Flags: approval1.8.1.4?
Attachment #263660 - Flags: approval1.8.1.4+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
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.
Keywords: fixed1.8.1.4
Duplicate of this bug: 378902
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
Keywords: fixed1.8.1.4 → verified1.8.1.4
Duplicate of this bug: 381627
You need to log in before you can comment on or make changes to this bug.