1113071 - async transaction commit makes runInBatchMode non-atomic

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

the test was intermittently failing without those, but that's unexpected, yielding should do the right thing already.
Something in BookmarksHTMLUtils.jsm is not properly waiting

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: runInBatch should notify onEndUpdateBatch when the transaction has surely commited

The root cause of the failure of test_bookmarks_html.js is that BookmarkImporter.importFromURL did not wait for async commit in runInBatchMode.

This patch moves onEndUpdateBatch after the async commit has surely done. So BookmarkImporter.importFromURL can wait for the onEndUpdateBatch.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: BookmarkImporter.importFromURL should wait for onEndUpdateBatch.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Though those patches caused a lot of failures on try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=250a18ae0e23

Need to be investigated.

Comment 4

[Marco Bonardo [:mak]]

Oh good catch, indeed we made transactions commit async until we had new bookmarks API in shape.
the problem is that runInBatchMode is a synchronous API, so we can't just change the notification to be async and hope things will keep working.

Based on this, I think your promiseAsyncUpdates() fix is the right one, I don't think we can do any better for now. We'll re-evaluate once the new Bookmarks API is ready.

It's possible this issue is the root cause of other intermittent failures that begun after bug 1047811.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: transaction fix

This is independent from above patch series but I found this mistake while I am reading codes around places.

UpdateKeywordsHashForRemovedBookmark issues an async statement so transaction.Commit() should be moved after the statement.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8540032 [details] [diff] [review]
transaction fix

Review of attachment 8540032 [details] [diff] [review]:
-----------------------------------------------------------------

this transaction is not async, so its commit is synchronous.

only a few transactions in history are async.

Comment 7

[Marco Bonardo [:mak]]

We don't need this anymore for a couple reasons:
1. we're moving to async, so async commit is not something we're still interested into
2. we're removing the concept of batches (for now), an in any case we plan to change the notification system