1316165 - reply_list/xcb_get_input_focus_reply_t leak from nsShmImage

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

The introduction of xcb_get_input_focus() in
https://hg.mozilla.org/mozilla-central/rev/ddcceb6bf9a6#l1.102 with the
xcb_get_input_focus_reply() in CreateDrawTarget() means that libxcb leaks the
replies in _xcb_in::replies whenever the nsShmImage is destroyed between Put()
and CreateDrawTarget(), which I assume is when destruction happens.

The leaks create a longer linked list for poll_for_reply() to search for every
subsequent reply that is freed.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: bug 1316165 ensure xcb_get_input_focus_reply is called to avoid leaking the reply

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

Comment 2

[Lee Salzman [:lsalzman]]

Comment on attachment 8808822 [details]
bug 1316165 ensure xcb_get_input_focus_reply is called to avoid leaking the reply

https://reviewboard.mozilla.org/r/91550/#review91674

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2635cff0413b
ensure xcb_get_input_focus_reply is called to avoid leaking the reply r=lsalzman

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2635cff0413b

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8808822 [details]
bug 1316165 ensure xcb_get_input_focus_reply is called to avoid leaking the reply

Approval Request Comment
[Feature/regressing bug #]: bug 1287666
[User impact if declined]:
A small memory leak, but one that may affect performance as it accumulates over time.
[Describe test coverage new/current, TreeHerder]:
Leak testing doesn't catch this because there is still a pointer to the leaked objects.
[Risks and why]: 
Low risk because the patch is simple.
Tested locally, but this is disabled by default in Nightly with many systems.
Enabled by default on Aurora.
[String/UUID change made/needed]: None.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8808822 [details]
bug 1316165 ensure xcb_get_input_focus_reply is called to avoid leaking the reply

Memory leak/perf fix, hooray, let's take it in 51 before the beta merge.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This is hitting conflicts with bug 1314291. Either need a rebased patch or an uplift request there too.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: aurora/51 patch

This is what I pushed to Aurora.

Same thing, but xcb_request_check(mPutRequest), which removed on central, is
also moved with the xcb_get_input_focus_reply().  Because there is no reply to
the put, this part would only leak if there were an error, which is not
expected, but keeping this code together is also consistent.

Comment 9

[Karl Tomlinson (:karlt)]

Thanks, Ryan.  I should have thought about that.

Pushed
https://hg.mozilla.org/releases/mozilla-aurora/rev/1da2ad7a0924a22c90c71ca24c7d53a7ff9eb9b9

Comment 10

[Karl Tomlinson (:karlt)]

Also needed to handle the early return should the put error:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec2757bbefb2134970ee18ff67ee43086a8ef4a0

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: roll-up of aurora/51 changes