1389716 - `Bookmarks.reorder` shouldn't call `withConnectionWrapper` from within a transaction

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement)

Description

[Lina Butler [:lina]]

Far-fetched theory for bug 1387782: `reorderChildren` immediately calls `db.executeTransaction`, which calls `fetchBookmarksByParent`, which calls `withConnectionWrapper` again. It's not clear to me if that's actually a problem, though: transactions are deferred by default, and `fetchBookmarksByParent` only reads. I wonder if other concurrently executing statements could affect this, or why it wasn't an issue before. Sync switched to using `Bookmarks.reorder` in 51, so I'd have expected this to come up earlier. We did switch to using `PlacesUtils.withConnectionWrapper` instead of `promiseDBConnection` in more places (bug 1382363), which might have surfaced this bug.

FWIW, a simple test like `PlacesUtils.withConnectionWrapper("Outer connection", async function(db) { await db.executeTransaction(() => { await PlacesUtils.withConnectionWrapper("Inner connection", function(db) { return db.executeCached(`SELECT 1`); }); await db.executeCached(`UPDATE moz_bookmarks SET lastModified = :lastModified WHERE id = 1`, { lastModified: PlacesUtils.toPRTime(new Date()) }); }); }).then(() => console.log("Finished both"))` works without hanging.

Fixing `fetchBookmarksByParent` to take an existing connection is easy enough, so let's see if it helps. If so, we can close bug 1387782; if not, we'll need to investigate further.

Comment 1

[Lina Butler [:lina]]

Attachment: Bug 1389716 - `Bookmarks.reorder` shouldn't call `withConnectionWrapper` from within a transaction.

Review commit: https://reviewboard.mozilla.org/r/167782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167782/

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8896527 [details]
Bug 1389716 - `Bookmarks.reorder` shouldn't call `withConnectionWrapper` from within a transaction.

https://reviewboard.mozilla.org/r/167782/#review173308

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I don't know if this helps, but it's surely right.
async shutdown is conceptually nice, but it opens a big can of worms where a promise may depend on another one that depends on the former one, and basically we end up in an deadlock. I'd like if there was an easy solution to avoid these mistakes, but I can't see one.

Comment 4

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Error accessing https://api.pub.build.mozilla.org/treestatus/trees/autoland :
remote: HTTP Error 503: SERVICE UNAVAILABLE
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.a_treeclosure hook failed
abort: push failed on remote

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/926ca8e8972e
`Bookmarks.reorder` shouldn't call `withConnectionWrapper` from within a transaction. r=mak

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/926ca8e8972e

Comment 7

[Ronan Jouchet]

Kit, Mark, looks like the fix attempt at bug 1389716 didn't solve my bug 1387782.
Today's crash: https://crash-stats.mozilla.com/report/index/588463bd-7373-4b19-8cf7-5badd0170817

Feel free to ask for more info, or even a temporary remote access to my machine.