1375217 - Avoid raw pointers in mozStorageAsyncStatementExecution.cpp

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, enhancement, P1)

Description

[Marco Bonardo [:mak]]

We can likely use NewRunnableMethod here instead of creating our own runnable classes.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1375217 - Avoid raw pointers in mozStorageAsyncStatementExecution.cpp.

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

Comment 2

[Marco Bonardo [:mak]]

A Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26615ec2799dc2cb8ec5c2a29b9ee9530f27b718

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Marco Bonardo [:mak]]

sigh, notifyError takes ownership but not responsibility to free the callback. btw, updated patch incoming.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8880290 [details]
Bug 1375217 - Avoid raw pointers in mozStorageAsyncStatementExecution.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151652/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58eabe7c225b
Avoid raw pointers in mozStorageAsyncStatementExecution.cpp. r=asuth

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58eabe7c225b

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch for ESR52

[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.

Comment 10

[Marco Bonardo [:mak]]

Note, the patch also includes bug 1350752.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8948026 [details] [diff] [review]
patch for ESR52

Approved for ESR 52.7, thanks for the rebased patch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/00fc630c9a46