Closed Bug 1580270 Opened 5 years ago Closed 5 years ago

Remove `Async.jankYielder` from bookmarks mirror transactions

Categories

(Firefox :: Sync, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 71
Tracking Status
firefox69 --- disabled
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: lina, Assigned: lina)

Details

Attachments

(1 file)

Async.jankYielder can yield for long periods of time, sometimes over a minute—see bug 1478918, comment 5 and bug 1472963, comment 8. When used inside of a transaction, it blocks all other transactions on that connection, and can cause a shutdown hang if the transaction is in an executeBeforeShutdown block.

We still have one more use of it in SyncedBookmarksMirror#store. All other uses in the mirror are for reads, which are safer, and have a 5-minute watchdog timer.

This might mean that large syncs will become janky; see bug 1440334 for why we added this in the first place. That said, I think some jank on a large first sync is a slightly better outcome than wedging the connection, and having bookmarks and history just...not work for a few dozen seconds or minutes.

If we wanted to get fancy, we could have adaptive chunking, where we run the transaction in a requestIdleCallback, count how many records we can insert in the timeRemaining(), and set the size of the next chunk based on that. Or, more simply, we could check the current timestamp after inserting, keep going as long as it's under the deadline, then call requestIdleCallback again as soon as we hit the deadline. But, given that we eventually want to move to Rust Sync—which would run on the background thread and avoid this issue entirely—and most users have small bookmark libraries, I don't think it's worth exploring this until we see it's a problem.

Async.jankYielder can yield for long periods of time. When used in a
Sqlite.jsm transaction, it blocks all other transactions on the
connection, and can cause a shutdown hang if the transaction is in an
executeBeforeShutdown block.

This commit removes jankYielder from SyncedBookmarksMirror::store,
and also makes store abortable.

Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ff648dfb55b
Remove `Async.jankYielder` from bookmarks mirror transactions. r=markh
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: