Closed Bug 1375217 Opened 6 years ago Closed 6 years ago

Avoid raw pointers in mozStorageAsyncStatementExecution.cpp


(Toolkit :: Storage, enhancement, P1)

55 Branch



Tracking Status
firefox-esr52 --- fixed
firefox56 --- fixed


(Reporter: mak, Assigned: mak)


(Blocks 1 open bug)


(Whiteboard: [fxsearch])


(2 files)

We can likely use NewRunnableMethod here instead of creating our own runnable classes.
Comment on attachment 8880290 [details]
Bug 1375217 - Avoid raw pointers in mozStorageAsyncStatementExecution.cpp.

This is a nice cleanup!

::: storage/mozStorageAsyncStatementExecution.h:86
(Diff revision 1)
> +   * Used by notifyComplete(), notifyError() and notifyResults() to notify on
> +  *  the calling thread.
> +   */

nit: indentation

::: storage/mozStorageAsyncStatementExecution.cpp:365
(Diff revision 1)
> +  // Hold a strong reference to the callback while notifying it, so that if
> +  // it spins the event loop, the callback won't be released and freed out
> +  // from under us.
> +  // Additionally, take ownership of it, so it gets released on this thread.

I suggest increasing the specificity of this comment and its clones. Specifically, 2 variants:

Callback-using-but-not-freeing (notifyResults):
Acquire our own strong reference so that if the callback spins a nested event loop and notifyCompleteOnCallingThread/notifyErrorOnCallingThread are executed, forgetting mCallback, we still have a valid/strong reference that won't be freed until we exit.

Take ownership of mCallback and responsibility for freeing it when we release it.  Any notifyResultsOnCallingThread calls on the stack spinning the event loop have guaranteed their safety by creating their own strong reference before invoking the callback.
Attachment #8880290 - Flags: review?(bugmail) → review+
yeah, I just noticed my comments were bogus. I fixed the code but not the comments.

notifyError uses the callback, but doesn't take ownership nor free it, the only one taking responsibility to free the callback is rightly the complete callback. indeed we notify completion after errors/results.

I'll update all comments accordingly.
sigh, notifyError takes ownership but not responsibility to free the callback. btw, updated patch incoming.
Pushed by
Avoid raw pointers in mozStorageAsyncStatementExecution.cpp. r=asuth
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1348955
Depends on: 1350752
Attached patch patch for ESR52Splinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It fixes Bug 1348955 that is a sec-high.
User impact if declined: Bug 1348955
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): not particularly risky
String or UUID changes made by this patch:  none

See for more info.
Attachment #8948026 - Flags: approval-mozilla-esr52?
Note, the patch also includes bug 1350752.
Comment on attachment 8948026 [details] [diff] [review]
patch for ESR52

Approved for ESR 52.7, thanks for the rebased patch.
Attachment #8948026 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.