Closed Bug 1113071 Opened 5 years ago Closed 2 years ago

async transaction commit makes runInBatchMode non-atomic


(Toolkit :: Places, defect, P3)






(Reporter: mak, Unassigned)




(3 files)

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
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.
Attachment #8540013 - Flags: feedback?(mak77)
Though those patches caused a lot of failures on try.

Need to be investigated.
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.
Blocks: 1079387, 1071357
No longer depends on: 1079387
Summary: test_bookmarks_html should not need to promiseAsyncUpdates → async transaction commit makes runInBatchMode non-atomic
Attachment #8540013 - Flags: feedback?(mak77)
Attachment #8540014 - Flags: feedback?(mak77)
Blocks: 1047811
Attached patch transaction fixSplinter Review
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.
Attachment #8540032 - Flags: review?(mak77)
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.
Attachment #8540032 - Flags: review?(mak77) → review-
Priority: -- → P3
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
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.