"Bookmark This Page" throws exception on close in bookmarkProperties.js if you have "bookmarkPropertiesDialog/lastUsed" annotations that point to itemIds for folder that don't exist

VERIFIED FIXED in Firefox 3 alpha6

Status

()

VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: stephend, Assigned: moco)

Tracking

Trunk
Firefox 3 alpha6
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a6pre) Gecko/20070615 Minefield/3.0a6pre

Summary: "Bookmark This Page" throws exception in bookmarkProperties.js

Steps to Reproduce:

1. Click "Bookmarks" then "Bookmark This Page"
2. Close the dialog

Expected Results:

No exception(s) thrown

Actual Results:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getItemTitle]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/bookmarkProperties.js :: BPP__appendFolderItemToMenuList :: line 379"  data: no]

NOTE: I can successfully add the bookmark (if I so choose), but the fact that we're throwing an exception seems bad...
Dup of https://bugzilla.mozilla.org/show_bug.cgi?id=384690

fix is in the works...

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a6pre) Gecko/20070616 Minefield/3.0a6pre Firefox/3.0 ID:2007061615
fixed by Bug 384690
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Depends on: 384690
Resolution: --- → FIXED
(Reporter)

Comment 3

11 years ago
You sure?  2007-06-17-04 isn't fixed, and that's a build after the patch landed...
oops, it looks like I did this in the wrong bug
(Reporter)

Updated

11 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm hitting this exception with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070620 Minefield/3.0a6pre.

I think the problem has to do with the "bookmarkPropertiesDialog/lastUsed", see bug #375629, or something related.  (In my case though, I don't think I deleted any folder and when looking at places.sqlite, I don't see any stale annotations.)

at the worst case, we should handle this gracefully properly if a "last used folder" doesn't exist.
Status: REOPENED → ASSIGNED
here's what was going on for me.  my annotations were valid, but I was getting to the add bookmark dialog through some code that was not updated when showAddBookmarkUI() was updated to take aDefaultInsertionPoint.  

the code in question is code that adds a bookmark as a sidebar panel, and there are two ways to do that.  (test case coming)

We were passing in true for aDefaultInsertionPoint, so aDefaultInsertionPoint.itemId was undefined.

I was hitting the exception that stephen was hitting because we'd eventually call _appendFolderItemToMenupopup() with undefined, fail to get the title annotation for that id, throw an exception.

that would cause us to bail out of _initFolderMenuList(), which is called on load before we call sizeToContent().  (tip of the propeller hat to stephen in bug #384733, comment #0)

I've got a patch that:

1) fixes the two sidebar related callers
2) makes our code handle not finding the title of a folder id better.  we'll assert, but then gracefully fail to append the menuitem and select the first item, which is the bookmarks root.

since I claim you could get into this state with a stale bookmarkPropertiesDialog/lastUsed annotation (from before mano's fix for bug #375629 or something else), for nightly testers who see this, I'm hoping they will report a bug with a screen shot to the assertion.

finally, there is one spin off bug, which is even after this fix, the add bookmark dialog appears to have problems (too short) when adding a sidebar bookmark and trying to expand the folder picker.  I'll log a bug on that.
Assignee: nobody → sspitzer
Status: ASSIGNED → NEW
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha6
> the add bookmark dialog appears to have problems (too short) when adding a 
> sidebar bookmark and trying to expand the folder picker.  I'll log a bug on 
> that.

See bug #385497
Status: NEW → ASSIGNED
Comment on attachment 269407 [details] [diff] [review]
patch

>Index: browser/components/places/content/bookmarkProperties.js
>===================================================================

>+    // if we fail to get the menuitem for the default insertion point
>+    // use the first item, which is the Bookmarks root
>+    if (defaultItem)
>+      this._folderMenuList.selectedItem = defaultItem;
>+    else
>+      this._folderMenuList.selectedIndex = 0;

or cleaner:

  if (!defaultItem)
    defaultItem = this._element("bookmarksRootItem");
  this._folderMenuList.selectedItem = defaultItem

r=mano otherwise, though I would still like to figure out the broken annotations story here (the first item in moz_bookmarks has item_id==1...).
Attachment #269407 - Flags: review?(mano) → review+
Created attachment 269444 [details] [diff] [review]
patch
Attachment #269407 - Attachment is obsolete: true
fixed.

Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--
 bookmarkProperties.js
new revision: 1.52; previous revision: 1.51
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Summary: "Bookmark This Page" throws exception in bookmarkProperties.js → "Bookmark This Page" throws exception on close in bookmarkProperties.js if you have "bookmarkPropertiesDialog/lastUsed" annotations that point to itemIds for folder that don't exist
> I would still like to figure out the broken annotations story here

me too.  Some how stephen ended up with a places.sqlite file with a bookmarkPropertiesDialog/lastUsed annotation for item_id of 0.  I tried re-importing his pre-places bookmarks.html file, but I couldn't reproduce the problem
(Reporter)

Updated

11 years ago
No longer depends on: 384690
(Reporter)

Comment 13

11 years ago
Build: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a6pre) Gecko/20070622 Minefield/3.0a6pre

From: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-win32-tbox-trunk/

Note that I hit the assertion (https://bugzilla.mozilla.org/attachment.cgi?id=269418) (from bug 384733 comment 10), but after dismissing the assertion dialog, I can bookmark just fine, with no exception thrown in the Error Console.

Using my steps in comment 0, this is now Verified FIXED on trunk.
Status: RESOLVED → VERIFIED

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.