+++ 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.

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

According to Bug 1177654 this was broken already? Or were only folders broken before?

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

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.

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

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.

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

nice to have, but not a feature blocker.

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.

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.

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.

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.

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

Did this fix bug 1177654 as well?

(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

Will this fix facilitate a fix for bug 1177654?

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.