modify to xpcom/ to use mozilla::Atomic

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Everything except atomic reference-counting, which is bug 884061.
This patch compiles everywhere, but also leaks CondVars and Mutexes everywhere:

https://tbpl.mozilla.org/?tree=Try&rev=72036a04c4f4

Guessing it has something to do with Timers, but it's not immediately obvious
to me what's at fault.  Wonder if this is related to the intermittent leaks of
CondVars and Mutexes that we see normally...
Comment on attachment 764107 [details] [diff] [review]
start making modifications to xpcom/ to use mozilla::Atomic

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

::: xpcom/threads/nsTimerImpl.cpp
@@ +141,5 @@
>      MOZ_COUNT_DTOR(nsTimerEvent);
>  
>      MOZ_ASSERT(!sCanDeleteAllocator || sAllocatorUsers > 0,
>                 "This will result in us attempting to deallocate the nsTimerEvent allocator twice");
> +    sAllocatorUsers++;

Perhaps this is why you're leaking?
(In reply to Joshua Cranmer [:jcranmer] from comment #2)
> Comment on attachment 764107 [details] [diff] [review]
> start making modifications to xpcom/ to use mozilla::Atomic
> 
> Review of attachment 764107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/nsTimerImpl.cpp
> @@ +141,5 @@
> >      MOZ_COUNT_DTOR(nsTimerEvent);
> >  
> >      MOZ_ASSERT(!sCanDeleteAllocator || sAllocatorUsers > 0,
> >                 "This will result in us attempting to deallocate the nsTimerEvent allocator twice");
> > +    sAllocatorUsers++;
> 
> Perhaps this is why you're leaking?

Doh, that would do it, wouldn't it?
A better patch that looks much greener on Try.  Thanks to jcranmer for the sanity check.
Attachment #764107 - Attachment is obsolete: true
Attachment #764370 - Flags: review?(benjamin)
Depends on: 884676
No longer depends on: 884676
Attachment #764370 - Flags: review?(benjamin) → review+
Backed out for mass bustage (not that there's a comment here indicating that this was ever pushed to inbound...).
https://hg.mozilla.org/integration/mozilla-inbound/rev/c11e18571ae6
Assignee: nobody → nfroyd
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #5)
> Backed out for mass bustage (not that there's a comment here indicating that
> this was ever pushed to inbound...).
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c11e18571ae6

I was overly enthusiastic and made some ill-considered changes to nsSubstring.cpp that weren't fully correct.  I'll post a separate patch for those changes shortly.
nsStringBuffer is either new since I wrote the patch or I completely missed it the
first time.  In either event, the mRefCount changes need a sanity-check.  Not making
mRefCount Atomic induces all sorts of interesting-looking failures...
Attachment #794146 - Flags: review?(justin.lebar+bug)
Comment on attachment 794146 [details] [diff] [review]
xpcom Atomic changes, nsSubstring.cpp

>+          printf(" => mAllocCount:     % 10d\n", (int)mAllocCount);

Is this the best way to convert an Atomic<int32_t> to an int?
Attachment #794146 - Flags: review?(justin.lebar+bug) → review+
(In reply to Justin Lebar [:jlebar] from comment #8)
> Comment on attachment 794146 [details] [diff] [review]
> xpcom Atomic changes, nsSubstring.cpp
> 
> >+          printf(" => mAllocCount:     % 10d\n", (int)mAllocCount);
> 
> Is this the best way to convert an Atomic<int32_t> to an int?

Not sure.  But something like it is required, because varargs printf won't automagically invoke operator(), so without the cast the compiler complains about format string mismatches.
> so without the cast the compiler complains about format string mismatches.

For sure.  I guess I wonder if a function-style cast -- int(mAllocCount) -- would be safer than a C-style cast, in case reinterpret_cast'ing mAllocCount to int was not safe?
(In reply to Justin Lebar [:jlebar] from comment #10)
> > so without the cast the compiler complains about format string mismatches.
> 
> For sure.  I guess I wonder if a function-style cast -- int(mAllocCount) --
> would be safer than a C-style cast, in case reinterpret_cast'ing mAllocCount
> to int was not safe?

C++ style FTW.  I will do that.
https://hg.mozilla.org/mozilla-central/rev/b3070fcb40c9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.