1446181 - Investigate whether or not APZ subprotocols are being leaked

Status: REOPENED

(Core :: Panning and Zooming, task, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

According to https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/Best_Practices, all subprotocols should have Send__delete__() called on them to delete them. However I don't think we're doing this for our APZ protocols such as PAPZCTreeManager and PAPZ. So in theory we might be leaking stuff. We should investigate and make sure we're cleaning up properly.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I added a printf to the APZCTreeManagerParent constructor and destructor, and it looks like we create an APZCTreeManagerParent for each tab, as expected, and they only get destroyed when the *process* they're attached to terminates. So if you happen to close the last tab in a process a bunch of them get freed. If you run with dom.ipc.processCount=1 then none of them get freed until shutdown. So yeah, this seems like a leak we should fix. I haven't checked PAPZ yet.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Looks like PAPZ is fine, when the RemoteContentController is destroyed it sends a destroy message at [1] which does a Send__delete__ at [2].

[1] https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/gfx/layers/ipc/RemoteContentController.cpp#405
[2] https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/gfx/layers/ipc/APZChild.cpp#99

Comment 3

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7bd21955108f15e22e3039dd6d2be1fb15a168a

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1446181 - Send __delete__ messages when destroying APZCTreeManagerChild instances.

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

Comment 5

[Ryan Hunt [:rhunt]]

Comment on attachment 8990778 [details]
Bug 1446181 - Send __delete__ messages when destroying APZCTreeManagerChild instances.

https://reviewboard.mozilla.org/r/255830/#review262626

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b8041f7f604
Send __delete__ messages when destroying APZCTreeManagerChild instances. r=rhunt

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5b8041f7f604

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I'm going to back this out until I have time to fully investigate the crashes this introduced (bug 1475039). I took a quick look but I didn't see anything obvious, so this will take some debugging. In the meantime, transient leaks are better than crashes.

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0653a84312
Back out cset 5b8041f7f604 for introducing a low-volume crash. r=me

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I'm not actively working on this.