356017 - Clicking twice 'OK' needed for creating a bookmark with microsummary

Status: VERIFIED FIXED

(Firefox Graveyard :: Microsummaries, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061003 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061003 Firefox/2.0

Using a microsummary generator which catches two elements of a website an uses them for the the Microsummary, the 'OK'-Button very often (>90%) needs to beclicked twice for closing the dialog. It results in two Bookmarks, the first one with an empty title, the second one with correct title.

Reproducible: Sometimes

Steps to Reproduce:
1. Open a website for which you have a microsummary generator which picks more than one element from the site.
2. Choose 'Add Bookmark'
3. Choose the microsummary as title.
4. Click 'OK' the first time.
5. Click 'OK' the second time.
6. Delete the bookmark with the empty title.
Actual Results:  
'OK' needs to be clicked twice, a bookmark with empty title is created.

Expected Results:  
One bookmark with correct microsummary title is created.

http://www.chip.de/downloads/* cause the problem, f.e. http://www.chip.de/downloads/c1_downloads_14963206.html

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: used generator which causes the problem on http://www.chip.de/downloads/*

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The problem is seen when the folder tree has been opened.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Hmm, it sounds like the microsummary service is throwing an error the first time you click the button, perhaps after setting the bookmark type to "microsummary bookmark" but before setting the value of the bookmark's "generated title" property.  That would explain why the title of the first bookmark is empty.

Unfortunately, I don't seem to be able to reproduce the bug using the generator you attached and the page you mention.

Does anything unusual show up on the Error Console after the first click?

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

After the first click there are no errors shown. But after the secondan error is thrown:
Error: bookmark [xpconnect wrapped nsIRDFResource] does not have a microsummary
Source: file:///F:/Software/Browser/Firefox/components/nsMicrosummaryService.js
Line: 242

After creating the title of the first bookmark can be changed to the microsummary. It's probably Not the microsummary generator because i had the same problem also on another web site.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Ok, I can reproduce this now.  For me the problem only happens when I click the button to the right of the "Create in" drop-down menu to open the folder tree and  then select a folder from the tree.

The first time I click the Add button, the following happens:

1. the folder I selected in the folder tree becomes unselected
   (although it remains selected in the "Create in" drop-down menu);
2. a bookmark with a blank title is created in that folder;
3. the dialog doesn't close;
4. the following errors appear on the error console:

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXULTreeBuilder.getResourceAtIndex]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://browser/content/bookmarks/bookmarksTree.xml :: get_currentResource :: line 251" data: no]
bookmarksTree.xml (line 251)
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXULTreeBuilder.getResourceAtIndex]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://browser/content/bookmarks/bookmarksTree.xml :: get_currentResource :: line 251" data: no]
bookmarksTree.xml (line 251)

A few seconds later, without further intervention on my part, this error appears on the error console:

bookmark [xpconnect wrapped nsIRDFResource] does not have a microsummary
nsMicrosummarySer... (line 242)

The second time I click the Add button, the dialog closes, and a bookmark with the microsummary as its title appears in the folder I selected.

But if I reselect the folder in the folder tree between the first and second clicks on the Add button, the erroneous behavior from the first click recurs after the second click.  And it continues to recur if I continue to reselect the folder (or any other) before clicking the Add button again.

So the problem is specific to folders selected in the folder tree.

Comment 6

[Johannes la Poutre]

I got the same behavior both on Mac OSX (10.4.8, Intel) and Win32 (XP-SP2), FF 2.0 release candidate 3 on  both platforms.

The steps to reproduce:

- Add bookmark (Ctrl/Cmd-D)
- Select Live Title instead of default bookmark title
- Expand the folder listing pane
- Select target folder
- Click "Add"
- Click "Add" again (NOTE)

--> nothing happens, but an empty titled bookmark appears to be added  for every time I clicked Add (shown afterwards through Manage Bookmarks). 
On Mac OSX the dialog doesn't close even after the second click, the dialog can only be dismissed by clicking Cancel.
 From the Bookmarks Management interface, the properties can be edited, and this time the live title can be correctly selected and saved.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes problem

What's happening here is that when the Add button is clicked, addBookmark2.js::onOK() calls MicrosummaryPicker::commit(), which calls nsMicrosummaryService::setMicrosummary() to set the microsummary for the bookmark.

But setting a microsummary triggers bookmark trees to rebuild as a workaround for bug 348928, and the rebuild triggers the "onselect" handler for the bookmarks tree in the dialog, which calls addBookmark2.js::selectTreeFolder().

selectTreeFolder() doesn't realize a rebuild is taking place and that an item has just been unselected.  It assumes an item has just been selected, and it calls bookmarksTree.xml::currentResource() to get the resource corresponding to the newly selected item.

currentResource() then calls nsXULTreeBuilder::getResourceAtIndex() passing it the value -1 for its aRowIndex argument.  But -1 isn't a valid value for that argument, so getResourceAtIndex() throws an error.

I think the simplest (and perhaps best) solution is for addBookmark2.js::selectTreeFolder() to ensure some item is selected before handling the select event, since there's nothing for that function to do if no folder is selected.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Fix checked in to the trunk.  This is a pretty serious bug for microsummaries, and the fix is very low risk (it's a null check), so I think this should go into the 2.0 branch for inclusion in the next minor update.

Comment 9

[Robert Sayre]

do we have test coverage for this?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> do we have test coverage for this?

It doesn't look like it:

http://litmus.mozilla.org/show_test.cgi?type=by_category&product=1&testgroup=27&subgroup=496

cc:ing Zach, who might be able to add a test to litmus (or know who can).

Comment 11

[Myk Melez [:myk] [@mykmelez]]

cc:ing other folks who could add a test for this.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Marcia gave me privs on litmus, and I've added test 2709:

http://litmus.mozilla.org/show_test.cgi?id=2709

Comment 13

[Jay Patel [:jay]]

Comment on attachment 242737 [details] [diff] [review]
patch v1: fixes problem

Approved for 1.8.1 branch.  a=jay for drivers, please land asap.  Thanks!

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Fix checked in to the 1.8 branch (MOZILLA_1_8_BRANCH):

Checking in browser/components/bookmarks/content/addBookmark2.js;
/cvsroot/mozilla/browser/components/bookmarks/content/addBookmark2.js,v  <--  addBookmark2.js
new revision: 1.36.2.10; previous revision: 1.36.2.9
done

Comment 15

[Chris Cooper [:coop] (he/him)]

WFM:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061130 BonEcho/2.0.0.1pre