There are various tests in browser/ that assume places UI commands are synchronous (e.g. the test for cmd_cut). These tests would fail, as they are, with async. transactions enabled. We need to fix those before we can enable async transactions

Mano, can you please estimate this?

FTR, please let's review and land the patch in bug 1094864 (to be posted) before continuing here.

1. Ignore firefox.js and browser.ini changes 2. I'll do browser_bookmarkProperties in a separate patch (in this bug) 3. browser_library_commands won't pass because a query node created there has no bookmarkGuid for some reason. I'll file a separate bug sson.

Comment on attachment 8578527 [details] [diff] [review] everything but browser_bookmarkProperties and browser_library_commands Review of attachment 8578527 [details] [diff] [review]: ----------------------------------------------------------------- It looks ok, so far. ::: browser/components/places/tests/browser/browser_475045.js @@ +40,5 @@ > ChromeUtils.synthesizeDrop(placesItems.childNodes[0], > + placesItems, > + [[{ type: aMimeType, > + data: uriSpec}]], > + aEffect, window); while here, please remove trailing spaces and fix indentation @@ +42,5 @@ > + [[{ type: aMimeType, > + data: uriSpec}]], > + aEffect, window); > + > + yield window.PlacesControllerDragHelper._onDropPromise; indentation @@ +47,3 @@ > > // Verify that the drop produces exactly one bookmark. > + let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(uri); Please use the new API in tests @@ +52,1 @@ > PlacesUtils.bookmarks.removeItem(bookmarkIds[0]); ditto @@ +52,4 @@ > PlacesUtils.bookmarks.removeItem(bookmarkIds[0]); > > // Verify that we removed the bookmark successfully. > ok(!PlacesUtils.bookmarks.isBookmarked(uri), "URI should be removed"); ditto @@ +57,1 @@ > while here, please remove the trailing spaces ::: browser/components/places/tests/browser/browser_library_batch_delete.js @@ +57,5 @@ > + > + // doCommand may do async work through PT, so we need to serialize the rest > + // of this test. > + PlacesTransactions.batch(function* () { > + ok(!PlacesUtils.bookmarks.isBookmarked(testURI), use the new API please ::: browser/components/places/tests/browser/browser_library_commands.js @@ +130,5 @@ > + > + // doCommand may do async work through PT, so we need to serialize the rest > + // of this test with PT. > + yield PlacesTransactions.batch(() => {}); > + yield promiseItemRemoved; trailing space

Mano, are you still interest in finish this patch? Or I can help finish that.

Mano is unlikely to continue the work on this, I suspect he's quite busy with his new job. So feel free to take over. I will unassign his Places bugs so it's clear they are available.

Note: the last test has been split out to bug 1376925, so that we can get the intermediate work landed.

Comment on attachment 8881936 [details] Bug 1119282 - Update browser_library_batch_delete.js to use more modern test constructs. https://reviewboard.mozilla.org/r/153010/#review158234

Comment on attachment 8881937 [details] Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. https://reviewboard.mozilla.org/r/153012/#review158246 ::: browser/components/places/tests/browser/browser_475045.js:52 (Diff revision 1) > > + await promiseItemAddedNotification; > + > // Verify that the drop produces exactly one bookmark. > - let bookmarkIds = PlacesUtils.bookmarks > - .getBookmarkIdsForURI(uri); > + let bookmark = await PlacesUtils.bookmarks.fetch({url}); > + Assert.equal(typeof(bookmark), "object", "There should be exactly one bookmark"); why so complex, it's either an object or null, so just Assert.ok(bookmark) should do. if you want to heck that it's only one, you should use the fetch callback to collect all the bookmarks in an array. to be clear, fetch always returns just null or the most recent bookmarks, never an array.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ca16ed38806f Update browser_library_batch_delete.js to use more modern test constructs. r=mak https://hg.mozilla.org/integration/autoland/rev/5659f6c9a7c5 Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. r=mak

Backed out for test_engine_changes_during_sync.js, at least on Windows 7 VM: https://hg.mozilla.org/integration/autoland/rev/b0b36bfe7d8889bfe80217ffcf15e953dca8d8b2 https://hg.mozilla.org/integration/autoland/rev/2ed4887d5010ed74204a4e34003d9e410923371d Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5659f6c9a7c5b31929bdda8cd58a2001356190c0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110573003&repo=autoland 18:27:36 INFO - TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 343] true == true 18:27:36 INFO - TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 344] "undefined" == "undefined" 18:27:36 INFO - TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 351] "undefined" == "undefined" 18:27:36 INFO - TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 352] "undefined" != 1 18:27:36 INFO - TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 343] true == true 18:27:36 INFO - TEST-PASS | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 344] "undefined" == "undefined" 18:27:36 WARNING - TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_engine_changes_during_sync.js | test_bookmark_change_during_sync - [test_bookmark_change_during_sync : 346] "undefined" == [{"name":"clientCycles","count":1}] 18:27:36 INFO - c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/head_helpers.js:assert_success_ping/<:346 18:27:36 INFO - c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/head_helpers.js:assert_success_ping:338 18:27:36 INFO - c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_engine_changes_during_sync.js:test_bookmark_change_during_sync/<:421 18:27:36 INFO - c:/slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_engine_changes_during_sync.js:test_bookmark_change_during_sync:420 18:27:36 INFO - exiting test

Comment on attachment 8881937 [details] Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. https://reviewboard.mozilla.org/r/153012/#review158428 ::: toolkit/components/places/PlacesTransactions.jsm:1226 (Diff revision 2) > if (newParentId == oldParentId && oldIndex > undoIndex) > PlacesUtils.bookmarks.moveItem(itemId, oldParentId, oldIndex + 1); > else > PlacesUtils.bookmarks.moveItem(itemId, oldParentId, oldIndex); > }; > + return aGuid; This is unrelated to the failure. I just noticed that this doesn't see to make sense, the guid is already known, so there's no reason Move should return a guid.

Comment on attachment 8881937 [details] Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. https://reviewboard.mozilla.org/r/153012/#review158484 ::: toolkit/components/places/nsNavHistory.cpp:3979 (Diff revision 2) > } > else { > // This is a regular query. > resultNode = new nsNavHistoryQueryResultNode(aTitle, aTime, queries, options); > resultNode->mItemId = itemId; > + resultNode->mBookmarkGuid = aBookmarkGuid; It looks like this fixes a bug where `bookmarkGuid` would be erroneously empty? I wonder if the sync bookmark validator is expanding queries incorrectly, and this change uncovered that... http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/services/sync/modules/bookmark_validator.js#232-279

Comment on attachment 8882227 [details] Bug 1119282 - Fix the sync bookmark validator cycle detection to correctly get folder contents. https://reviewboard.mozilla.org/r/153330/#review158532

Comment on attachment 8881937 [details] Bug 1119282 - Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. https://reviewboard.mozilla.org/r/153012/#review158428 > This is unrelated to the failure. I just noticed that this doesn't see to make sense, the guid is already known, so there's no reason Move should return a guid. Discussed this on irc and decided to leave it in there - the caller needs a transaction returning the guid to work nicely alongside the copy function.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4e7837d88d8b Update browser_library_batch_delete.js to use more modern test constructs. r=mak https://hg.mozilla.org/integration/autoland/rev/107b93dffe70 Update most browser/components/places browser mochitests to pass with async Places transations and fix some async transaction issues. r=mak https://hg.mozilla.org/integration/autoland/rev/d05bb3f0b142 Fix the sync bookmark validator cycle detection to correctly get folder contents. r=kitcambridge

https://hg.mozilla.org/mozilla-central/rev/4e7837d88d8b https://hg.mozilla.org/mozilla-central/rev/107b93dffe70 https://hg.mozilla.org/mozilla-central/rev/d05bb3f0b142