Mir

Merge lp://qastaging/~albaguirre/mir/fix-1433330 into lp://qastaging/mir

Proposed by Alberto Aguirre
Status: Merged
Approved by: Alberto Aguirre
Approved revision: no longer in the source branch.
Merged at revision: 2406
Proposed branch: lp://qastaging/~albaguirre/mir/fix-1433330
Merge into: lp://qastaging/mir
Diff against target: 151 lines (+45/-18)
4 files modified
cmake/MirCommon.cmake (+0/-1)
src/server/frontend/protobuf_message_processor.cpp (+42/-15)
src/server/frontend/protobuf_message_processor.h (+2/-1)
tests/unit-tests/frontend/test_protobuf_message_processor.cpp (+1/-1)
To merge this branch: bzr merge lp://qastaging/~albaguirre/mir/fix-1433330
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Kevin DuBois (community) Approve
Alexandros Frantzis (community) Approve
Alan Griffiths Approve
Review via email: mp+253285@code.qastaging.launchpad.net

Commit message

Fix calling dead objects from some protobuf closure objects.

Since the closure/callback object may be invoked by a different thread in some instances (e.g. SessionMediator::exchange_buffer), the closure object now gets a weak_ptr to the ProtobufMessageProcessor (instead of a raw pointer).

Description of the change

Fix calling dead objects from some protobuf closure objects.

Since the closure/callback object may be invoked by a different thread in some instances (e.g. SessionMediator::exchange_buffer), the closure object now gets a weak_ptr to the ProtobufMessageProcessor (instead of a raw pointer).

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

29 + callback();
30 + delete this;

We should ensure that 'this' is deleted even if callback() throws.

Looks good otherwise.

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

20 +class SelfDeletingCallback : public google::protobuf::Closure {
21 +public:
22 +
23 + SelfDeletingCallback(std::function<void()> const& callback)
24 + : callback(callback)
25 + {
26 + }
27 +
28 + void Run() {
29 + callback();
30 + delete this;
31 + }
32 +
33 + private:
34 + SelfDeletingCallback(SelfDeletingCallback&) = delete;
35 + void operator=(const SelfDeletingCallback&) = delete;
36 + std::function<void()> callback;
37 +};

Why is this needed? google::protobuf::NewCallback<> deletes itself. (NewPermanentCallback<> is the version that doesn't).

And as Alexandros says, callbacks could throw.

review: Needs Fixing
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> 20 +class SelfDeletingCallback : public google::protobuf::Closure {
> 21 +public:
> 22 +
> 23 + SelfDeletingCallback(std::function<void()> const& callback)
> 24 + : callback(callback)
> 25 + {
> 26 + }
> 27 +
> 28 + void Run() {
> 29 + callback();
> 30 + delete this;
> 31 + }
> 32 +
> 33 + private:
> 34 + SelfDeletingCallback(SelfDeletingCallback&) = delete;
> 35 + void operator=(const SelfDeletingCallback&) = delete;
> 36 + std::function<void()> callback;
> 37 +};
>
> Why is this needed? google::protobuf::NewCallback<> deletes itself.
> (NewPermanentCallback<> is the version that doesn't).

The code in question used to use google::protobuf::NewCallback<>. I need to be able to promote a weak_ptr within Run() so I can't use it as is. I don't want to change any Protobuf related code hence SelfDeletingCallback.

> And as Alexandros says, callbacks could throw.
OK.

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> 29 + callback();
> 30 + delete this;
>
> We should ensure that 'this' is deleted even if callback() throws.
>
> Looks good otherwise.

Will fix.
Note all ::google::protobuf::Closure implementations have the same issue.

Revision history for this message
Kevin DuBois (kdub) wrote :

20 +class SelfDeletingCallback : public google::protobuf::Closure {
indentation

28 + void Run()
override?

also, something like this would unintuitively double-free:
{
  auto c = std::make_unique<SelfDeletingCallback>();
  c->Run();
}
maybe make ~SelfDeletingCallback() private to avoid.

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

OK

review: Approve
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Looks good.

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

lgtm too

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

^---Ummm, can't replicate locally and can't tell if it is related to this MP. Let's try re-triggering a build.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches