Closed Bug 457473 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: WPWoodJr+Bugzilla, Assigned: dietrich)

References

Details

(Keywords: regression, verified1.9.0.5)

Attachments

(1 file, 2 obsolete files)

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.
The change is from bug 437078
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?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
(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.
Component: Bookmarks & History → Places
OS: Windows XP → All
Priority: -- → P1
Product: Firefox → Toolkit
QA Contact: bookmarks → places
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1b2
Version: unspecified → Trunk
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.
Attached patch wip (obsolete) — Splinter Review
wip patch to discard GUIDs on copy for all items. should be able test this via mochitest.
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
Attached patch with test (obsolete) — Splinter Review
Attachment #345977 - Attachment is obsolete: true
Attachment #345997 - Flags: review?(mak77)
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 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
Attachment #345997 - Flags: review?(mak77) → review+
also please, report followup bug number
Attached patch comments fixedSplinter Review
comments fixed, test cleaned up
Attachment #345997 - Attachment is obsolete: true
Attachment #346152 - Flags: review?(mano)
http://hg.mozilla.org/mozilla-central/rev/be0571e7681b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
branch drivers: see above comments from extension authors, this can cause problems for extensions that sync bookmarks across profiles.
Flags: blocking1.9.0.5?
sorry, didn't mean to ask blocking, just a=.
Flags: blocking1.9.0.5?
Attachment #346152 - Flags: approval1.9.0.5?
Blocks: 437078
Keywords: regression
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?
Attachment #346152 - Flags: superreview?(gavin.sharp)
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.
Attachment #346152 - Flags: superreview?(gavin.sharp) → superreview+
Comment on attachment 346152 [details] [diff] [review]
comments fixed

Thanks!

Approved for 1.9.0.5, a=dveditz for release-drivers
Attachment #346152 - Flags: approval1.9.0.5? → approval1.9.0.5+
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
Keywords: fixed1.9.0.5
Verified that this is fixed in 1.9.0.5.
You need to log in before you can comment on or make changes to this bug.