1615072 - Leak checking for non-default processes not working on OSX

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

As part of investigating a leak, I wrote a patch that adds an intentional leak to the main process and content processes. Except, when I run the browser on OSX, it only reports leaks for the main process.

It seems like the problem is that the names of the leak logs are not being set properly on OSX. The log spew contains lots of this:

### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to (some stuff)/runtests_leaks.log

Now, some of those are supposed to have file names more like runtests_leaks_tab_pid91111.log. I think the file names are messed up and every process is dumping its leak log onto a single file.

The good news here is that judging by logs on TreeHerder, things are working fine on Linux and Windows, so hopefully there's not some hideous backlog of leaks that have crept in since this broke.

I haven't figured out exactly what has gone wrong yet. Bug 1604084 is the last major change to this file, but at first glance it feels like the only thing that could cause this is XRE_IsParentProcess() somehow returning the wrong value so I don't know what might be going on here.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Also, it would be nice to have some regression testing for whatever the issue is. I'm not sure if the log file is being completely overwritten, or appended to. If it is the latter, I have a bug on file for that issue already. But from some local testing, maybe it is being overwritten?

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

It does look like the issue is that XRE_IsParentProcess() is always returning true...

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

According to my logging, it looks like XRE_SetProcessType() is getting called after InitLog(), so IsParentProcess is returning true.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

It looks like we're calling MOZ_COUNT_CTOR in the ctor for nsTArray_base, before XRE_SetProcessType() is called.

Here's the stack, which does not really help me figure out what is going on:

#01: InitTraceLog()[/Users/andrewmccreight/mc/obj-dbg.noindex/toolkit/library/build/XUL +0xabe78]
#02: NS_LogCtor[/Users/andrewmccreight/mc/obj-dbg.noindex/toolkit/library/build/XUL +0xacf2a]
#03: mozilla::DeadlockDetector<mozilla::BlockingResourceBase>::Add(mozilla::BlockingResourceBase const*)[/Users/andrewmccreight/mc/obj-dbg.noindex/toolkit/library/build/XUL +0x14c38f]
#04: mozilla::RWLock::RWLock(char const*)[/Users/andrewmccreight/mc/obj-dbg.noindex/toolkit/library/build/XUL +0x155b10]
#05: __cxx_global_var_init.293[/Users/andrewmccreight/mc/obj-dbg.noindex/toolkit/library/build/XUL +0x171fa1c]

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

It looks like the string passed into RWLock is "rlbox", so maybe this is a regression from bug 1566288?

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

The rlbox thing is behind an #ifdef XP_MACOSX, which would explain why this is OSX only.

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

[Tracking Requested - why for this release]: We might be leaking in content processes on OSX and not know it.

Comment 8

[Nathan Froyd [:froydnj]]

Sorry, I will fix this. I think I thought about the problem and/or thought about fixing it in a followup, but completely failed to do that.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: Bug 1615072 - don't trigger the deadlock detector from static initializers for RWLock; r=mccr8

This change is a little gross, because we don't totally control where
the statically initialized instances of RWLock live, due to its uses
in third-party libraries.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b0bed287f1a
don't trigger the deadlock detector from static initializers for RWLock; r=mccr8

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4b0bed287f1a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 9126760 [details]
Bug 1615072 - don't trigger the deadlock detector from static initializers for RWLock; r=mccr8

Beta/Release Uplift Approval Request

• User impact if declined: None. But our beta builds won't be able to detect leaks for OS X, which seems bad.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Changing code in a not-frequently used area of the code, and the code changes are very similar to ones we have elsewhere for e.g. StaticMutex.
• String changes made/needed: None.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9126760 [details]
Bug 1615072 - don't trigger the deadlock detector from static initializers for RWLock; r=mccr8

Fixes broken leak checking on macOS. Approved for 74.0b4.

Comment 15

[Nathan Froyd [:froydnj]]

Do you have a link to the original intentional leak patch, so I could do a try push with that?

(Also, holy speedy approval, Batman!)

Comment 16

[Andrew McCreight (out of office until 8/21) [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #15)

Do you have a link to the original intentional leak patch, so I could do a try push with that?

You can add this to BackstagePass::BackstagePass() (I'm not sure the horrible pointer arithmetic is actually necessary):
NS_LogCtor(((char*)this) + 100, "WayBackstagePass", sizeof(*this));

Note that you'll still get leaks with this, but some should be "leakcheck | tab" in addition to having them "leakcheck | default" (which is how they show up in the main process). You could probably guard the NS_LogCtor call to only happen if we're not in the main process to cut down on the noise.

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f9c4712fcbb