Closed Bug 1437310 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Places, defect, P1)

60 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified

People

(Reporter: alice0775, Assigned: standard8)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

"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
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".
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: nobody → standard8
Priority: -- → P1
Whiteboard: [fxsearch]
Status: NEW → ASSIGNED
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 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)
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 on attachment 8951444 [details]
Bug 1437310 - Fix moving bookmarks across special folders.

https://reviewboard.mozilla.org/r/220746/#review227192
Attachment #8951444 - Flags: review?(mak77)
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.
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 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+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbd1c8bcb246
Fix moving bookmarks across special folders. r=mak
https://hg.mozilla.org/mozilla-central/rev/dbd1c8bcb246
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: qe-verify+
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.