When dragging and dropping a bookmark which is located just under special folders, it is executed as "copy" instead of "move".

VERIFIED FIXED in Firefox 60

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: alice0775, Assigned: standard8)

Tracking

({regression})

60 Branch
mozilla60
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
"Peja Stija" reproted in mozillazine(http://forums.mozillazine.org/viewtopic.php?p=14791475#p14791475).


When dragging and dropping a bookmark between special folders(Bookmarks Toolbar, Bookmarks Menu, Other Bookmarks), it is executed as "copy" instead of "move".

This happens not only folders but also regular bookmarks.

Reproducible: always

Steps to reproduce:
1. Open Bookmarks Sidebar
2. Drag aaaaa and drop it between the "Other Bookmarks" and ccccc..

Bookmarks Toolbar
Bookmarks Menu
   aaaaa
   bbbbb
Other Bookmarks
   ccccc

Actual results:

Bookmarks Toolbar
Bookmarks Menu
   aaaaa
   bbbbb
Other Bookmarks
   aaaaa
   ccccc

Expected results:

Bookmarks Toolbar
Bookmarks Menu
   bbbbb
Other Bookmarks
   aaaaa
   ccccc



Regresion window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=12de5643ae0ae6ad6a97797f2a07fe856f9b66e1&tochange=c388570c330fd7745f12eca3258cc3ae4b3d5bb2
(Reporter)

Comment 1

a year ago
it is happens when dragging and dropping a bookmark which is located just under special folders,

Steps to reproduce:
1. Open Bookmarks Sidebar
2. Drag aaaaa and drop it under bbbbb.

Bookmarks Toolbar
Bookmarks Menu
   aaaaa
   bbbbb
Other Bookmarks
   ccccc

Actual results:

Bookmarks Toolbar
Bookmarks Menu
   aaaaa
   bbbbb
   aaaaa
Other Bookmarks
   ccccc


Expected results:

Bookmarks Toolbar
Bookmarks Menu
   bbbbb
   aaaaa
Other Bookmarks
   ccccc
Summary: When dragging and dropping a bookmark between special folders, it is executed as "copy" instead of "move". → When dragging and dropping a bookmark which is located just under special folders, it is executed as "copy" instead of "move".
(Reporter)

Comment 2

a year ago
Browser Console shows the following error:

Tried to move an unmovable Places node, reverting to a copy operation.  controller.js:1688
	getTransactionsForTransferItems chrome://browser/content/places/controller.js:1688:7
	InterpretGeneratorResume self-hosted:1272:8
	next self-hosted:1227:9
(Assignee)

Updated

a year ago
Assignee: nobody → standard8
Priority: -- → P1
Whiteboard: [fxsearch]
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
mozreview-review
Comment on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review226670

::: browser/components/places/content/controller.js:1385
(Diff revision 1)
>      if ((unwrappedNode.concreteGuid && PlacesUtils.isRootItem(unwrappedNode.concreteGuid)) ||
>          unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id)) {
>        return false;
>      }
> -    let parentId = unwrappedNode.parent;
> -    if (parentId <= 0 ||
> +    let parentGuid = unwrappedNode.parentGuid;
> +    if (parentGuid == PlacesUtils.bookmarks.rootGuid ||

I haven't been able to find a case where we wouldn't get the parentId, and I couldn't seem to make a query inside a query, so I don't think we could hit this case.

I swapped to guids as that seems better for the future and easier to handle.

Comment 5

a year ago
mozreview-review
Comment on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review226760

::: browser/components/places/content/controller.js:1385
(Diff revision 1)
>      if ((unwrappedNode.concreteGuid && PlacesUtils.isRootItem(unwrappedNode.concreteGuid)) ||
>          unwrappedNode.id <= 0 || PlacesUtils.isRootItem(unwrappedNode.id)) {
>        return false;
>      }
> -    let parentId = unwrappedNode.parent;
> -    if (parentId <= 0 ||
> +    let parentGuid = unwrappedNode.parentGuid;
> +    if (parentGuid == PlacesUtils.bookmarks.rootGuid ||

So, that is pretty much a generated (not on-disk) query that returns bookmarks, and we are dragging one of those bookmarks.
I suppose it could happen with tags? the main Tags query generates single tag queries (with no ids) and each tag contains bookmarks (that have an id).
This is maybe currently covered by the tagsGuid check, but maybe not, because I don't see why parent and grandParent would be set on those to the tags guid... It's all queries.
From tag containers currently we always copy, please check that's still the case.

::: browser/components/places/tests/browser/browser_controller_onDrop_sidebar.js:3
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: I don't care much, but could use PD

::: toolkit/components/places/PlacesUtils.jsm:175
(Diff revision 1)
>        } else {
>          // This is a bookmark folder.
>          data.type = PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER;
> +        if (isVirtualRoot) {
> +          data.itemGuid = PlacesUtils.getConcreteItemGuid(aNode);
> +          data.id = concreteId;

This worries me a bit, because it seems to expose the underlying root to unwanted operations (like a move). It makes the unwrapped node look like being the root itself. If we mess up somehow the consumer of unwrapNodes, we are likely to cause corruption.

Why is not ok to keep the original code setting it as  a folder shortcut with a concreteId and concreteGuid and rather fix the consumer?
The first comment is an answer to the question in comment 4 (bugzilla doesn't help making that clear)
(Assignee)

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review226760

> This worries me a bit, because it seems to expose the underlying root to unwanted operations (like a move). It makes the unwrapped node look like being the root itself. If we mess up somehow the consumer of unwrapNodes, we are likely to cause corruption.
> 
> Why is not ok to keep the original code setting it as  a folder shortcut with a concreteId and concreteGuid and rather fix the consumer?

My thinking was that it seemed better to fix the source, as that's specifying what wants to be copied/moved. However, I'm ok with moving that to the consumer (although I think it'll may be more complex there, but I can take a look).

Comment 8

a year ago
mozreview-review
Comment on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review227192
Attachment #8951444 - Flags: review?(mak77)
(Assignee)

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review226760

> My thinking was that it seemed better to fix the source, as that's specifying what wants to be copied/moved. However, I'm ok with moving that to the consumer (although I think it'll may be more complex there, but I can take a look).

Per discussion on irc, we realised that in actual fact the previous behaviour was to copy folder shortcuts. We want to maintain that at the moment, hence dropping that part from this patch.
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review226760

> So, that is pretty much a generated (not on-disk) query that returns bookmarks, and we are dragging one of those bookmarks.
> I suppose it could happen with tags? the main Tags query generates single tag queries (with no ids) and each tag contains bookmarks (that have an id).
> This is maybe currently covered by the tagsGuid check, but maybe not, because I don't see why parent and grandParent would be set on those to the tags guid... It's all queries.
> From tag containers currently we always copy, please check that's still the case.

You're right, that tags guids don't get set. So I've checked that in those cases we don't actually get a guid set, and I've updated the if statement.
Comment hidden (mozreview-request)

Comment 12

a year ago
mozreview-review
Comment on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review227820

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 2)
>      data.itemGuid = guid;
> -    if (aNode.parent)
> +    if (aNode.parent) {
>        data.parent = aNode.parent.itemId;
> -    let grandParent = aNode.parent && aNode.parent.parent;
> -    if (grandParent)
> +      data.parentGuid = aNode.parent.bookmarkGuid;
> +    }
> -      data.grandParentId = grandParent.itemId;

There are a couple uses of data.grandParentId in this same code that should be fixed.
https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/components/places/PlacesUtils.jsm#155,179
Attachment #8951444 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbd1c8bcb246
Fix moving bookmarks across special folders. r=mak

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbd1c8bcb246
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
I was able to reproduce the issue on Win 10 x64 using Fx 60.0a1 with the build ID: 20180210220139.
Tested on Win10 x64, Mac 10.13.3 and Ubuntu 14.04 x64 using the latest Fx Beta (Fx 60.0b6) and I can confirm that the issue has been fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.