`RemoveFolderTransaction::UndoTransaction` passes an empty GUID to `onItemAdded`

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Places
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

`undoTransaction` currently reinserts the folder with the same ID, but a different GUID: http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/toolkit/components/places/nsNavBookmarks.h#394-396 It's not clear to me if that's another bug, and it should actually use the same GUID, but the existing behavior has a subtle bug.

http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/toolkit/components/places/nsNavBookmarks.cpp#407-408 only fetches the ID and GUID if *_itemId == -1, which isn't true if we're reinserting. This fires `onItemAdded` with an empty GUID, confusing the GuidHelper's observer (http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/toolkit/components/places/PlacesUtils.jsm#2512-2513).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8792071 [details]
Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers.

https://reviewboard.mozilla.org/r/79298/#review79402

::: toolkit/components/places/nsNavBookmarks.cpp:415
(Diff revision 2)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (*_itemId == -1) {
>      // Get the newly inserted item id and GUID.
>      nsCOMPtr<mozIStorageStatement> lastInsertIdStmt = mDB->GetStatement(
>        "SELECT id, guid "

looks like there's no more need to fetch the guid here.

Actually, for History I went a little bit further (with the idea one day to do the same for bookmarks) and implemented the sql function store_last_inserted_id.

it could be easily extended using the CREATE_BOOKMARKS_FOREIGNCOUNT_AFTERINSERT_TRIGGER, so that on every INSERT we store the last bookmark id, and then here we could avoid a SELECT query completley and use sLastInsertedBookmarkId;

You could clearly either finally fix that, or ignore me, it's not critical but may be a good thing to do.

::: toolkit/components/places/tests/bookmarks/test_1303405.js:26
(Diff revision 2)
> +    folderId,
> +    uri("http://getthunderbird.com"),
> +    PlacesUtils.bookmarks.DEFAULT_INDEX,
> +    "Get Thunderbird!"
> +  );
> +  let tbGuid = yield PlacesUtils.promiseItemGuid(tbId);

Afaict, you can use the new API here, indeed I'd suggest that. We should not use the old API in new code unless it's critically needed.
We already have enough consumers to convert :)

::: toolkit/components/places/tests/bookmarks/xpcshell.ini:30
(Diff revision 2)
>  [test_818587_compress-bookmarks-backups.js]
>  [test_818593-store-backup-metadata.js]
>  [test_992901-backup-unsorted-hierarchy.js]
>  [test_997030-bookmarks-html-encode.js]
>  [test_1129529.js]
> +[test_1303405.js]

if possible, please give tests a meaningful name. Adding the bug# is optional, we decided some time ago to stop naming tests after bug# cause it complicates finding tests relevant to something.

Comment 4

2 years ago
mozreview-review
Comment on attachment 8792071 [details]
Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers.

https://reviewboard.mozilla.org/r/79298/#review79408

I'd like to have a very quick look at a revised patch. The lastInsert bits are optional, but bonu points if implemented.
Attachment #8792071 - Flags: review?(mak77)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8792071 [details]
Bug 1303405 - Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers.

https://reviewboard.mozilla.org/r/79298/#review79504

it looks great!

::: toolkit/components/places/nsNavBookmarks.h:218
(Diff revision 3)
>  
>    static const int32_t kGetChildrenIndex_Guid;
>    static const int32_t kGetChildrenIndex_Position;
>    static const int32_t kGetChildrenIndex_Type;
>    static const int32_t kGetChildrenIndex_PlaceID;
> +  static mozilla::Atomic<int64_t> sLastInsertedItemId;

nit: Add newline before it, since it's not related to the previous properties.

::: toolkit/components/places/nsNavBookmarks.h:220
(Diff revision 3)
>    static const int32_t kGetChildrenIndex_Position;
>    static const int32_t kGetChildrenIndex_Type;
>    static const int32_t kGetChildrenIndex_PlaceID;
> +  static mozilla::Atomic<int64_t> sLastInsertedItemId;
> +
> +  static void StoreLastInsertedId(const nsACString& aTable,

nit: while I don't care much abour a newline here, since the 2 are strictly related

::: toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js:14
(Diff revision 3)
> +    parentGuid: PlacesUtils.bookmarks.menuGuid,
> +    title: "Test folder",
> +  });
> +  let folderId = yield PlacesUtils.promiseItemId(folder.guid);
> +  let fx = yield PlacesUtils.bookmarks.insert({
> +    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

nit: when url is defined, type is optional (url is enough to know it's a bookmark)

::: toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js:21
(Diff revision 3)
> +    title: "Get Firefox!",
> +    url: "http://getfirefox.com",
> +  });
> +  let fxId = yield PlacesUtils.promiseItemId(fx.guid);
> +  let tb = yield PlacesUtils.bookmarks.insert({
> +    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

ditto

::: toolkit/components/places/tests/bookmarks/test_removeFolderTransaction_reinsert.js:42
(Diff revision 3)
> +      notifications.push(["onItemAdded", itemId, parentId, guid, parentGuid]);
> +    },
> +    onItemRemoved(itemId, parentId, index, type, uri, guid, parentGuid) {
> +      notifications.push(["onItemRemoved", itemId, parentId, guid, parentGuid]);
> +    },
> +  }, false);

somewhere this observer should be removed...
Attachment #8792071 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2146f5e5c5df
Ensure `RemoveFolderTransaction::UndoTransaction` passes the reinserted GUID to observers. r=mak

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2146f5e5c5df
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.