@Mupasa reported, see http://forums.mozillazine.org/viewtopic.php?p=14761724#p14761724 Reproducible: always Steps To Reproduce: 1. Delete, Add or Edit any bookmarks 2. Right click on any bookmarks and choose Properties 3. Cancel the dialog Actual Results: The previously edited bookmarks are unexpectedly undone. Expected Results: Do nothing 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

[Tracking Requested - why for this release]: dataloss

More detailed STR: 1) Display the bookmarks toolbar. 2) Add bookmarks to the toolbar if necessary. 3) Right-click on a bookmark 4) Select Delete -> Bookmark is deleted 5) Right-click on a different bookmark 6) Select Properties. 7) Select `Cancel` on the Bookmark Properties dialog. Expected Results: -> Nothing changes at this stage Actual Results -> The bookmark deleted at step 4 comes back again.

The steps in comment 2 also work with the sidebar as well the toolbar. They might also work on Windows/Linux in the Bookmarks menu.

I've just realised the important part here, is that in comment 2, there are no actions between steps 6 and 7 - i.e. the properties dialog doesn't have anything changed. What is happening is that the properties dialog is shown, so it sets up a new batch mode, when it gets cancelled, it ends the batch, and then assumes it can undo it. However, the batch is empty, so doesn't actually exist, and it ends up undoing the previous action - the delete of the bookmark. I suspect the old transaction code would have stored an entry regardless, which is why it wasn't an issue there. However, I think it would be better if the dialog didn't call undo if no actions have taken place. I think we can keep a record of the top undo entry when starting the batch, and see if it is the same or not after the batch has finished.

The previous code indeed took care of this and had a test http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/components/places/tests/unit/test_placesTxn.js#132 and it was managed like this http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/toolkit/components/places/PlacesUtils.jsm#638

Comment on attachment 8900686 [details] Bug 1391393 - Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. https://reviewboard.mozilla.org/r/172124/#review177484 ::: browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js:61 (Diff revision 1) > + await BrowserTestUtils.waitForCondition(() => !acceptButton.disabled, > + "The accept button should be enabled"); > + } > + ); > + > + Assert.ok(PlacesTransactions.undo.notCalled, "undo should not have been called"); may we also check the bookmark we removed is not there? ::: browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js:83 (Diff revision 1) > + "The accept button should be enabled"); > + > + let promiseKeywordNotification = PlacesTestUtils.waitForNotification( > + "onItemChanged", (itemId, prop, isAnno, val) => prop == "keyword" && val == "kw"); > + > + fillBookmarkTextField("editBMPanel_keywordField", "kw", dialogWin); I'd prefer if you'd change the title, keywords may change in the future, the title is unlikely.

Comment on attachment 8902440 [details] Bug 1391393 - When waiting on notifications, ensure async updates have completed before moving on. Bah, these browser places tests really don't want to play nice - I've now got a memory leak showing up on one of them.

Bug 1395994 fixes the memory leak.

Try build with bug 1395994 and the updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e124d6f0244f080e2b87e5217d9fe64f3b547865 The updated patch includes a couple of test-only tweaks to ensure tests correctly pass and don't hit intermittent issues.

One more minor update to skip the test when async transactions are turned off.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4ef718d6269b Cancelling the bookmarks properties dialog shouldn't undo when no changes have been made. r=mak

Build ID: 20171016220427 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.

Also verified that this issue is fixed using Firefox 57 beta 10 across platforms (Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 32bit).