346662 - crash when deleting a Live bookmark via "Bookmark Properties" [@ BindStatementURI]

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[baffclan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060730 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060730 Minefield/3.0a1

crash when delete a Live bookmark on "Bookmark Properties"

Reproducible: Always

Steps to Reproduce:
1. Subscribe Live Bookmark on Bookmaeks toolbar
2. rigth click a Live Bookmark, and open Bookmark Properties
3. click a "Delete Bookmark" on "Bookmark Properties"

Actual Results:  
crash a Firefox.



Talkback ID : TB21619770X, TB21619766Q, TB21619761W

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Can confirm this - quick search found no duplicates so marking as new (please re-mark is a dupe is found.)

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Incident ID: 21619770
Stack Signature	BindStatementURI dce28dd2
Product ID	FirefoxTrunk
Build ID	2006073004
Trigger Time	2006-07-31 05:09:01.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	firefox.exe + (00482467)
URL visited	
User Comments	
Since Last Crash	14 sec
Total Uptime	49952 sec
Trigger Reason	Access violation
Source File, Line No.	c:\builds\tinderbox\fx-trunk-cairo\winnt_5.2_depend\mozilla\toolkit\components\places\src\nsnavhistory.cpp, line 4301
Stack Trace 	
BindStatementURI
nsNavBookmarks::GetBookmarkFoldersTArray
nsNavBookmarks::GetBookmarkFolders
XPTC_InvokeByIndex
XPCWrappedNative::CallMethod

Comment 3

[Joel Nackman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060730 Minefield/3.0a1

Also crashes on the Mac (by hitting "Get Info", instead of "Properties").

Comment 4

[Joel Nackman]

Attachment: Patch

This fixes the bug on my build, but I'm not completely sure that this is the correct way to fix it.  BPP_deleteBookmark was using _bookmarkURI even when the bookmark was a livemark, meaning it was a folder ID, and _bookmarkURI was null.

http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/bookmarkProperties.js#669

Comment 5

[Adam Guthrie]

Comment on attachment 231589 [details] [diff] [review]
Patch

I actually think someone like Annie Sullivan (annie.sullivan@g) or Brett Wilson (brettw@g) would be more appropriate to review this, since they wrote the code.

Comment 6

[Joel Nackman]

Attachment: Patch (fixed indents)

Got rid of the tabs and made 2 space indents.

Comment 7

[Annie Sullivan]

Comment on attachment 231605 [details] [diff] [review]
Patch (fixed indents)

This looks like the right way to fix it, but the indentation is still a little messed up.


>+   var transactions = [];
>+	

Everything is indented 3 characters instead of 4. There's a tab on the blank line here

>+   if (this._identifierIsURI()) {
>+     var folders = this._bms.getBookmarkFolders(bookmarkURI, {});
>+     if (folders.length == 0)
>+     return;
>+
>+     for (var i = 0; i < folders.length; i++) {
>+       var index = this._bms.indexOfItem(folders[i], bookmarkURI);
>+       var transaction = new PlacesRemoveItemTransaction(bookmarkURI,
>+	                                                        folders[i], index)

The first character of this last line is a tab

>+       transactions.push(transaction);
>+     }
>+   } else { // This is a folder Id
>+	  var transaction = new PlacesRemoveFolderTransaction(this._folderId);
>+	  transactions.push(transaction);
>+   }

The "var transaction" and "transactions.push(" lines start with tabs, as does the following blank line

>+	
>     var aggregate =
>       new PlacesAggregateTransaction(this._getDialogTitle(), transactions);
>     this._controller.tm.doTransaction(aggregate);
>   },
> 
>   /**
>    * This method checks the current state of the input fields in the
>    * dialog, and if any of them are in an invalid state, it will disable

Sorry to be so nit-picky.

Comment 8

[Joel Nackman]

Attachment: Patch w/correct indents

The indents should hopefully be correct now.  diff was somehow stripping a space from all of my lines.

Comment 9

[Adam Guthrie]

Comment on attachment 231620 [details] [diff] [review]
Patch w/correct indents

Joel, this patch doesn't need superreview; it's ready for check in. Just add [checkin needed] to the Status Whiteboard so people (e.g. gavin) know that it needs to be checked in.

Comment 10

[Joel Nackman]

Oh, I wasn't really sure, sorry.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/places/content/bookmarkProperties.js 	1.22

Comment 12

[baffclan]

cannot reproduce with Firefox/2006081204-trunk/WinXP.
thanks for fixing this.

-> v.

Comment 13

[Adam Guthrie]

*** Bug 348560 has been marked as a duplicate of this bug. ***

Comment 14

[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