Cannot copy or delete bookmarks from search results in library

VERIFIED FIXED in Firefox 36

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: timbugzilla, Assigned: mak)

Tracking

({regression})

Trunk
Firefox 36
regression
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox35 unaffected, firefox36+ verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8518364 [details]
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.
(Reporter)

Updated

4 years ago
Version: unspecified → Trunk
(Assignee)

Comment 1

4 years ago
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
Blocks: 1068671
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
(Assignee)

Updated

4 years ago
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Updated

4 years ago
Summary: Cannot delete bookmarks from search results in library → Cannot copy or delete bookmarks from search results in library
(Assignee)

Comment 2

4 years ago
I think we may be trying to pass the root node to isContentsReadOnly here, the root node is a query.
(Reporter)

Comment 3

4 years ago
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

4 years ago
[Tracking Requested - why for this release]: regressed by bug 1068671
status-firefox35: --- → unaffected
status-firefox36: --- → affected
tracking-firefox36: --- → ?
(Assignee)

Comment 5

4 years ago
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.
Flags: needinfo?(mano)
(Assignee)

Comment 6

4 years ago
Taking this regression.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
OS: Windows 8.1 → All
(Assignee)

Updated

4 years ago
Flags: needinfo?(mano)

Updated

4 years ago
Iteration: --- → 36.2

Updated

4 years ago
Iteration: 36.2 → 36.3
(Assignee)

Comment 7

4 years ago
Created attachment 8520647 [details] [diff] [review]
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.
Attachment #8520647 - Flags: review?(mano)
(Assignee)

Comment 8

4 years ago
I will now push this to Try.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1096592
(Assignee)

Comment 11

4 years ago
there is a BC1 failure likely due to a previous test not doing proper cleanup.
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?
Attachment #8520647 - Flags: review?(mano) → review+
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Comment 14

4 years ago
Created attachment 8521289 [details] [diff] [review]
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
Attachment #8521289 - Flags: review?(mano)
(Assignee)

Comment 16

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/a3d02e44a476
Target Milestone: --- → Firefox 36
https://hg.mozilla.org/mozilla-central/rev/a3d02e44a476
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
QA Contact: andrei.vaida
(Assignee)

Updated

4 years ago
Blocks: 1099273
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
Status: RESOLVED → VERIFIED
status-firefox36: affected → verified
Flags: needinfo?(mak77)
tracking-firefox36: ? → +
(Assignee)

Comment 19

4 years ago
> 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.
Flags: needinfo?(mak77)
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.
You need to log in before you can comment on or make changes to this bug.