Open Bug 1387775 Opened 7 years ago Updated 2 years ago

Sqlite.jsm `onRow` handler swallows exceptions

Categories

(Toolkit :: Storage, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox57 --- fix-optional

People

(Reporter: lina, Unassigned, Mentored)

Details

(Whiteboard: [lang=cpp][good next bug])

I spent some time debugging why the tree structure I was building from a Places query was incomplete. It turns out one of the methods I was calling from the `onRow` callback had a bug, causing it to throw. (There are cases where I still want that method to throw, but this wasn't one of them). I didn't have SQLite logging turned on, so I missed the "Exception when calling onRow callback" logging: https://searchfox.org/mozilla-central/rev/39ccebaf1890f8731d534b5eb3818b66d2b25221/toolkit/modules/Sqlite.jsm#778-780,787

IIUC, removing the `try...catch` isn't enough. XPCOM will convert the exception into a failed `nsresult`, but https://searchfox.org/mozilla-central/rev/39ccebaf1890f8731d534b5eb3818b66d2b25221/storage/mozStorageAsyncStatementExecution.cpp#441,450,452 ignores it.

I wonder if we could have `notifyResultsOnCallingThread` call `notifyErrorOnCallingThread`, cancel the statement, and eventually call `handleCompletion` with `REASON_ERROR`.

Or, if it's not worth the trouble, I can remove the `onRow` handler from my code and iterate over the array afterward.
Now that we're targeting 57 and extensions can no longer access mozStorage, this sounds like a win with no downsides.  I'm certainly on board with having all callbacks propagate errors forward.  The main thing is to keep the error propagation exclusively on the owning/main thread (so we don't have to synchronize with I/O on the async thread and can otherwise maintain correctness).
Mentor: mak
Whiteboard: [lang=cpp][good next bug]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.