Copy & paste Bookmark will fail between 2 instance of browser

VERIFIED FIXED in Firefox 58

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: alice0775, Assigned: standard8)

Tracking

(Depends on 1 bug, {regression, reproducible, ux-consistency})

56 Branch
mozilla58
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 disabled, firefox58 verified)

Details

(Whiteboard: [parity-chrome][fxsearch])

Attachments

(1 attachment)

Reporter

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1177654 +++

Bookmarks feature of Firefox is gradually broken and broken while version escalation. :(

The problem is started since Nightly56.0a1.
This problem happens Bookmarks menu/toolbar as well as Library.

Steps to reproduce:
1. Start Firefox[A]
2. Start Firefox[B] with -no-remote -P different profile
3. Copy a Bookmark on browser[A]
4. Paste on browser[B]

Actual Results:
Bookmark would not be copied.

Expected Results:
Bookmark should be copied.
Reporter

Updated

2 years ago
Version: 41 Branch → 56 Branch
Reporter

Comment 1

2 years ago
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0da00124af43d44fed96133300ba5e32b0d821a8&tochange=0a4690dfd7b383e2f732210cf8906ce51a5b2433

Regressed by: 0a4690dfd7b3	Mark Banner — Bug 1071513 - Enable async PlacesTransactions for nightly builds. r=mak
Blocks: 1071513
No longer blocks: 1095411
According to Bug 1177654 this was broken already? Or were only folders broken before?
Priority: -- → P2
Whiteboard: [parity-Chrome] → [parity-chrome][fxsearch]
Reporter

Comment 3

2 years ago
Prior to land Bug 1071513, it was broken only folder.

Updated

2 years ago
Priority: P2 → P3
P2 since this was already partially broken, but should be fixed.
Alice, I can't reproduce this. I tried between bookmark toolbars, and between the library windows.

Can you retry, and provide a few more detailed steps if you still see it? Thanks.
Flags: needinfo?(alice0775)
Reporter

Comment 6

2 years ago
I can still reproduce the problem on Nightly57.0a1(2017-08-22) Build ID 20170822171841.

Steps To Reproduce
1. Launch Nightly (say [browser A])
   firefox.exe -P foofoo
2. Launch another instance of Nightly (say [browserB])
   firefox -no-remote -P barbar

3. Copy a bookmark from [browser A]
   Right click on a bookmark item from Library, menubar, sidebar, toolbar of [browser A]
   Choose "Copy"
4. Paste it to [browser B]
   Right click on Library, menubar, sidebar, toolbar of [browser B]
   Choose "Paste"


Actual Results:
Nothing pasted

Browser console for [browser B] shows tthe following error when did "paste":

TypeError: creationInfo is null
Stack trace:
execute@resource://gre/modules/PlacesTransactions.jsm:1632:5
Async*transact/promise<@resource://gre/modules/PlacesTransactions.jsm:546:26
async*enqueue/promise<@resource://gre/modules/PlacesTransactions.jsm:472:58
promise callback*enqueue@resource://gre/modules/PlacesTransactions.jsm:472:19
transact@resource://gre/modules/PlacesTransactions.jsm:543:19
transact@resource://gre/modules/PlacesTransactions.jsm:231:16
paste/<@chrome://browser/content/places/controller.js:1294:30
async*batch/<@resource://gre/modules/PlacesTransactions.jsm:566:20
async*enqueue/promise<@resource://gre/modules/PlacesTransactions.jsm:472:58
promise callback*enqueue@resource://gre/modules/PlacesTransactions.jsm:472:19
batch@resource://gre/modules/PlacesTransactions.jsm:561:12
batch@resource://gre/modules/PlacesTransactions.jsm:340:14
paste@chrome://browser/content/places/controller.js:1279:15
async*PC_doCommand@chrome://browser/content/places/controller.js:241:7
goDoPlacesCommand@chrome://browser/content/places/controller.js:1723:5
oncommand@chrome://browser/content/browser.xul:1:1
  Console.jsm:505
TypeError: creationInfo is null
Stack trace:
execute@resource://gre/modules/PlacesTransactions.jsm:1632:5
Async*transact/promise<@resource://gre/modules/PlacesTransactions.jsm:546:26
async*enqueue/promise<@resource://gre/modules/PlacesTransactions.jsm:472:58
promise callback*enqueue@resource://gre/modules/PlacesTransactions.jsm:472:19
transact@resource://gre/modules/PlacesTransactions.jsm:543:19
transact@resource://gre/modules/PlacesTransactions.jsm:231:16
paste/<@chrome://browser/content/places/controller.js:1294:30
async*batch/<@resource://gre/modules/PlacesTransactions.jsm:566:20
async*enqueue/promise<@resource://gre/modules/PlacesTransactions.jsm:472:58
promise callback*enqueue@resource://gre/modules/PlacesTransactions.jsm:472:19
batch@resource://gre/modules/PlacesTransactions.jsm:561:12
batch@resource://gre/modules/PlacesTransactions.jsm:340:14
paste@chrome://browser/content/places/controller.js:1279:15
async*PC_doCommand@chrome://browser/content/places/controller.js:241:7
goDoPlacesCommand@chrome://browser/content/places/controller.js:1723:5
oncommand@chrome://browser/content/browser.xul:1:1
  Console.jsm:505
TypeError: creationInfo is null
Stack trace:
execute@resource://gre/modules/PlacesTransactions.jsm:1632:5
Async*transact/promise<@resource://gre/modules/PlacesTransactions.jsm:546:26
async*enqueue/promise<@resource://gre/modules/PlacesTransactions.jsm:472:58
promise callback*enqueue@resource://gre/modules/PlacesTransactions.jsm:472:19
transact@resource://gre/modules/PlacesTransactions.jsm:543:19
transact@resource://gre/modules/PlacesTransactions.jsm:231:16
paste/<@chrome://browser/content/places/controller.js:1294:30
async*batch/<@resource://gre/modules/PlacesTransactions.jsm:566:20
async*enqueue/promise<@resource://gre/modules/PlacesTransactions.jsm:472:58
promise callback*enqueue@resource://gre/modules/PlacesTransactions.jsm:472:19
batch@resource://gre/modules/PlacesTransactions.jsm:561:12
batch@resource://gre/modules/PlacesTransactions.jsm:340:14
paste@chrome://browser/content/places/controller.js:1279:15
async*PC_doCommand@chrome://browser/content/places/controller.js:241:7
goDoPlacesCommand@chrome://browser/content/places/controller.js:1723:5
oncommand@chrome://browser/content/browser.xul:1:1
  Console.jsm:505
creationInfo is null  PlacesTransactions.jsm:1632
Flags: needinfo?(alice0775)
Thanks Alice - I've tested again and I can't reproduce, however Marco can reproduce on Windows, so we're thinking this is platform specific in some way.
OS: Unspecified → Windows
This is a WIP for a fix for this bug. Basically we were assuming the bookmark data with a guid was always going to be in our database, and that isn't true for cross-browser pasting.

I think the patch is working ok, but will need the tests completing.

I also did a try build with this in.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66fd95adcfb2c16e238856640644ef963d4aecf&selectedJob=129446753
Assignee: nobody → standard8
nice to have, but not a feature blocker.
No longer blocks: PlacesAsyncTransact
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8906558 [details]
Bug 1386513 - Handle cases of pasted bookmarks not being in the database, to fix copying bookmarks across browser instances.

https://reviewboard.mozilla.org/r/178304/#review190988

::: browser/components/places/PlacesUIUtils.jsm:595
(Diff revision 2)
> -  getTransactionForData(aData, aType, aNewParentGuid, aIndex, aCopy) {
> +  async getTransactionForData(aData, aType, aNewParentGuid, aIndex, aCopy) {
>      if (!this.SUPPORTED_FLAVORS.includes(aData.type))
>        throw new Error(`Unsupported '${aData.type}' data type`);
>  
>      if ("itemGuid" in aData) {
> +      let existingBookmark = await PlacesUtils.bookmarks.fetch(aData.itemGuid);

Sounds like this will be a perf hog, if the data involves hundreds of bookmarks, we have to fetch all of them?
Could we instead put something unique per instance in the data, so we can know it's from a different instance?I suppose we could add an instance id into the blob at the time of wrapping the nodes.
Attachment #8906558 - Flags: review?(mak77) → review-
Comment hidden (mozreview-request)
Comment on attachment 8906558 [details]
Bug 1386513 - Handle cases of pasted bookmarks not being in the database, to fix copying bookmarks across browser instances.

Marco, I've updated the patch based around an instanceId idea. Can you take a look and see what you think please?

Ignore the test for now, that's the same as before so probably will need updating - once you're happy with the methods then I'll fixup the tests.
Attachment #8906558 - Flags: review?(mak77) → feedback?(mak77)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8906558 [details]
Bug 1386513 - Handle cases of pasted bookmarks not being in the database, to fix copying bookmarks across browser instances.

https://reviewboard.mozilla.org/r/178304/#review192786

Yes, it sounds far cheaper!

::: toolkit/components/places/PlacesUtils.jsm:432
(Diff revision 3)
>  
>    getString: function PU_getString(key) {
>      return bundle.GetStringFromName(key);
>    },
>  
> +  getInstanceId() {

I suppose you could use defineLazyGetter on a PlacesUtils.instanceId property. It doesn't need to be a method.

::: toolkit/components/places/PlacesUtils.jsm:436
(Diff revision 3)
>  
> +  getInstanceId() {
> +    if (!gInstanceId) {
> +      const {generateUUID} = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
> +      gInstanceId = generateUUID().toString().slice(1, -1);
> +    }

nit: since this string is added to every single node we wrap, to limit memory usage we could use something shorter, like PlacesUtils.history.makeGuid.
Attachment #8906558 - Flags: feedback?(mak77) → feedback+
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8906558 [details]
Bug 1386513 - Handle cases of pasted bookmarks not being in the database, to fix copying bookmarks across browser instances.

https://reviewboard.mozilla.org/r/178304/#review193186

::: browser/components/places/tests/browser/browser_paste_bookmarks.js:31
(Diff revision 4)
> +  info("Selecting BookmarksToolbar in the left pane");
> +  PlacesOrganizer.selectLeftPaneQuery("BookmarksToolbar");
> +
> +  bookmark = await PlacesUtils.bookmarks.insert({
> +    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
> +    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

nit: optional for bookmarked urls

::: browser/components/places/tests/browser/browser_paste_bookmarks.js:39
(Diff revision 4)
> +  });
> +  bookmarkId = await PlacesUtils.promiseItemId(bookmark.guid);
> +
> +  ContentTree.view.selectItems([bookmarkId]);
> +
> +  await promiseClipboard(() => {

I cannot check the browser.ini file because mozreview refuses to show it to me, just a note that this should be in the clipboard sub-harness.

::: browser/components/places/tests/browser/browser_paste_bookmarks.js:89
(Diff revision 4)
> +  xferable.addDataFlavor(PlacesUtils.TYPE_X_MOZ_PLACE);
> +  xferable.setTransferData(PlacesUtils.TYPE_X_MOZ_PLACE, PlacesUtils.toISupportsString(data),
> +                           data.length * 2);
> +
> +  let clipboard = Cc["@mozilla.org/widget/clipboard;1"]
> +                    .getService(Ci.nsIClipboard);

Services.clipboard

::: browser/components/places/tests/browser/browser_paste_bookmarks.js:106
(Diff revision 4)
> +               "Should have the correct title");
> +  Assert.equal(tree.children[0].uri, TEST_URL1,
> +               "Should have the correct URL");
> +
> +  await PlacesUtils.bookmarks.remove(tree.children[0].guid);
> +});

What about instead of a copy the test does a "cut", that should actually move the nodes in the first test, but "copy" them in the second test, right?
It sounds like it may be covering more code.

::: toolkit/components/places/PlacesUtils.jsm:2022
(Diff revision 4)
>           getService(Ci.nsIStringBundleService).
>           createBundle(PLACES_STRING_BUNDLE_URI);
>  });
>  
> +XPCOMUtils.defineLazyGetter(PlacesUtils, "instanceId", () => {
> +  return PlacesUtils.history.makeGuid();

nit: worth a brief comment about the fact this is just used as a reasonably-random value.
Attachment #8906558 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 19

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eac9a60e4edb
Handle cases of pasted bookmarks not being in the database, to fix copying bookmarks across browser instances. r=mak
https://hg.mozilla.org/mozilla-central/rev/eac9a60e4edb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Did this fix bug 1177654 as well?
Flags: needinfo?(standard8)
(In reply to Marco Bonardo [::mak] from comment #21)
> Did this fix bug 1177654 as well?

No, for reference it now says: Can't copy a container from a legacy-transactions build
Flags: needinfo?(standard8)

Comment 23

2 years ago
Will this fix facilitate a fix for bug 1177654?

Comment 24

2 years ago
Build ID: 20171017220415
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1393021
You need to log in before you can comment on or make changes to this bug.