modify to xpcom/ to use mozilla::Atomic

RESOLVED FIXED in mozilla26

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla26
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Everything except atomic reference-counting, which is bug 884061.
(Assignee)

Comment 1

5 years ago
Created attachment 764107 [details] [diff] [review]
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...
(Assignee)

Updated

5 years ago
Blocks: 884068
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?
(Assignee)

Comment 3

5 years ago
(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?
(Assignee)

Comment 4

5 years ago
Created attachment 764370 [details] [diff] [review]
modifications to xpcom/ to use mozilla::Atomic

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

Updated

5 years ago
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
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 794146 [details] [diff] [review]
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...
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+
(Assignee)

Comment 9

5 years ago
(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?
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
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
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/b3070fcb40c9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.