1083897 - Require leak logs for tab processes

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew McCreight [:mccr8]]

Leak logging is (sort of) working on Linux now.  Unfortunately as we've seen random code can break leak logging without turning the tree orange, and then that code or later code can introduce leaks, which is bad.  So we should probably enable requiring logs ASAP.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Require leak logs for tab processes.

This isn't quite ready to go, but I want to have a reviewed patch in hand when it is.

Comment 2

[Andrew McCreight [:mccr8]]

What is blocking this now is various cases where we send messages late in shutdown on debug builds.

with this patch, with exit(0) on late messages: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3a3d4a617070
with this patch, with no exit(0) on late messages: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d7b5ee61b1af

Comment 3

[Andrew McCreight [:mccr8]]

Benjamin - Right now we have a slew of IPDL things that send messages late in shutdown, causing us to exit(0) in the child process (see the bugs blocking this one). This prevents us from getting leak logs consistently in the child process (this bug).

There are two possible solutions:
1. Fix all of these blocking bugs, so we never send messages late. This will be a bit tedious and may take a while.
2. Hack it so that we still don't send a message in this case, but we also don't exit(0). This leaves us in the state where we aren't sending messages some places when we might expect it. (Note that we don't report any error for this right now in a way that shows up on TBPL, so we're not really noticing this anyways...)

I asked Ben T., and he described himself as "kinda meh" on these options, and that I should ask you. Do you have a strong preference between the two of these? If not, I'm inclined to go with option 2. Thanks.

Comment 4

[Benjamin Smedberg]

I guess that depends on the goal, I think.

Option 1 is the expedient way to test *something*, but it will certainly cause more random orange. In particular if we start dropping messages on the floor, we are going to start leaking the objects related to those messages in some cases.

Option 2 seems like the thing we should be striving for. And even I think we should have some related invariants:

* There are no toplevel IPDL actors when we begin XPCOM shutdown.
* There are no active IPC Channel objects when we begin XPCOM shutdown.

That means we'll have to do the work to make sure we shut down actors/IPC before we begin shutdown, but it also means that we won't have to worry about the particular bug of having pending messages flying around during shutdown.

I'm sorry that's not a straight-up answer to the question. I expect that if you do option 1, you're going to end up being on the hook for option 2 later anyway, simply because of the randomorange.

Comment 5

[Andrew McCreight [:mccr8]]

Did you reverse option 1 and 2 there compared to what I had in comment 3 or am I just not understanding your point?  I don't understand how the hacky solution (just keep going if we send messages late) is something to be striving for. ;)

Comment 6

[Benjamin Smedberg]

Yes, I reversed them.

Comment 7

[Andrew McCreight [:mccr8]]

This is getting closer to being ready, but on Windows we intentionally exit(0) before we produce a leak log, due to an intermittent XP debug failure.  I was thinking this would be okay for now because we don't run e10s tests on Windows yet, but I forgot that we still do run some tests out-of-process in regular Mochitests. Maybe I can dig around in some test harness stuff to disable requiring logs on Windows.

Comment 8

[Andrew McCreight [:mccr8]]

Ok, this should be easy to fix.

I filed bug 1091917 for enabling requiring leak logs for content processes on Windows.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Don't require leak logs for tab processes on Windows.

We don't create leak logs in tab processes on Windows, so we shouldn't produce an error when they don't create one.

Windows try run looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4ae4d58dac66

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8515036 [details] [diff] [review]
part 2 - Don't require leak logs for tab processes on Windows.

Review of attachment 8515036 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for the bug info here!

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Require leak logs for tab processes on desktop.

Ignore missing leak logs for b2g tab processes.

Comment 12

[Andrew McCreight [:mccr8]]

Bug 1103036 seems like a better approach to dealing with shutdown weirdness.

Comment 13

[Andrew McCreight [:mccr8]]

e10s try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d6b02c33dce7
full try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7b13debca7e7

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ead5a0b265
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/783763e3562d

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b8ead5a0b265
https://hg.mozilla.org/mozilla-central/rev/783763e3562d

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Backed out on beta for bug 1138520:
https://hg.mozilla.org/releases/mozilla-beta/rev/56f805ac34ce