1095069 - Cannot copy or delete bookmarks from search results in library

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[timbugzilla]

Attachment: Context menu with "delete" grayed out

When searching for bookmarks in the library, one cannot delete bookmarks from this view (displaying the matching bookmarks).

Neither context-menu nor Del key work.

See image for an example.

Comment 1

[Marco Bonardo [:mak]]

Is this a regression or was it doing the same in previous versions (like in Aurora) as well?

I'm more worried about the fact Copy is disabled.
could be a regression from recent read-only changes.

I indeed see an error in the Console r"invalid value for aNodeOrItemId"

  isContentsReadOnly: function (aNodeOrItemId) {
    let itemId;
    if (typeof(aNodeOrItemId) == "number") {
      itemId = aNodeOrItemId;
    }
    else if (PlacesUtils.nodeIsFolder(aNodeOrItemId)) {
      itemId = PlacesUtils.getConcreteItemId(aNodeOrItemId);
    }
    else {
      throw new Error("invalid value for aNodeOrItemId");
    }

I'm sure we should allow to copy, but I don't recall if before we were allowing to remove entries (for sure we don't allow to move/cut), it would make sense

Comment 2

[Marco Bonardo [:mak]]

I think we may be trying to pass the root node to isContentsReadOnly here, the root node is a query.

Comment 3

[timbugzilla]

I think I first noticed it with a nightly build from about 01 Nov 2014. Think I could delete bookmarks from that view in early September.

Comment 4

[Alice0775 White]

[Tracking Requested - why for this release]: regressed by bug 1068671

Comment 5

[Marco Bonardo [:mak]]

I think the fix went too strict, by disallowing removals from queries.
likely the problem is here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#334
canUserRemove for children of queries should likely return true. This off-hand sounds like a nonsense, but there's no case I can think of where we disallow removing from a query. The copy case is probably just a consequence of the exception.

but looks like also canMoveNode is now wrong, it should not allow to move children of queries.

The previous logic was too cumbersome to figure all these cases, and we don't have good tests coverage here.

Comment 6

[Marco Bonardo [:mak]]

Taking this regression.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1

Changes in this patch:

1. fix possible bug (and I actually hit that in the test I wrote for this bug) that may cause us to try reusing a closed Library/Browser window
2. small perf improvement in handling onItemMoved for excludeItems queries, cause I originally thought the bug was there and ended up improving that
3. fix this bug. the fix also affects a couple edge-cases with tags that were mis-handled. the new test goes through some of them.

Comment 8

[Marco Bonardo [:mak]]

I will now push this to Try.

Comment 10

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1d4695785e70

Comment 11

[Marco Bonardo [:mak]]

there is a BC1 failure likely due to a previous test not doing proper cleanup.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 8520647 [details] [diff] [review]
patch v1

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

::: browser/base/content/browser-places.js
@@ +478,1 @@
>        // No currently open places window, so open one with the specified mode.

I'm surprised this is only hit by tests. Please reference the bug # you're working around here.

::: browser/components/places/content/controller.js
@@ +158,2 @@
>            return false;
> +        }

I'm going to file a bug for removing this dialog BTW.

@@ +315,4 @@
>     * @return true if all nodes in the selection can be removed,
>     *         false otherwise.
>     */
> +  _hasRemovableSelection: function PC__hasRemovableSelection() {

_hasRemovableSelection()

::: browser/components/places/tests/browser/head.js
@@ +42,5 @@
>   */
>  function promiseLibrary(aLeftPaneRoot) {
>    let deferred = Promise.defer();
>    let library = Services.wm.getMostRecentWindow("Places:Organizer");
> +  if (library && !library.closed) {

Consider consolidating this method with the (fixed) one in browser-places in some new PlacesUIUtils method (you may keep this method as a forward to avoid fixing callers).

@@ +228,5 @@
> +  let places = [];
> +  if (aPlaceInfo instanceof Ci.nsIURI) {
> +    places.push({ uri: aPlaceInfo });
> +  }
> +  else if (Array.isArray(aPlaceInfo)) {

either this style

@@ +230,5 @@
> +    places.push({ uri: aPlaceInfo });
> +  }
> +  else if (Array.isArray(aPlaceInfo)) {
> +    places = places.concat(aPlaceInfo);
> +  } else {

or this one

@@ +232,5 @@
> +  else if (Array.isArray(aPlaceInfo)) {
> +    places = places.concat(aPlaceInfo);
> +  } else {
> +    places.push(aPlaceInfo)
> +  }

or just do away with the braces.

@@ +236,5 @@
> +  }
> +
> +  // Create mozIVisitInfo for each entry.
> +  let now = Date.now();
> +  for (let i = 0; i < places.length; i++) {

for...of

@@ +251,5 @@
> +
> +  PlacesUtils.asyncHistory.updatePlaces(
> +    places,
> +    {
> +      handleError: function AAV_handleError(aResultCode, aPlaceInfo) {

handleError(aResultCode, aPlaceInfo) etc.

@@ +271,5 @@
> + * Asynchronously check a url is visited.
> + *
> + * @param aURI The URI.
> + * @return {Promise}
> + * @resolves When the check has been added successfully.

when

@@ +289,5 @@
> + * Waits for all pending async statements on the default connection.
> + *
> + * @return {Promise}
> + * @resolves When all pending async statements finished.
> + * @rejects Never.

when, never.

@@ +299,5 @@
> + *       this is a problem only across different connections.
> + */
> +function promiseAsyncUpdates()
> +{
> +  let deferred = Promise.defer();

new Promise and such.

::: toolkit/components/places/PlacesUtils.jsm
@@ +1052,2 @@
>            aJSNode.parent = parent.itemId;
> +        }

remove this change.

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +3859,5 @@
> +                      mOptions->ExcludeItems();
> +
> +  uint32_t index;
> +  nsNavHistoryResultNode* node = FindChildById(aItemId, &index);
> +  if (node && excludeItems && (node->IsURI() || node->IsSeparator())) {

You MOZ_ASSERT node later, shouldn't you do so here?

Comment 13

[Marco Bonardo [:mak]]

I don't think I'm going to change the methods in head.js, cause they are plain copy of the existing ones. if I change them we'll start having too many diverging implementations. We should instead implement PlacesTestUtils.jsm and move them there, at that point we can effectively cleanup them.

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #12)

> ::: toolkit/components/places/nsNavHistoryResult.cpp
> @@ +3859,5 @@
> > +                      mOptions->ExcludeItems();
> > +
> > +  uint32_t index;
> > +  nsNavHistoryResultNode* node = FindChildById(aItemId, &index);
> > +  if (node && excludeItems && (node->IsURI() || node->IsSeparator())) {
> 
> You MOZ_ASSERT node later, shouldn't you do so here?

The moz_assert is only for the case where we move a node inside the same folder, if we move a node across 2 folders, then it's possible we are the destination folder and we cannot have the node.

fwiw I'm investigating an interesting case where we hit another similar assertion onItemRemoved and I'm starting thinking it may be common to not find the node in some edge cases.

Comment 14

[Marco Bonardo [:mak]]

Attachment: interdiff

patch on top, fixes comments (apart the discussed ones), fixes a couple oranges due to tests not properly cleaning up stuff, and avoids some assertions in results due to bug 1097528

Comment 15

[Marco Bonardo [:mak]]

Attachment: 1095069.diff

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e18612121f0

Comment 16

[Marco Bonardo [:mak]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3d02e44a476

Comment 18

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 36.0a1 (2014-11-16), using Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5. 

Marco, there are a few potential issues I spotted while testing around this fix, but I couldn't find any existing bug matches on file for them. I'd be great if you could take a look at them first [1], but if you don't have the time to review them, I'll file separate bugs and CC you.


1: https://etherpad.mozilla.org/Bug1095069

Comment 19

[Marco Bonardo [:mak]]

> 1. [Windows, Linux, Mac] Copying a bookmark then pasting it in the same 
> location results in a JS error

It could be an NS_ERROR_NOT_AVAILABLE thrown by nsNavHistoryResult.cpp... it should be harmless, but I'd suggest to file it and sometimes in future we may fix it, cause errors noise is bad in general.

> 2. [Windows, Linux, Mac] Searching for actual or non-existent bookmarks
> results in uncaught exceptions

The same as above, please file it, but it's not critical.

> 3. [Windows, Linux] Accessing bookmarks directly from the Library's search
> results causes a JS error

I don't think these are related to bookmarks at all, it's related to loading the pages. So, not our fault.

> 4. [Windows, Mac] Cannot drag&drop bookmarks from the Library after a prior
> attempt with a special folder

Please file this, with very specific steps to reproduce. it looks sort of an edge-case so likely not very critical, but better to check it.

5. [Windows, Linux, Mac] The name field is not auto-populated in case of ctrl / cmd + d bookmarking method

This is an e10s bug, bug 1067042 more specifically. Not related to this.

Comment 20

[Andrei Vaida [:avaida]]

Thanks a lot for taking the time to review these, Marco. I filed Bug 1100794 for Issue#1, Bug 1100798 for Issue#2 and Bug 1100814 for Issue#4.