Merge lp://staging/~ted/ubuntu-app-launch/lp1495871-unref-context into lp://staging/ubuntu-app-launch/16.04

Proposed by Ted Gould
Status: Merged
Approved by: Charles Kerr
Approved revision: 215
Merged at revision: 213
Proposed branch: lp://staging/~ted/ubuntu-app-launch/lp1495871-unref-context
Merge into: lp://staging/ubuntu-app-launch/16.04
Prerequisite: lp://staging/~mterry/ubuntu-app-launch/fix-ftbfs
Diff against target: 193 lines (+84/-7)
6 files modified
debian/libubuntu-app-launch2.symbols (+1/-0)
helpers-shared.c (+11/-2)
libubuntu-app-launch/ubuntu-app-launch.c (+5/-5)
libubuntu-app-launch/ubuntu-app-launch.h (+12/-0)
tools/CMakeLists.txt (+9/-0)
tools/ubuntu-app-list-pids.c (+46/-0)
To merge this branch: bzr merge lp://staging/~ted/ubuntu-app-launch/lp1495871-unref-context
Reviewer Review Type Date Requested Status
Charles Kerr (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+279801@code.staging.launchpad.net

This proposal supersedes a proposal from 2015-12-02.

Commit message

Ensure all pending events on the context are complete before unref'ing it.

Description of the change

So while we create a context to ensure that we can capture all the events to the CGManager connection it seems that some of those events can get stuck on the context, and they need to get processed to complete. Otherwise they hold onto references of things, and that actually results in a g_spawn getting left around because it never gets free'd. (actually of the context itself, which is a bit confusing, but eh, refcounting is fun)

Also brought in the pid tools branch because it made testing this much, much easier. You can use the ubuntu-app-list-pids tool to cause the FD to be leaked and then see it fixed with something like (assuming you've done ubuntu-app-launch firefox):

valgrind --track-fds=yes ./ubuntu-app-list-pids firefox

Which on trunk will have 6 FDs left over, but with this branch only 5. (which come from globals that we don't control)

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Charles Kerr (charlesk) wrote :

The code looks fine to me.

IIUC, the while(pending){iterate();} snippet is the main fix, while the rest of it is either sugar (eg qdata instead of data) or the pid tools branch, right?

Optional, I'd suggest extracting while(pending){iterate();} into an inline function to improve the readability a bit, eg flush_pending_events(GMainContext*). IMO this would also address thomas' comment on the readability of the /* may block */ comment

review: Approve

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