Can't use XPCOM stack walking before NS_LogInit() is called

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Unassigned)

Tracking

Trunk
mozilla18
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Bug 696376 changed things so if I start Firefox with the environment

  XPCOM_MEM_REFCNT_LOG=1 XPCOM_MEM_ALLOC_LOG=1 XPCOM_MEM_LOG_CLASSES=nsTArray_base

I die with

  "Assertion failure: gCriticalAddress.mInit, at ../../../src/xpcom/base/nsStackWalk.cpp:1191"

The problem is that gCriticalAddress.mInit is true only after NS_LogInit() is called, and we create nsTArray's before that point in time.

Since Linux doesn't even care about these critical addresses, a solution there is just to call StackWalkInitCriticalAddresses() instead of fatally asserting.

I don't know if this will work for Mac, though.
(Reporter)

Comment 1

6 years ago
Created attachment 658576 [details] [diff] [review]
Patch, v1

I have no idea if this is right on mac.
Attachment #658576 - Flags: review?(respindola)
It might work as long as we are not creating a mutex before getting to StackWalkInitCriticalAddress. For a description of the problem, see

https://blog.mozilla.org/respindola/2011/12/04/tracking-malloc-on-os-x/

I think the best solution is to move the call to StackWalkInitCriticalAddresses from to NS_LogInit to some earlier position in the code. If this is too hard, this patch might be OK with a comment explaining that when refcount logging is enabled we can try to stack walk before NS_LogInit, but in that case StackWalkInitCriticalAddress is still called before any mutex is created (if that is the case of course).

StackWalkInitCriticalAddress itself asserts that it is able to find new_sem_from_pool, so you should see failures on 10.6 when running the leaktest if this patch has a problem. Unfortunately, try runs the leaktest on 10.7 these days, so you will have to check this locally.

(Yet another option is to say you need 10.7 to run the leaktest, not sure if we should do that or not).
(Reporter)

Comment 3

6 years ago
> It might work as long as we are not creating a mutex before getting to 
> StackWalkInitCriticalAddress.

This patch can only result in StackWalkInitCriticalAddress being called /earlier/, so in this respect, it should be no less safe than the current code.

> I think the best solution is to move the call to StackWalkInitCriticalAddresses from to NS_LogInit 
> to some earlier position in the code.

The trick is, you'd have to move that call before we create any leak-tracked object, otherwise we still have the same problem, just s/nsTArray/some-other-object/.  It's not clear to me where you'd move it that could have this property, but maybe there exists a location...
> This patch can only result in StackWalkInitCriticalAddress being called
> /earlier/, so in this respect, it should be no less safe than the current
> code.

True. The only possible remaining problem is that we might also be creating a mutex earlier when leak logging is enabled, but in that case this patch will only change which assert gets hit. I will r+.

> > I think the best solution is to move the call to StackWalkInitCriticalAddresses from to NS_LogInit 
> > to some earlier position in the code.
> 
> The trick is, you'd have to move that call before we create any leak-tracked
> object, otherwise we still have the same problem, just
> s/nsTArray/some-other-object/.  It's not clear to me where you'd move it
> that could have this property, but maybe there exists a location...

Yes, I remember it was difficult to find a good place to put the call when writing the original patch.
Comment on attachment 658576 [details] [diff] [review]
Patch, v1

Please just add a comment to StackWalkInitCriticalAddress about where it is called from and why we need both NS_LogInit (regular runs) and the stack walking functions (refcount logging is enabled).
Attachment #658576 - Flags: review?(respindola) → review+
https://hg.mozilla.org/mozilla-central/rev/cf39151876dc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.