457473 - Bookmark copy creates bookmark with duplicate guid - regression in FF 3.0.3

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Bill Wood]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

A regression bug has appeared in FF 3.0.3 in file modules\utils.js line 1331.

Previously a bookmark which was copied with drag and drop while holding the CTRL key down created a copy bookmark with a new guid.  In FF 3.0.3, the guid is copied with the other annotations creating two bookmarks with the same guid.

Line 1331 used to read (in FF 3.0.2):
            return anno.name != "placesInternal/GUID";

Now it reads:
            return true;


Reproducible: Always

Steps to Reproduce:
1. Go to the bookmarks library
2. Copy a bookmark using drag-and-drop while holding down the CTRL key
3. The new, copied bookmark has the same guid as the original bookmark
Actual Results:  
Copied bookmark has the same guid as the original.

Expected Results:  
Copied bookmark has its own unique guid.

I'm not sure why this change was made in FF 3.0.3, but I am under the impression that every bookmark should have a unique guid.  I am working on an extension that relies on unique guids.

Comment 1

[Matthias Versen [:Matti]]

The change is from bug 437078

Comment 2

[Bill Wood]

I figured it was something to do with Foxmarks.  However, the issue described in bug 437078 is a different use case and has created another bug, non-unique bookmark ids.  Can we fix bug 437078 without breaking guid uniqueness?

Comment 3

[Todd Agulnick]

We just discovered the same problem.

Serializing the guid is fine (so it can be restored if the user does a manual restore of a backup), but on paste the guid should be discarded.

If I had to chose between preserving the guid across backup/restore or sacrificing uniqueness, I'd lean slightly toward the former. So if a fix for this bug isn't tractable, I'd rather see the fix for bug 437078 backed out.

Comment 4

[Bill Wood]

Todd, I assume you meant the "latter" not the "former" in your comment?

Sacrificing uniqueness would pretty much negate the purpose of GUIDs and presumably cause errors in Foxmarks and other programs.

Comment 5

[Todd Agulnick]

(In reply to comment #4)
> Todd, I assume you meant the "latter" not the "former" in your comment?
> 
> Sacrificing uniqueness would pretty much negate the purpose of GUIDs and
> presumably cause errors in Foxmarks and other programs.

Whoops. Yes, what I wrote above doesn't make any sense logically.

What I meant to say:

I'd rather go back to losing GUID's on backup/restore, because not having uniqueness in GUID's is indeed a severe problem, for Foxmarks and (I would guess) others.

Thanks for catching that.

Comment 6

[steves06]

Note that this bug has broken my use of Foxmarks in a pretty big way, as I keep a good number of bookmarks as copies in different folders (cross-file the same bookmark in different subject areas).  Once I upgraded to FF 3.0.3 my bookmarks
now have big gaps, and different gaps depending on which machine I'm on as some have the copies intact and others do not (depending on where I was when I copied).  I'm not sure I'll ever be able to easily sort this out ... I'm hoping a fix to make the GUIDs unique again would somehow help me.

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: wip

wip patch to discard GUIDs on copy for all items. should be able test this via mochitest.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: with test

Comment 9

[Dietrich Ayala (:dietrich)]

note the commented out test: if an item is created via the transaction service, and then that transaction is undone and redone, the resulting item will have a GUID that is different from the originally created item. this is because GUIDs are lazily created, so the original transaction did not create a GUID to delete/recreate with undo/redo.

this only occurs for newly created items. this seemed a case both edgier, and far better to live with than a lack of uniqueness, so will address it in a followup bug.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 345997 [details] [diff] [review]
with test

diff --git a/browser/components/places/content/utils.js b/browser/components/places/content/utils.js
--- a/browser/components/places/content/utils.js
+++ b/browser/components/places/content/utils.js
@@ -207,11 +207,12 @@
     var itemTitle = aData.title;
     var keyword = aData.keyword || null;
     var annos = aData.annos || [];
-    if (aExcludeAnnotations) {
-      annos = annos.filter(function(aValue, aIndex, aArray) {
-        return aExcludeAnnotations.indexOf(aValue.name) == -1;
-      });
-    }
+    var excludeAnnos = ["placesInternal/GUID"];

Wouldn't be better defining a const for this anno name?
Also add comment as in other places to explain we exclude it when copying items


diff --git a/browser/components/places/tests/browser/browser_457473_no_copy_guid.js b/browser/components/places/tests/browser/browser_457473_no_copy_guid.js
new file mode 100644
--- /dev/null
+++ b/browser/components/places/tests/browser/browser_457473_no_copy_guid.js

+  var folderANode = testRootNode.getChild(0);
+  var oldGUIDs = getGUIDs(folderANode);
+
+  // test the test function
+  //ok(checkGUIDs(folderANode, oldGUIDs, true), "confirm guid test works");;

Should not this work? why is it commented? (and double ;)


+  // redo the transaction
+  // confirming GUIDs persist through undo/redo
+  PlacesUIUtils.ptm.redoTransaction();
+  is(testRootNode.childCount, 2, "confirm redo re-copied the folder");
+  folderBNode = testRootNode.getChild(1);
+  ok(checkGUIDs(folderBNode, oldGUIDs, false), "old guids after undo/redo don't match source guids"); // sanity check
+
+  // XXXdietrich fails since GUIDs are created lazily. the anno
+  // isn't present at the time the transaction is first executed,
+  // and undo just undoes the original transaction - doesn't pull
+  // in any new changes.
+  //ok(checkGUIDs(folderBNode, newGUIDs, true, true), "new guids after under/redo should match pre-undo/redo guids");

so, is the first check the opposite of this? i would put it after the correct commented-out one, actually they feels like different tests
and check for 4th param (remove it if not used), see below.

+function checkGUIDs(aFolderNode, aGUIDs, aShouldMatch, d) {

"d" is not much clear as a param name and not used

r=mak77

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

also please, report followup bug number

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: comments fixed

comments fixed, test cleaned up

Comment 13

[Asaf Romano (gone)]

Comment on attachment 346152 [details] [diff] [review]
comments fixed

r=mano

Comment 14

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/be0571e7681b

Comment 15

[Dietrich Ayala (:dietrich)]

branch drivers: see above comments from extension authors, this can cause problems for extensions that sync bookmarks across profiles.

Comment 16

[Dietrich Ayala (:dietrich)]

sorry, didn't mean to ask blocking, just a=.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 346152 [details] [diff] [review]
comments fixed

Time is short for this release, we don't have a lot of time for testing branch updates so we'd like a second review on this as a double-check.

>-      return true;
>-    }, this);
>+      // always exclude GUID when copying any item
>+      return aAnno.name != GUID_ANNO;
>+    });

You don't need the "this" argument anymore?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 346152 [details] [diff] [review]
comments fixed

The "this" argument isn't used, so shouldn't be a problem. Looks good otherwise, too.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 346152 [details] [diff] [review]
comments fixed

Thanks!

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 20

[Dietrich Ayala (:dietrich)]

checked in on branch.

Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.141; previous revision: 1.140
done
Checking in browser/components/places/tests/browser/Makefile.in;
/cvsroot/mozilla/browser/components/places/tests/browser/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/browser/components/places/tests/browser/browser_457473_no_copy_guid.js,v
done
Checking in browser/components/places/tests/browser/browser_457473_no_copy_guid.js;
/cvsroot/mozilla/browser/components/places/tests/browser/browser_457473_no_copy_guid.js,v  <--  browser_457473_no_copy_guid.js
initial revision: 1.1
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.23; previous revision: 1.22
done

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Verified that this is fixed in 1.9.0.5.