884281 - modify to xpcom/ to use mozilla::Atomic

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

Everything except atomic reference-counting, which is bug 884061.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: start making modifications to xpcom/ to use mozilla::Atomic

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 2

[Joshua Cranmer [:jcranmer]]

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?

Comment 3

[Nathan Froyd [:froydnj]]

(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?

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: modifications to xpcom/ to use mozilla::Atomic

A better patch that looks much greener on Try.  Thanks to jcranmer for the sanity check.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Nathan Froyd [:froydnj]]

(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.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: xpcom Atomic changes, nsSubstring.cpp

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...

Comment 8

[Justin Lebar (not reading bugmail)]

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?

Comment 9

[Nathan Froyd [:froydnj]]

(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.

Comment 10

[Justin Lebar (not reading bugmail)]

> 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?

Comment 11

[Nathan Froyd [:froydnj]]

(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.

Comment 12

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3070fcb40c9

Try link (mostly complete, but no random bustage like last time): https://tbpl.mozilla.org/?tree=Try&rev=76aaa0c4a133

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3070fcb40c9