Closed Bug 1386513 Opened 4 years ago Closed 4 years ago

Copy & paste Bookmark will fail between 2 instance of browser

Categories

(Toolkit :: Places, defect, P2)

56 Branch
Unspecified
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- disabled
firefox58 --- verified

People

(Reporter: alice0775, Assigned: standard8)

References

(Depends on 1 open bug)

Details

(Keywords: regression, reproducible, ux-consistency, Whiteboard: [parity-chrome][fxsearch])

Attachments

(1 file)

+++ 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.
Version: 41 Branch → 56 Branch
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]
Prior to land Bug 1071513, it was broken only folder.
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)
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 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 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 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 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+
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
Closed: 4 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)
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.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1393021
You need to log in before you can comment on or make changes to this bug.