Closed
Bug 483500
Opened 16 years ago
Closed 16 years ago
trace-refcnt should use 64-bit counters: root cause follow-up to bug 482236
Categories
(Core :: XPCOM, defect, P3)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla1.9.1b4
People
(Reporter: jruderman, Assigned: dbaron)
References
Details
(Keywords: fixed1.9.1, intermittent-failure, memory-leak)
Attachments
(2 files)
14.00 KB,
patch
|
Details | Diff | Splinter Review | |
4.63 KB,
patch
|
benjamin
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
I suspect the weirdness in bug 482236 is a result of more than 2^32 objects being created. Can we fix this by replacing some "PRInt32" and "nsrefcnt" with 64-bit types?
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcntImpl.cpp#122
Updated•16 years ago
|
Flags: wanted1.9.1?
Updated•16 years ago
|
Summary: trace-refcnt should use 64-bit counters → trace-refcnt should use 64-bit counters: root cause follow-up to bug 482236
Whiteboard: [orange]
Yes, I think we need to. I agree that this is quite likely the cause of random orangeness.
Should be very easy ... any volunteers?
Whiteboard: [orange] → [orange][good first bug]
Comment 3•16 years ago
|
||
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237584473.1237590737.1535.gz
OS X 10.5.2 mozilla-central unit test on 2009/03/20 14:27:53
Comment 4•16 years ago
|
||
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237853880.1237861341.4729.gz
OS X 10.5.2 mozilla-central unit test on 2009/03/23 17:18:00
Updated•16 years ago
|
Comment 5•16 years ago
|
||
Updated•16 years ago
|
Attachment #370587 -
Flags: review?(bsmedberg)
Updated•16 years ago
|
Attachment #370587 -
Flags: review?(bsmedberg) → review?(benjamin)
Assignee | ||
Comment 6•16 years ago
|
||
So, I actually wrote another patch for this before finding this bug. I think my patch is roughly a subset of the above changes (except I converted some things to unsigned as well), and I think it's the subset that we want. I don't think we need to change the reference counts for individual objects; we only need to change the variables used to accumulate totals.
I think we definitely do *not* want to use "long nsrefcnt".
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #371921 -
Flags: review?(benjamin)
Updated•16 years ago
|
Attachment #371921 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → dbaron
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Updated•16 years ago
|
Attachment #370587 -
Flags: review?(benjamin)
Assignee | ||
Comment 8•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [orange][good first bug] → [orange]
Comment 9•16 years ago
|
||
(In reply to comment #6)
> except I converted some things to unsigned as well
Then, this can't handle 'Adds < Releases' or 'Creates < Destroys' anymore, can it ?
Assignee | ||
Comment 10•16 years ago
|
||
(In reply to comment #9)
> Then, this can't handle 'Adds < Releases' or 'Creates < Destroys' anymore, can
> it ?
If either of those are the case, we've probably crashed already. (Or the logging is broken.)
Those cases were already pretty broken even before this patch, though.
Comment 11•16 years ago
|
||
(In reply to comment #10)
> If either of those are the case, we've probably crashed already. (Or the
> logging is broken.)
Well, I wonder about such a crash,
and (bug 456894 comment 0 and) your bug 482236 comment 7 even made me think that to detect such cases was a feature.
(That's why I wanted to mention it.)
> Those cases were already pretty broken even before this patch, though.
But if you say now that the new behavior is better, then fine by me.
(In which case, I'll update (back?) the python parsing code.)
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 371921 [details] [diff] [review]
smaller patch
I think we want this on 1.9.1 to fix bug 483917
Attachment #371921 -
Flags: approval1.9.1?
Comment 13•16 years ago
|
||
Comment on attachment 371921 [details] [diff] [review]
smaller patch
a191=beltzner
Attachment #371921 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 14•16 years ago
|
||
Keywords: fixed1.9.1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b4
Comment 15•15 years ago
|
||
Oh ,yes.
we definitely don't want the "nsrefcnt".
I apologize for adding it and I think I wasn't sure whether PRUint64 could in fact replace "long nsrefcnt", even though I spent quite a bit of time trying to locate the appropriate replacement for nsrefcnt.
Apart from that I wasn't really sure about adding the unsigned part for the resolution.
For some reason, it looks like I didn't CC this to myself back then :(
Thanks sgautherie, dbaron and beltzner for the resolution of this bug
(In reply to comment #6)
> So, I actually wrote another patch for this before finding this bug. I think
> my patch is roughly a subset of the above changes (except I converted some
> things to unsigned as well), and I think it's the subset that we want. I don't
> think we need to change the reference counts for individual objects; we only
> need to change the variables used to accumulate totals.
>
> I think we definitely do *not* want to use "long nsrefcnt".
Comment 16•15 years ago
|
||
#if defined(XP_WIN) && PR_BYTES_PER_LONG == 4
typedef unsigned long nsrefcnt;
#else
typedef PRUint32 nsrefcnt;
#endif
@
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h
helped me justify the usage of PRUint64.
Updated•13 years ago
|
Flags: wanted1.9.1?
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [orange]
You need to log in
before you can comment on or make changes to this bug.
Description
•