Avoid raw pointers in mozStorageAsyncStatementExecution.cpp

RESOLVED FIXED in Firefox -esr52

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks 1 bug)

55 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 fixed, firefox56 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

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.

https://reviewboard.mozilla.org/r/151652/#review156948

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.

Ownership-taking/freeing:
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 mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/58eabe7c225b
Avoid raw pointers in mozStorageAsyncStatementExecution.cpp. r=asuth
https://hg.mozilla.org/mozilla-central/rev/58eabe7c225b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1348955
Depends on: 1350752
[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 https://wiki.mozilla.org/Release_Management/ESR_Landing_Process 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.