Fix error callback life-time for ref-counted mojo clients |
||||
Issue descriptionDuring any mojo callback (either the success or the error callbacks) the client could delete its mojo bindings. This raises questions about the life-time of ref-counted clients where the mojo callbacks hold the only references to that client [1]. Both the success / error mojo callbacks should be held alive during the entire duration of the callback for such clients. This allows them to delete their mojo bindings safely if required. Issues: The success callback is currently held on the stack in mojo in all cases - when directly invoked, and when the error callback is invoked. That looks good. However, the error callback is not held on the local stack. Fix: The error callback should be on the local stack while it is being invoked, e.g., stash it in a local variable and then invoke it. Result: Mojo callbacks are never deleted while being invoked. This makes life-time simpler to reason about for all mojo users. [1] https://codereview.chromium.org/2705613003/#msg78
,
Mar 2 2017
>Is there something you think we can do in the bindings to make it
easier for uses to avoid this kind of mistake?
Yes. Where mojo is about to invoke the error callback, save the callback on the stack locally first, then invoke it as you do now.
base::Callback error_callback = mojo_error_callback_;
if (!mojo_error_callback_.is_null())
mojo_error_callback_.Run();
The retains at least one reference to ref-counted clients bound to the callback while the callback is being invoked.
,
Mar 2 2017
First of all, what is an "error" callback? I don't think we have used such a term in the mojo bindings. Do you mean the connection error handler callback?
,
Mar 2 2017
Yes, the connection error handler callback.
,
Mar 3 2017
,
Mar 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/477d1f7294ce5a7b13ce399ca4da899b667547a5 commit 477d1f7294ce5a7b13ce399ca4da899b667547a5 Author: yzshen <yzshen@chromium.org> Date: Fri Mar 03 17:52:51 2017 Mojo C++ bindings: keep connection error handler on the stack while running it. Otherwise, if the error handler owns the interface endpoint, resetting the interface endpoint in the error handler will destroy the error handler and the interface endpoint it owns, which leads to crash. BUG= 697728 Review-Url: https://codereview.chromium.org/2729133004 Cr-Commit-Position: refs/heads/master@{#454618} [modify] https://crrev.com/477d1f7294ce5a7b13ce399ca4da899b667547a5/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc [modify] https://crrev.com/477d1f7294ce5a7b13ce399ca4da899b667547a5/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc
,
Mar 3 2017
,
Mar 4 2017
Thank you, looks good to me. |
||||
►
Sign in to add a comment |
||||
Comment 1 by roc...@chromium.org
, Mar 2 2017