1163447 - selectItems in Places no longer selects items within Toolbar or Sidebar folders

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[mchenryc]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36

Steps to reproduce:

This is a regression in an API call which cannot be invoked directly through the GUI, introduced in FF-34.

1. Create a bookmark folder in the toolbar (unfortunately UI has no easy way to get the ID).
2. Open the Places organizer (aka 'Library'), and select 'All Bookmarks' (just to ensure target is not selected).
3. From the `places` tree object, invoke `selectItems([id])` with the id of the folder created in step 1

I am in the process of creating a test which demonstrate the issue, to be attached to this bug.



Actual results:

Nothing: the selectItems(...) method returns without selecting the target folder.


Expected results:

The folder created in step 1 should have been selected in the tree of the Places view.

Regression introduction:
  file: http://hg.mozilla.org/mozilla-central/diff/722fd1b0e050/browser/components/places/content/tree.xml#l1.100
  changeset: http://hg.mozilla.org/mozilla-central/rev/722fd1b0e050
  bug: https://bugzilla.mozilla.org/show_bug.cgi?id=435851

The regression, introduced with the intent of avoiding infinite loops while traversing a bookmark tree (i.e. shortcuts to a parent), inadvertently broke the ability to select bookmarks by ID starting at the root of the Places Tree.

The Places tree has 'All Bookmarks' contains shortcuts to 'Bookmarks Toolbar' and 'Bookmarks Menu', neither of which are traversed.

Note that even if traversal is started from one of the top level folders (i.e. 'Toolbar Folder', or 'Bookmarks Menu'), the issue can occur if a shortcut points to a sub-tree of the other top level folder.

Comment 1

[mchenryc]

Attachment: bug-1163447-fix.patch

Includes tests which fails prior to fix being applied, and said fix.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8603994 [details] [diff] [review]
bug-1163447-fix.patch

Thanks, I will look into your patch

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8603994 [details] [diff] [review]
bug-1163447-fix.patch

Review of attachment 8603994 [details] [diff] [review]:
-----------------------------------------------------------------

the patch looks mostly good, there's just some minor fixes to do.
Are you working on an add-on that uses Places, or something else? Looks like you know the code. Maybe you might be interested in contributing to other bugs around.

::: browser/components/places/content/tree.xml
@@ +574,4 @@
>            // and thus shouldn't be searched again. This is empty at the initial
>            // start of the recursion and gets filled in as the recursion
>            // progresses.
> +          var nodesGUIDChecked = {};

while here, I's suggest to make this a Set (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set) and rename it to checkedGuidsSet

@@ +600,5 @@
>                ids.splice(index, 1);
>              }
>  
> +            // only lookup concreteGuid if node is a container that needs to be searched
> +            let concreteGuid;

getConcreteItemGuid is an unexpensive call, so you might just call it here and remove the comment.

@@ +605,2 @@
>              if (ids.length == 0 || !PlacesUtils.nodeIsContainer(node) ||
> +                nodesGUIDChecked[(concreteGuid = PlacesUtils.getConcreteItemGuid(node))])

and here use the .has method of the set

@@ +610,3 @@
>              let shouldOpen = aOpenContainers &&
> +                             (node.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER ||
> +                              node.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT);

PlacesUtils.nodeIsFolder(node)

@@ +613,5 @@
>              PlacesUtils.asContainer(node);
>              if (!node.containerOpen && !shouldOpen)
>                return foundOne;
>  
> +            nodesGUIDChecked[concreteGuid] = true;

use .add()

::: browser/components/places/tests/chrome/test_bug1163447_selectItems_through_shortcut.xul
@@ +75,5 @@
> +        // Select the folder.
> +        let itemId = yield PlacesUtils.promiseItemId(folder.guid);
> +        tree.selectItems([itemId]);
> +
> +        is(tree.selectedNode && tree.selectedNode.itemId, itemId, "The node was selected through the shortcut");

rather than getting itemId, you can use .bookmarkGuid and compare with folder.guid

@@ +78,5 @@
> +
> +        is(tree.selectedNode && tree.selectedNode.itemId, itemId, "The node was selected through the shortcut");
> +
> +        // Cleanup
> +        bmu.eraseEverything();

missing a yield

@@ +79,5 @@
> +        is(tree.selectedNode && tree.selectedNode.itemId, itemId, "The node was selected through the shortcut");
> +
> +        // Cleanup
> +        bmu.eraseEverything();
> +      }).then(SimpleTest.finish);

this wouldn't call finish in case of an exception, you likely want something like this:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/chrome/test_371798.xul#93

Comment 4

[mchenryc]

Comment on attachment 8603994 [details] [diff] [review]
bug-1163447-fix.patch

Review of attachment 8603994 [details] [diff] [review]:
-----------------------------------------------------------------

Long ago I made a plugin (https://addons.mozilla.org/en-US/firefox/addon/manage-folders/) and a user comment on AMO alerted me to the regression.

I am interested in doing a little work around Bookmarks Management, and of generally improving the bookmarking UX. Can you recommend higher priority targets in this area? I am still familiarizing myself with bugzilla, and associated practices.

Is itemId in bookmarks deprecated (see response in test_bug1163447....xul)? Regardless - can you point me to a document regarding moz codebase deprecated behavior/functionality/patterns/policy?

::: browser/components/places/tests/chrome/test_bug1163447_selectItems_through_shortcut.xul
@@ +75,5 @@
> +        // Select the folder.
> +        let itemId = yield PlacesUtils.promiseItemId(folder.guid);
> +        tree.selectItems([itemId]);
> +
> +        is(tree.selectedNode && tree.selectedNode.itemId, itemId, "The node was selected through the shortcut");

Was generally sticking with itemId use, as that was the issue being excercised, but indeed, not neccessary here.

You make me wonder though: is use of itemId being deprecated in favor of guid?

Comment 5

[mchenryc]

Attachment: bug-1163447-v1.1.patch

Comment 6

[mchenryc]

Attachment: bug-1163447-v1.2.patch

Comment 7

[Marco Bonardo [:mak]]

please set the review? flag on attachments, or devs won't notice you added something. We get hundreds bugmails a day...

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8610309 [details] [diff] [review]
bug-1163447-v1.2.patch

Review of attachment 8610309 [details] [diff] [review]:
-----------------------------------------------------------------

please add "r=mak" to the commit message

::: browser/components/places/content/tree.xml
@@ +606,5 @@
>                ids.splice(index, 1);
>              }
>  
> +            // only lookup concreteGuid if node is a container that needs to be searched
> +            var concreteGuid = PlacesUtils.getConcreteItemGuid(node);

the comment can be removed now.

::: browser/components/places/tests/chrome/test_bug1163447_selectItems_through_shortcut.xul
@@ +83,5 @@
> +
> +      }).catch(err => {
> +        ok(false, `Uncaught error: ${err}`);
> +
> +      }).then(SimpleTest.finish);

may remove the empty newline up there

Comment 9

[Marco Bonardo [:mak]]

Thank you. Once you attach the updated patch and the following test run is complete, you can add the checkin-needed keyword up in the bug and someone will take care of pushing the patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5843d078e45b

Comment 10

[mchenryc]

Attachment: bug-1163447-v1.3.patch

Comment 11

[Marco Bonardo [:mak]]

hm, looks like there is a failure in mochitest-other test_bug1163447_selectItems_through_shortcut.xul (The new test basically)

Assertion failure: mItemId == mTargetFolderItemId || aItemId != mItemId, at /builds/slave/try-lx-d-000000000000000000000/build/src/toolkit/components/places/nsNavHistoryResult.cpp:3615
TEST-UNEXPECTED-FAIL | browser/components/places/tests/chrome/test_bug1163447_selectItems_through_shortcut.xul | application terminated with exit code 11
PROCESS-CRASH | browser/components/places/tests/chrome/test_bug1163447_selectItems_through_shortcut.xul | application crashed [@ nsNavHistoryFolderResultNode::OnItemRemoved(long long, long long, int, unsigned short, nsIURI*, nsACString_internal const&, nsACString_internal const&)] 

this might indicate the test just hit a bug in some other code, indeed it's likely due to eraseEverything notifying the result that for some reason notifies the folder shortcut about a removal... needs to be debugged to figure if it's a broken assert or an actual bug :(

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch with fixed assertions

I fixed the assertion, it was invalid for recursive shortcuts and looks like we didn't have a test with a recursive shortcut yet.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e3bfbe6ed3

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch with fixed assertions

oops, MOZ_ASSERT_IF doesn't support a message, like MOZ_ASSERT.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8cd474fe3ba

Comment 14

[mchenryc]

Thanks Marco, I would not have been able to look into this for another week yet.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/945d7b08f296

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/945d7b08f296