Use mozilla/Atomic instead of NS_AtomicRefcnt* for threadsafe refcounts

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(31 attachments, 3 obsolete attachments)

5.21 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
8.01 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
1.70 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
1.87 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.78 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
21.70 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.46 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
68.16 KB, patch
dhylands
: review+
smaug
: review+
Details | Diff | Splinter Review
8.39 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
8.25 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.41 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
13.57 KB, patch
joe
: review+
Details | Diff | Splinter Review
18.15 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
3.92 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
49.86 KB, patch
bholley
: review+
Details | Diff | Splinter Review
12.99 KB, patch
abr
: review+
Details | Diff | Splinter Review
20.69 KB, patch
taras.mozilla
: review+
benjamin
: review+
Details | Diff | Splinter Review
161.13 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
51.67 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
2.88 KB, patch
mwu
: review+
Details | Diff | Splinter Review
16.75 KB, patch
mak
: review+
Details | Diff | Splinter Review
38.58 KB, patch
mossop
: review+
Details | Diff | Splinter Review
9.51 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.37 KB, patch
jimm
: review+
Details | Diff | Splinter Review
79.41 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
3.18 KB, patch
neil
: review+
Details | Diff | Splinter Review
35.37 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
134.88 KB, patch
standard8
: review+
Details | Diff | Splinter Review
5.06 KB, patch
standard8
: review+
Details | Diff | Splinter Review
4.38 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
NS_AtomicRefcnt are basically inline functions wrapping PR_ATOMIC_* stuff; we now have C++11-style atomics in MFBT, so we should use them here instead. The main difficulty is that the MFBT wrappers don't allow atomic operations on regular variables by design, so replacing this stuff is a bit difficult.

Currently, the threadsafe gunk in nsSupportsImpl.h are:
NS_INLINE_DECL_THREADSAFE_RECOUNTING (trivial changes here)
NS_IMPL_THREADSAFE_QUERYINTERFACE* (#define'd to the regular kind, no changes)
NS_IMPL_THREADSAFE_ADDREF/_REMOVE
NS_IMPL_THREADSAFE_ISUPPORTS*

In order to support MFBT atomics, we'd need to introduce an NS_DECL_THREADSAFE_ISUPPORTS that declares the Atomic<nsrefcnt> mRefCnt parameter.

As an extra bonus, we could eliminate the need for a separate NS_IMPL_THREADSAFE* suite of macros by changing this line of the regular macro:
  NS_ASSERT_OWNINGTHREAD_AND_NOT_CCTHREAD(_class);

to call instead of a macro a templated function on nsrefcnt/Atomic<nsrefcnt>.
(Assignee)

Updated

6 years ago
Blocks: 884068
(Assignee)

Comment 1

6 years ago
This is roughly the kind of thing I'm looking at doing here--adding NS_DECL_THREADSAFE_ISUPPORTS, and using it whenever people currently use NS_IMPL_ISUPPORTS. My plan of action is as follows:

1. Swap the NS_DECL_INLINE_* macros first and deal with fallout with that change alone.
2. Add NS_DECL_THREADSAFE_ISUPPORTS, rejigger the non-threadsafe addref/release to work with either NS_DECL_THREADSAFE_ISUPPORTS or NS_DECL_ISUPPORTS.
[2b: For testing, assert that NS_IMPL_THREADSAFE_ISUPPORTS* match their NS_DECL_* counterparts]
3. Change all the implementations to use NS_DECL_THREADSAFE_ISUPPORTS and NS_IMPL_ISUPPORTS_*, broken up by top-level directory
4. Redefine all NS_IMPL_THREADSAFE_* to their non-threadsafe counterparts. Or delete them, if we don't care about API compatibility.
5. Fix up everyone else who uses nsAtomicRefcnt.h
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #764545 - Flags: feedback?(justin.lebar+bug)
> 5. Fix up everyone else who uses nsAtomicRefcnt.h

It sounds like we shouldn't even need nsAtomicRefcnt.h anymore?
(Assignee)

Comment 3

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #2)
> > 5. Fix up everyone else who uses nsAtomicRefcnt.h
> 
> It sounds like we shouldn't even need nsAtomicRefcnt.h anymore?

Once 5 is done, correct.
This sounds great.

>diff --git a/xpcom/glue/nsISupportsImpl.h b/xpcom/glue/nsISupportsImpl.h
>--- a/xpcom/glue/nsISupportsImpl.h
>+++ b/xpcom/glue/nsISupportsImpl.h
>+namespace mozilla {
>+class ThreadSafeAutoRefCnt {
>+ public:
>+    ThreadSafeAutoRefCnt() : mValue(0) {}
>+    ThreadSafeAutoRefCnt(nsrefcnt aValue) : mValue(aValue) {}
>+
>+    // only support prefix increment/decrement
>+    nsrefcnt operator++() { return ++mValue; }
>+    nsrefcnt operator--() { return --mValue; }
>+
>+    nsrefcnt operator=(nsrefcnt aValue) { return (mValue = aValue); }
>+    operator nsrefcnt() const { return mValue; }
>+    nsrefcnt get() const { return mValue; }
>+ private:
>+    // do not define these to enforce the faster prefix notation
>+    nsrefcnt operator++(int);
>+    nsrefcnt operator--(int);

FWIW I don't think prefix is any faster than suffix in our particular implementation.

>+    Atomic<nsrefcnt> mValue;

Do we want looser ordering here?  It seems to me that refcounts don't need to be sequentially consistent, since we don't care how one object's refcount compares to another object's refcount.

>@@ -282,16 +304,27 @@ public:                                 
>                             void** aInstancePtr);                             \
>   NS_IMETHOD_(nsrefcnt) AddRef(void);                                         \
>   NS_IMETHOD_(nsrefcnt) Release(void);                                        \
> protected:                                                                    \
>   nsAutoRefCnt mRefCnt;                                                       \
>   NS_DECL_OWNINGTHREAD                                                        \
> public:
> 
>+#define NS_DECL_THREADSAFE_ISUPPORTS                                          \
>+public:                                                                       \
>+  NS_IMETHOD QueryInterface(REFNSIID aIID,                                    \
>+                            void** aInstancePtr);                             \
>+  NS_IMETHOD_(nsrefcnt) AddRef(void);                                         \
>+  NS_IMETHOD_(nsrefcnt) Release(void);                                        \
>+protected:                                                                    \
>+  mozilla::ThreadSafeAutoRefCnt mRefCnt;                                      \

To be pedantic, I think should be ::mozilla::ThreadSafeAutoRefCnt?
Attachment #764545 - Flags: feedback?(justin.lebar+bug) → feedback+
(Assignee)

Comment 5

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #4)
> >+    Atomic<nsrefcnt> mValue;
> 
> Do we want looser ordering here?  It seems to me that refcounts don't need
> to be sequentially consistent, since we don't care how one object's refcount
> compares to another object's refcount.

It should be safe to use Atomic<nsrefcnt, ReleaseAcquire> here; I'm hesitant to do so because problems won't show up on x86/x86-64 (all atomic-rmw operations on x86 ISA are inherently sequentially consistent), and I don't know how much to trust our automated test infrastructure to pick up on potential problems on ARM.
I don't think I fully understand why relaxed ordering wouldn't work.  I tried to come up with some situations where it would make a difference, but all of them have two race conditions -- one on the refcount, and one on some other piece of data.  If you remove the second race, the first one doesn't matter.

Anyway if you want to do the slow thing to be cautious, that's fine with me.  But we have seen threadsafe refcounting show up in profiles.
(Assignee)

Comment 7

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #6)
> I don't think I fully understand why relaxed ordering wouldn't work.  I
> tried to come up with some situations where it would make a difference, but
> all of them have two race conditions -- one on the refcount, and one on some
> other piece of data.  If you remove the second race, the first one doesn't
> matter.

Here's a situation:

[foo has two references]
Thread A:
 Bar *baz = foo->GetElement(3);
 foo->Release();

Thread B:
 foo->Release();

Under relaxed memory ordering model, the compiler or hardware can reorder the decrement of foo's refcount to before the access of foo->GetElement(3); then B could decrement the refcount, detect that it's 0, and delete foo before A gets a chance to access GetElement(3). Yes, this is a data race, but it would not be one if refcounting were ReleaseAcquire or SequentiallyConsistent (since the atomic decrements synchronize with each other). Additionally, keeping at least ReleaseAcquire consistency allows us to guarantee that data which is used by only thread at a time can synchronize safely with only the refcounting.

> Anyway if you want to do the slow thing to be cautious, that's fine with me.
> But we have seen threadsafe refcounting show up in profiles.

And on x86/x86-64, the only way to make atomic refcounting faster is to not to do it, since you only have sequentially-consistency for atomic rmw operations.
I see, thanks for the example!

> And on x86/x86-64, the only way to make atomic refcounting faster is to not to do it, 
> since you only have sequentially-consistency for atomic rmw operations.

Understood, but obviously we care about ARM perf too these days.
(Assignee)

Comment 9

6 years ago
There are two complications I've discovered while attempting to convert everybody:

1. nsAutoRefCnt has copy-constructors and assignment operators defined by default for the compiler. mozilla::Atomic explicitly deletes its copy/assignment (since std::atomic did the same), so mozilla::ThreadSafeAutoRefCnt won't have them. This fact shouldn't matter in theory, but:
<http://dxr.mozilla.org/mozilla-central/source/content/media/MediaResource.h#l55> (WTF!?)

Looking in further, this is *very* broken code and seems almost certain to cause a leak. Is it worth filing a follow up to delete operator=/copy-constructor on other ref counts as well?

2. In our build, <atomic> gets wrapped in STL-wrappers, so conversion causes this issue:
nsTestSample.cpp
In file included from ../../../dist/include/mozilla/Atomics.h:176:0,
                 from ../../../dist/include/nsISupportsImpl.h:33,
                 from ../../../dist/include/nsISupportsUtils.h:26,
                 from ../../../dist/include/nsCOMPtr.h:34,
                 from /src/trunk/mozilla/xpcom/sample/program/nsTestSample.cpp:16:
../../../dist/stl_wrappers/atomic:41:4: error: #error "STL code can only be used with infallible ::operator new()"

The easiest way I saw around this was to undef XPCOM_GLUE around the nsCOMPtr.h inclusion, which feels dirty to me and so I want to avoid it. Thoughts?
I suspect that answer is that we just shouldn't have atomic refcounting or include Atomics.h when XPCOM_GLUE is defined. It can't use infallible-malloc because that requires the base library which hasn't been located yet.

Comment 11

6 years ago
(In reply to comment #9)
> There are two complications I've discovered while attempting to convert
> everybody:
> 
> 1. nsAutoRefCnt has copy-constructors and assignment operators defined by
> default for the compiler. mozilla::Atomic explicitly deletes its
> copy/assignment (since std::atomic did the same), so
> mozilla::ThreadSafeAutoRefCnt won't have them. This fact shouldn't matter in
> theory, but:
> <http://dxr.mozilla.org/mozilla-central/source/content/media/MediaResource.h#l55>
> (WTF!?)

Wrong link?
> Wrong link?

The copy constructor copies the old object's refcount.

Comment 13

6 years ago
(In reply to comment #12)
> > Wrong link?
> 
> The copy constructor copies the old object's refcount.

Yes, jcranmer proved me wrong on IRC immediately after I submitted that comment.  :-)
(Assignee)

Comment 14

6 years ago
This patch appears to be working on all platforms:
<https://tbpl.mozilla.org/?tree=Try&rev=68e8cec6c0de> (I'm assuming that most issues would be caught by mochitests and xpcshell tests); since it's nice, small, self-contained, and doesn't appear to be breaking anybody, I'll upload this separately from everyone else.

This also fixes the borked copy-constructor issue mentioned earlier.
Attachment #764545 - Attachment is obsolete: true
Attachment #771023 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 15

6 years ago
This is the patch that really lets us mozilla::Atomic. Since NS_IMPL_THREADSAFE_ADDREF/_RELEASE wouldn't mean anything different from their non-threadsafe counterparts, I've opted to decide to remove them altogether. This means that the NS_IMPL_ADDREF/_RELEASE methods become slightly funky to keep the threadsafety assertions in place. I can't find an alternative implementation of these checks that would allow me to omit NS_DECL_OWNINGTHREAD and keep the assertion message (retaining the class name is effectively impossible if I try to use methods instead).
Attachment #771793 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 16

6 years ago
I'm not uploading parts 3 and 4 (and maybe 5? I haven't quite decided how to structure the middle part of my queue yet) for review until I get explicit feedback on this patch, particularly the part about removing the IMPL_THREADSAFE macros altogether. This patch also has some junk in it about adding in pratom.h includes to fix the included-by-proxy assumptions that I'll move to other patches before the final review request; the main part here to look at is the changes in nsISupportsImpl.h.
Attachment #771795 - Flags: feedback?(justin.lebar+bug)
Attachment #771795 - Flags: feedback?(benjamin)
Comment on attachment 771023 [details] [diff] [review]
Part 1: Use mozilla::Atomic in NS_INLINE_DECL_THREADSAFE_REFCOUNTING

>+    // do not define these to enforce the faster prefix notation

This feels like a premature optimization; I'd prefer if we defined the postfix operators, unless we actually have a good reason not to.  The postfix operation on Atomic appears to be no slower than the prefix one.  Perhaps there are other relevant speed differences, but I'd expect the overhead of Atomic to eclipse them.

But I don't gate the review on that change.
Attachment #771023 - Flags: review?(justin.lebar+bug) → review+
Maybe you could add a comment on 

     Atomic<nsrefcnt> mValue;

indicating that ReleaseAcquire ordering should work, in theory?  That way if someone comes poking around here because atomic refcounting is slow on Android, they at least know that something can be done, maybe.
> This feels like a premature optimization

Ah, I guess you're just matching what nsAutoRefCount does.  I suppose that's OK.
(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> I can't find an
> alternative implementation of these checks that would allow me to omit
> NS_DECL_OWNINGTHREAD and keep the assertion message (retaining the class
> name is effectively impossible if I try to use methods instead).

I suppose you could add a NS_DECL_FAKE_OWNINGTHREAD, that declares a member that has the same C++ interface as nsAutoOwningThread, but is actually empty.  But since NS_DECL_OWNINGTHREAD doesn't do anything in release builds, I guess it doesn't matter.
Attachment #771793 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 771795 [details] [diff] [review]
Part N: Remove transitional vestiges

Yep, this seems sane to me.
Attachment #771795 - Flags: feedback?(justin.lebar+bug) → feedback+
Comment on attachment 771795 [details] [diff] [review]
Part N: Remove transitional vestiges

jcranmer said on IRC that he was asking for feedback on removing NS_IMPL_THREADSAFE*. And although I'm a little worried about code which using these atomics becoming less obvious because the atomic action looks like a normal integer increment, I don't think that affects the macro-removal. We shouldn't leave the macros around for compat, because they don't actually guarantee anything any more, and people would get confused.
Attachment #771795 - Flags: feedback?(benjamin) → feedback+
(Assignee)

Updated

6 years ago
Attachment #776166 - Flags: review?(jonas)
(Assignee)

Comment 36

6 years ago
Attachment #776174 - Flags: review?(brendan)
Attachment #776174 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 38

6 years ago
Attachment #776176 - Flags: review?(taras.mozilla)
Attachment #776176 - Flags: review?(benjamin)
(Assignee)

Comment 40

6 years ago
Attachment #776178 - Flags: review?(axel)
(Assignee)

Comment 44

6 years ago
Attachment #776182 - Flags: review?(dtownsend+bugmail)
(Assignee)

Comment 49

6 years ago
An awful lot of code expects nsISupportsImpl.h/nsAtomicRefcnt.h/nsCOMPtr.h to pull in pratom.h. And in one case, nsError.h.
Attachment #771795 - Attachment is obsolete: true
Attachment #776188 - Flags: review?(justin.lebar+bug)
Comment on attachment 776160 [details] [diff] [review]
Part 3b: Use NS_DECL_THREADSAFE_ISUPPORTS in caps/

r=me
Attachment #776160 - Flags: review?(bzbarsky) → review+
Comment on attachment 776164 [details] [diff] [review]
Part 3d: Use NS_DECL_THREADSAFE_ISUPPORTS in content/

Going to punt this to Olli.
Attachment #776164 - Flags: review?(bzbarsky) → review?(bugs)
Comment on attachment 776165 [details] [diff] [review]
Part 3e: Use NS_DECL_THREADSAFE_ISUPPORTS in docshell/

r=me
Attachment #776165 - Flags: review?(bzbarsky) → review+
Comment on attachment 776183 [details] [diff] [review]
Part 3w: Use NS_DECL_THREADSAFE_ISUPPORTS in uriloader/

r=me
Attachment #776183 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 56

6 years ago
Note to reviewers: I didn't try to guess whether classes needed to be threadsafe or not, just ported the declarations across as appropriate, except where things were really wonky (I believe that nsPlatformCharset is the only case). The change of NS_IMPL_THREADSAFE_ -> NS_IMPL_ was done by script in general, while almost all of the other changes were done by hand but verified via several try builds. Confirmation that I moved them correctly was done by this try build: <https://tbpl.mozilla.org/?tree=Try&rev=09e84fb489d3> (OS X release has the intermittent leakstats bug, which is why it is red).

The patches are up-to-date as of <http://hg.mozilla.org/mozilla-central/rev/18467a85acf6> (m-c tip on Saturday). In my experience, there is very little new THREADSAFE classes added, and most of what is added appears to happen in netwerk. My assumption is that r+ here is an implicit rubber-stamp to also convert affected new code added between Saturday and the actual push via the same semi-mechanical change process--I'll keep that as a separate patch that I'll post here for record-keeping.

I'm not sure all the reviewers I have are appropriately requested, as some of this code doesn't appear to be listed in the modules page. If you think I need extra reviewers for any of these patches, please add them since I don't have a good idea on who is best suited for review in most of mozilla-central.
Attachment #776166 - Flags: review?(doug.turner) → review?(dhylands)
Comment on attachment 776179 [details] [diff] [review]
Part 3s: Use NS_DECL_THREADSAFE_ISUPPORTS in security/

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

r+ with review comments addressed.

::: security/manager/boot/src/nsEntropyCollector.cpp
@@ +27,2 @@
>                                nsIEntropyCollector,
>                                nsIBufEntropyCollector)

Fix the indention.

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +155,5 @@
>                                nsIWebProgressListener,
>                                nsIFormSubmitObserver,
>                                nsIObserver,
>                                nsISupportsWeakReference,
>                                nsISSLStatusProvider)

ditto.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +87,2 @@
>                                nsIObserver,
>                                nsIStrictTransportSecurityService)

ditto.

::: security/manager/pki/src/nsASN1Tree.cpp
@@ +12,1 @@
>                                                   nsITreeView)

ditto. also, trailing whitespace.

::: security/manager/pki/src/nsNSSDialogs.cpp
@@ +53,5 @@
>                                              nsICertPickDialogs,
>                                              nsITokenDialogs,
>                                              nsIDOMCryptoDialogs,
>                                              nsIGeneratingKeypairInfoDialogs,
>                                              nsISSLCertErrorDialog)

ditto.

::: security/manager/pki/src/nsPKIParamBlock.cpp
@@ +13,1 @@
>                                                 nsIDialogParamBlock)

ditto.

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +73,5 @@
>                                nsIInterfaceRequestor,
>                                nsISSLStatusProvider,
>                                nsIAssociatedContentSecurity,
>                                nsISerializable,
>                                nsIClassInfo)

ditto.

::: security/manager/ssl/src/nsCMS.cpp
@@ +33,1 @@
>                                              nsICMSMessage2)

ditto. also, trailing whitespace.

::: security/manager/ssl/src/nsCertOverrideService.cpp
@@ +85,3 @@
>                                nsICertOverrideService,
>                                nsIObserver,
>                                nsISupportsWeakReference)

ditto, also trailing whitespace.

::: security/manager/ssl/src/nsClientAuthRemember.cpp
@@ +31,2 @@
>                                nsIObserver,
>                                nsISupportsWeakReference)

ditto. also, trailing whitespace.

::: security/manager/ssl/src/nsKeygenHandler.h
@@ +32,5 @@
>  
>    NS_IMETHOD ProvideContent(const nsAString& aFormType, 
>                              nsTArray<nsString>& aContent, 
>                              nsAString& aAttribute); 
> +  NS_DECL_THREADSAFE_ISUPPORTS 

trailing whitespace.

::: security/manager/ssl/src/nsNSSASN1Object.cpp
@@ +13,1 @@
>                                                   nsIASN1Object)

fix indention.

@@ +15,1 @@
>                                                        nsIASN1Object)

fix indention.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +303,5 @@
>  
>  void
>  nsNSSHttpRequestSession::AddRef()
>  {
> +  ++mRefCount;

Shouldn't this class just be derived from AtomicRefCounted<nsNSSHttpRequestSession> instead?

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +73,5 @@
>                                                  nsIX509Cert3,
>                                                  nsIIdentityInfo,
>                                                  nsISMimeCert,
>                                                  nsISerializable,
>                                                  nsIClassInfo)

Fix alignment, putting the nsIX509Cert part on its own line like is normally done.

@@ +1586,1 @@
>                                nsISimpleEnumerator)

fix alignment and trailing whitespace.

::: security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
@@ +24,2 @@
>                                                  nsISerializable,
>                                                  nsIClassInfo)

fix alignment and trailing whitespace, putting nsIX50Cert on its own line as is normally done.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1418,5 @@
>                                nsISignatureVerifier,
>                                nsIEntropyCollector,
>                                nsINSSComponent,
>                                nsIObserver,
>                                nsISupportsWeakReference)

fix alignment.

::: security/manager/ssl/src/nsRecentBadCerts.cpp
@@ +26,1 @@
>                                nsIRecentBadCerts)

fix alignment.
Attachment #776179 - Flags: review?(brian) → review+
You probably want me or mounir for hal; I don't think cjones is reviewing much these days.

I'm happy to do it but I'm swamped with critical-priority B2G stuff for the next week at least.  I'll try not to be the long pole here.
Comment on attachment 776172 [details] [diff] [review]
Part 3l: Use NS_DECL_THREADSAFE_ISUPPORTS in intl/

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

r=me with indentation nits fixed.

::: intl/strres/src/nsStringBundle.cpp
@@ +202,2 @@
>                                nsIStringBundle)
>  

Nit: fix indentation

@@ +526,4 @@
>                                nsIStringBundleService,
>                                nsIObserver,
>                                nsISupportsWeakReference)
>  

Nit: fix indentation

::: intl/uconv/src/nsCharsetConverterManager.cpp
@@ +35,2 @@
>                                nsICharsetConverterManager)
>  

and here

::: intl/uconv/util/nsUCSupport.cpp
@@ +34,3 @@
>                                nsIUnicodeDecoder,
>                                nsIBasicDecoder)
>  #else

and here
Comment on attachment 776159 [details] [diff] [review]
Part 3a: Use NS_DECL_THREADSAFE_ISUPPORTS in accessible/

>-NS_IMPL_THREADSAFE_ISUPPORTS3(DocManager,
>+NS_IMPL_ISUPPORTS3(DocManager,
>                               nsIWebProgressListener,
>                               nsIDOMEventListener,

indentation

>diff --git a/accessible/src/base/DocManager.h b/accessible/src/base/DocManager.h
>--- a/accessible/src/base/DocManager.h
>-  NS_DECL_ISUPPORTS
>+  NS_DECL_THREADSAFE_ISUPPORTS

it doesn't make any sense for this to be used off main thread, but I'm not absolutely sure docshell doesn't do insane things with web progress listeners so...

fwiw assuming you don't want to fix up crap that doesn't need threadsafe refcounting imho you don't need per module sign off for everything that's just changing the macros to keep things the same.
Attachment #776159 - Flags: review?(dbolter) → review+
Comment on attachment 776164 [details] [diff] [review]
Part 3d: Use NS_DECL_THREADSAFE_ISUPPORTS in content/


>-NS_IMPL_THREADSAFE_ISUPPORTS1(nsHTMLDNSPrefetch::nsListener,
>+NS_IMPL_ISUPPORTS1(nsHTMLDNSPrefetch::nsListener,
>                               nsIDNSListener)

indentation


>-NS_IMPL_THREADSAFE_ISUPPORTS2(MediaDocumentStreamListener,
>+NS_IMPL_ISUPPORTS2(MediaDocumentStreamListener,
>                               nsIRequestObserver,
>                               nsIStreamListener)
> 
ditto
Attachment #776164 - Flags: review?(bugs) → review+
Comment on attachment 776184 [details] [diff] [review]
Part 3x: Use NS_DECL_THREADSAFE_ISUPPORTS in widget/

Everything appears to match up. Note, similar to other reviews, please touch up indentation in generated code.
Attachment #776184 - Flags: review?(jmathies) → review+
Comment on attachment 776177 [details] [diff] [review]
Part 3q: Use NS_DECL_THREADSAFE_ISUPPORTS in netwerk/

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

there are a couple comments about things other than indentation buried in here

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +712,1 @@
>                                nsIBackgroundFileSaver,

indent

@@ +810,5 @@
>  
>  ////////////////////////////////////////////////////////////////////////////////
>  //// BackgroundFileSaverStreamListener
>  
> +NS_IMPL_ISUPPORTS3(BackgroundFileSaverStreamListener,

indent

@@ +979,1 @@
>                                nsIOutputStream)

indent

::: netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp
@@ +30,5 @@
>  #else
>  #define LOG(args)
>  #endif
>  
> +NS_IMPL_ISUPPORTS2(nsAsyncRedirectVerifyHelper,

indent

::: netwerk/base/src/nsAsyncStreamCopier.cpp
@@ +92,1 @@
>                                nsIRequest,

indent

::: netwerk/base/src/nsFileStreams.cpp
@@ +55,1 @@
>                                nsISeekableStream,

indent

::: netwerk/base/src/nsIOService.cpp
@@ +300,5 @@
>      NS_ADDREF(gIOService);
>      return gIOService;
>  }
>  
> +NS_IMPL_ISUPPORTS6(nsIOService,

indent

::: netwerk/base/src/nsPACMan.cpp
@@ +561,5 @@
>    mInProgress = false;
>    return true;
>  }
>  
> +NS_IMPL_ISUPPORTS3(nsPACMan, nsIStreamLoaderObserver,

indent

::: netwerk/base/src/nsPreloadedStream.cpp
@@ +12,5 @@
>     
>  namespace mozilla {
>  namespace net {
>  
> +NS_IMPL_ISUPPORTS2(nsPreloadedStream,

indent

::: netwerk/base/src/nsRequestObserverProxy.cpp
@@ +114,5 @@
>  //-----------------------------------------------------------------------------
>  // nsRequestObserverProxy::nsISupports implementation...
>  //-----------------------------------------------------------------------------
>  
> +NS_IMPL_ISUPPORTS2(nsRequestObserverProxy,

indent

::: netwerk/base/src/nsServerSocket.cpp
@@ +409,5 @@
>    nsMainThreadPtrHandle<nsIServerSocketListener> mListener;
>    nsCOMPtr<nsIEventTarget> mTargetThread;
>  };
>  
> +NS_IMPL_ISUPPORTS1(ServerSocketListenerProxy,

indent

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +1731,5 @@
>  
>  //-----------------------------------------------------------------------------
>  // xpcom api
>  
> +NS_IMPL_ISUPPORTS4(nsSocketTransport,

indent

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +400,5 @@
>  
>  //-----------------------------------------------------------------------------
>  // xpcom api
>  
> +NS_IMPL_ISUPPORTS6(nsSocketTransportService,

indent

::: netwerk/base/src/nsStreamTransportService.cpp
@@ +69,5 @@
>      // from being modified once the copy is in progress.
>      bool                            mInProgress;
>  };
>  
> +NS_IMPL_ISUPPORTS2(nsInputStreamTransport,

indent

@@ +269,5 @@
>      // from being modified once the copy is in progress.
>      bool                            mInProgress;
>  };
>  
> +NS_IMPL_ISUPPORTS2(nsOutputStreamTransport,

indent

@@ +453,5 @@
>          obsSvc->AddObserver(this, "xpcom-shutdown-threads", false);
>      return NS_OK;
>  }
>  
> +NS_IMPL_ISUPPORTS3(nsStreamTransportService,

indent

::: netwerk/base/src/nsUDPServerSocket.cpp
@@ +562,5 @@
>    nsMainThreadPtrHandle<nsIUDPServerSocketListener> mListener;
>    nsCOMPtr<nsIEventTarget> mTargetThread;
>  };
>  
> +NS_IMPL_ISUPPORTS1(ServerSocketListenerProxy,

indent

::: netwerk/base/src/nsURLParsers.cpp
@@ +33,5 @@
>  //----------------------------------------------------------------------------
>  
>  // The URL parser service does not have any internal state; however, it can
>  // be called from multiple threads, so we must use a threadsafe AddRef and
>  // Release implementation.

this comment is now superfulous and confusing - please remove it

::: netwerk/cache/nsCacheEntryDescriptor.cpp
@@ +69,5 @@
>      nsCOMPtr<nsIThread>              mThread;
>  };
>  
>  
> +NS_IMPL_ISUPPORTS2(nsCacheEntryDescriptor,

indent

::: netwerk/dns/nsIDNService.cpp
@@ +45,5 @@
>  // nsIDNService
>  //-----------------------------------------------------------------------------
>  
>  /* Implementation file */
> +NS_IMPL_ISUPPORTS3(nsIDNService,

indent

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +61,5 @@
>  //-----------------------------------------------------------------------------
>  // RemoteOpenFileChild
>  //-----------------------------------------------------------------------------
>  
> +NS_IMPL_ISUPPORTS3(RemoteOpenFileChild,

indent

::: netwerk/protocol/device/nsDeviceProtocolHandler.cpp
@@ +11,5 @@
>  #include "nsSimpleURI.h"
>  
>  //-----------------------------------------------------------------------------
>  
> +NS_IMPL_ISUPPORTS1(nsDeviceProtocolHandler,

indent

::: netwerk/protocol/file/nsFileProtocolHandler.cpp
@@ +50,5 @@
>  {
>      return NS_OK;
>  }
>  
> +NS_IMPL_ISUPPORTS3(nsFileProtocolHandler,

indent

::: netwerk/protocol/ftp/nsFtpProtocolHandler.cpp
@@ +93,5 @@
>  
>      gFtpHandler = nullptr;
>  }
>  
> +NS_IMPL_ISUPPORTS4(nsFtpProtocolHandler,

indent

::: netwerk/protocol/http/nsHttpActivityDistributor.cpp
@@ +60,5 @@
>  
>      ObserverArray mObservers;
>  };
>  
> +NS_IMPL_ISUPPORTS2(nsHttpActivityDistributor,

indent

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1518,1 @@
>                                nsIInputStreamCallback,

indent

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2556,5 @@
>  
>  //////////////////////// nsHalfOpenSocket
>  
>  
> +NS_IMPL_ISUPPORTS4(nsHttpConnectionMgr::nsHalfOpenSocket,

indent

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1501,5 @@
>  //-----------------------------------------------------------------------------
>  // nsHttpHandler::nsISupports
>  //-----------------------------------------------------------------------------
>  
> +NS_IMPL_ISUPPORTS6(nsHttpHandler,

indent

@@ +1938,5 @@
>  //-----------------------------------------------------------------------------
>  // nsHttpsHandler implementation
>  //-----------------------------------------------------------------------------
>  
> +NS_IMPL_ISUPPORTS5(nsHttpsHandler,

indent

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1699,5 @@
>      }
>      return count;
>  }
>  
> +NS_IMPL_QUERY_INTERFACE2(nsHttpTransaction,

indent

::: netwerk/protocol/res/nsResProtocolHandler.cpp
@@ +198,5 @@
>  //----------------------------------------------------------------------------
>  // nsResProtocolHandler::nsISupports
>  //----------------------------------------------------------------------------
>  
> +NS_IMPL_ISUPPORTS3(nsResProtocolHandler,

indent

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +56,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +NS_IMPL_ISUPPORTS11(WebSocketChannel,

indent

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +61,5 @@
>                           public nsIInterfaceRequestor,
>                           public nsIChannelEventSink
>  {
>  public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

WebsocketChannel.cpp also uses PR_ATOMIC_* if you want to convert that too

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +94,5 @@
>  nsWyciwygChannel::~nsWyciwygChannel() 
>  {
>  }
>  
> +NS_IMPL_ISUPPORTS7(nsWyciwygChannel,

indent

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +271,5 @@
>  
>    disconnect_all();
>  }
>  
> +NS_IMPL_ISUPPORTS1(DataChannelConnection,

indent

::: netwerk/streamconv/converters/nsHTTPCompressConv.cpp
@@ +16,5 @@
>  #include "nsStringStream.h"
>  #include "nsComponentManagerUtils.h"
>  
>  // nsISupports implementation
> +NS_IMPL_ISUPPORTS3(nsHTTPCompressConv,

indent

::: netwerk/system/win32/nsNotifyAddrListener.cpp
@@ +50,5 @@
>          sNetshell = nullptr;
>      }
>  }
>  
> +NS_IMPL_ISUPPORTS3(nsNotifyAddrListener,

indent

::: netwerk/test/TestHttp.cpp
@@ +95,5 @@
>      MyNotifications() { }
>      virtual ~MyNotifications() {}
>  };
>  
> +NS_IMPL_ISUPPORTS2(MyNotifications,

indent

::: netwerk/test/TestPageLoad.cpp
@@ +204,5 @@
>      MyNotifications() { }
>      virtual ~MyNotifications() {}
>  };
>  
> +NS_IMPL_ISUPPORTS2(MyNotifications,

indent

::: netwerk/test/TestSocketTransport.cpp
@@ +114,5 @@
>      nsCString mBuf;
>      uint32_t  mWriteOffset;
>  };
>  
> +NS_IMPL_ISUPPORTS2(MyHandler,

indent

::: netwerk/test/TestStreamTransport.cpp
@@ +161,5 @@
>      nsCOMPtr<nsIAsyncOutputStream> mOutput;
>      nsresult                       mInputCondition;
>  };
>  
> +NS_IMPL_ISUPPORTS2(MyCopier,

indent

::: netwerk/test/TestUDPServerSocket.cpp
@@ +52,5 @@
>  
>    nsresult mResult;
>  };
>  
> +NS_IMPL_ISUPPORTS1(UDPListener,

indent

::: netwerk/wifi/nsWifiMonitor.cpp
@@ +23,5 @@
>  #if defined(PR_LOGGING)
>  PRLogModuleInfo *gWifiMonitorLog;
>  #endif
>  
> +NS_IMPL_ISUPPORTS3(nsWifiMonitor,

indent
Attachment #776177 - Flags: review?(mcmanus) → review+
Comment on attachment 776166 [details] [diff] [review]
Part 3f: Use NS_DECL_THREADSAFE_ISUPPORTS in dom/

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

r=me with indentation fixed

::: dom/file/ArchiveZipFile.cpp
@@ +86,2 @@
>                                nsIInputStream,
>                                nsISeekableStream)

nit: Fix indentation

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +189,1 @@
>                                                       mozIStorageProgressHandler)

nit: Fix indentation

::: dom/indexedDB/CheckPermissionsHelper.cpp
@@ +95,2 @@
>                                                        nsIInterfaceRequestor,
>                                                        nsIObserver)

nit: Fix indentation

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +615,1 @@
>                                nsIRunnable)

nit: Fix indentation

@@ +687,1 @@
>                                nsIRunnable)

nit: Fix indentation

::: dom/indexedDB/TransactionThreadPool.cpp
@@ +570,1 @@
>                                nsIRunnable)

nit: Fix indentation

@@ +669,1 @@
>                                nsIThreadPoolListener)

nit: fix indentation

::: dom/ipc/ContentParent.cpp
@@ +1457,3 @@
>                                nsIObserver,
>                                nsIThreadObserver,
>                                nsIDOMGeoPositionCallback)

nit: fix indentation

::: dom/plugins/base/nsPluginDirServiceProvider.cpp
@@ +181,1 @@
>                                nsIDirectoryServiceProvider)

nit: Fix indentation

::: dom/quota/CheckQuotaHelper.cpp
@@ +141,2 @@
>                                                  nsIInterfaceRequestor,
>                                                  nsIObserver)

nit: Fix indentation

::: dom/quota/QuotaManager.cpp
@@ +2147,2 @@
>                                nsIRunnable,
>                                nsIQuotaRequest)

nit: Fix indentation

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +48,3 @@
>                                nsIGeolocationProvider,
>                                nsIRILDataCallback,
>                                nsISettingsServiceCallback)

nit: Fix Indentation

::: dom/system/gonk/nsVolumeService.cpp
@@ +43,2 @@
>                                nsIVolumeService,
>                                nsIDOMMozWakeLockListener)

nit: Fix Indentation

::: dom/workers/ScriptLoader.cpp
@@ +524,1 @@
>                                                      nsIStreamLoaderObserver)

nit: Fix identation

::: dom/workers/WorkerPrivate.cpp
@@ +1888,1 @@
>                                nsIMemoryMultiReporter)

nit: Fix indentation
Attachment #776166 - Flags: review?(dhylands) → review+
Attachment #776181 - Flags: review?(mak77) → review+
Comment on attachment 776176 [details] [diff] [review]
Part 3p: Use NS_DECL_THREADSAFE_ISUPPORTS in modules/

seems like a reasonable mechanical change
Attachment #776176 - Flags: review?(taras.mozilla) → review+
(Assignee)

Updated

6 years ago
Attachment #776170 - Flags: review?(cjones.bugs) → review?(justin.lebar+bug)

Updated

6 years ago
Attachment #776180 - Flags: review?(mwu) → review+
Comment on attachment 776174 [details] [diff] [review]
Part 3n: Use NS_DECL_THREADSAFE_ISUPPORTS in js/

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

I didn't look too closely, but this looks fine. I don't think it needs brendan's review.

::: js/xpconnect/src/XPCComponents.cpp
@@ +2794,5 @@
>  #include "nsIURI.h"
>  #include "nsNetUtil.h"
>  const char kScriptSecurityManagerContractID[] = NS_SCRIPTSECURITYMANAGER_CONTRACTID;
>  
> +NS_IMPL_ISUPPORTS3(SandboxPrivate,

Weird indentation

::: js/xpconnect/src/nsXPConnect.cpp
@@ +58,1 @@
>                                nsIXPConnect,

Indentation
Attachment #776174 - Flags: review?(brendan)
Attachment #776174 - Flags: review?(bobbyholley+bmo)
Attachment #776174 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #776173 - Flags: review?(cjones.bugs) → review?(bent.mozilla)
Comment on attachment 776175 [details] [diff] [review]
Part 3o: Use NS_DECL_THREADSAFE_ISUPPORTS in media/

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

r+, assuming the three identified nits will be addressed. Thanks!

::: media/mtransport/nr_timer.cpp
@@ +88,5 @@
>    std::string function_;
>    int line_;
>  };
>  
>  // We're going to release ourself in the callback, so we need to be threadsafe

Please move this "threadsafe" comment up to just before line 73.

::: media/mtransport/transportlayerloopback.cpp
@@ +112,1 @@
>                                nsITimerCallback)

Please fix the indenting here.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +197,2 @@
>                                IPeerConnectionObserver,
>                                nsISupportsWeakReference)

Please fix the indenting here.
Attachment #776175 - Flags: review?(adam) → review+
Comment on attachment 776186 [details] [diff] [review]
Part 3z: Use NS_DECL_THREADSAFE_ISUPPORTS in xpfe/

> class WebShellWindowTimerCallback MOZ_FINAL : public nsITimerCallback
> {
> public:
>   WebShellWindowTimerCallback(nsWebShellWindow* aWindow)
>     : mWindow(aWindow)
>   {}
> 
>-  NS_DECL_ISUPPORTS
>+  NS_DECL_THREADSAFE_ISUPPORTS
[Huh? This is a timer callback. Why does it need to be threadsafe? Because neither danm or scc understood timers, I guess...]

>+NS_IMPL_ADDREF(WebShellWindowTimerCallback)
>+NS_IMPL_RELEASE(WebShellWindowTimerCallback)
>+NS_IMPL_QUERY_INTERFACE1(WebShellWindowTimerCallback,
>                                     nsITimerCallback)
Seeing as you need to fix the indentation anyway, you might as well go the whole hog with NS_IMPL_ISUPPORTS1(WebShellWindowTimerCallback, nsITimerCallback) [no idea why jlebar didn't do that in the first place].

>-NS_IMPL_THREADSAFE_ADDREF(nsXULWindow)
>-NS_IMPL_THREADSAFE_RELEASE(nsXULWindow)
>+NS_IMPL_ADDREF(nsXULWindow)
>+NS_IMPL_RELEASE(nsXULWindow)
[No, I don't know why this is threadsafe either...]
Attachment #776186 - Flags: review?(neil) → review+
Attachment #776171 - Flags: review?(joe) → review+
(In reply to Justin Lebar from comment #20)
> (In reply to Joshua Cranmer from comment #15)
> > I can't find an
> > alternative implementation of these checks that would allow me to omit
> > NS_DECL_OWNINGTHREAD and keep the assertion message (retaining the class
> > name is effectively impossible if I try to use methods instead).
> 
> I suppose you could add a NS_DECL_FAKE_OWNINGTHREAD, that declares a member
> that has the same C++ interface as nsAutoOwningThread, but is actually
> empty.  But since NS_DECL_OWNINGTHREAD doesn't do anything in release
> builds, I guess it doesn't matter.

Some sort of SFINAE on the presence of _mOwningThread perhaps?
(In reply to Justin Lebar [:jlebar] from comment #58)
> You probably want me or mounir for hal; I don't think cjones is reviewing
> much these days.
> 

I'm still available for reviews, but if you need quick turnaround I'm not the best person to ask.
Attachment #776170 - Flags: review?(justin.lebar+bug) → review+
Attachment #776188 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 71

6 years ago
Comment on attachment 776172 [details] [diff] [review]
Part 3l: Use NS_DECL_THREADSAFE_ISUPPORTS in intl/

[Marking, per comment 59 and my own tracking sanity.]
Attachment #776172 - Flags: review?(smontagu) → review+
(Assignee)

Comment 72

6 years ago
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #57)
> ::: security/manager/ssl/src/nsNSSCallbacks.cpp
> @@ +303,5 @@
> >  
> >  void
> >  nsNSSHttpRequestSession::AddRef()
> >  {
> > +  ++mRefCount;
> 
> Shouldn't this class just be derived from
> AtomicRefCounted<nsNSSHttpRequestSession> instead?

Hmm, that should work. An earlier iteration of this patch tried to aggressively move the handcoded atomic refcounting stuff to the common macros and blew up in my face (probably because I got blurry about niggly details in Release after changing the 100th invocation), so I instead moved to a conservative conversion approach. If it passes my local tests, I'll make the change.

(In reply to Patrick McManus [:mcmanus] from comment #63)
> WebsocketChannel.cpp also uses PR_ATOMIC_* if you want to convert that too

Due to the already high amount of code changes involved in this bug, and the slight risk involved with changing PR_ATOMIC->mozilla::Atomic to begin with, I've decided to leave the other changes into another bug.

(In reply to neil@parkwaycc.co.uk from comment #68)
> [Huh? This is a timer callback. Why does it need to be threadsafe? Because
> neither danm or scc understood timers, I guess...]

I noticed a lot of the timer callbacks were marked as threadsafe; given that the timer uses another thread, it's possible that people at one time thought all timer callbacks needed to be threadsafe.

> [No, I don't know why this is threadsafe either...]

This is why I like to think of this as the "Really? That is threadsafe?" bug...

(In reply to neil@parkwaycc.co.uk from comment #69)
> (In reply to Justin Lebar from comment #20)
> > (In reply to Joshua Cranmer from comment #15)
> > > I can't find an
> > > alternative implementation of these checks that would allow me to omit
> > > NS_DECL_OWNINGTHREAD and keep the assertion message (retaining the class
> > > name is effectively impossible if I try to use methods instead).
> > 
> > I suppose you could add a NS_DECL_FAKE_OWNINGTHREAD, that declares a member
> > that has the same C++ interface as nsAutoOwningThread, but is actually
> > empty.  But since NS_DECL_OWNINGTHREAD doesn't do anything in release
> > builds, I guess it doesn't matter.
> 
> Some sort of SFINAE on the presence of _mOwningThread perhaps?

The assertion message needs to be a string literal passed into MOZ_ASSERT, which means anything that requires code outside of NS_IMPL_ADDREF/NS_IMPL_RELEASE for the checking isn't going to work.

Comment 73

6 years ago
Comment on attachment 776178 [details] [diff] [review]
Part 3r: Use NS_DECL_THREADSAFE_ISUPPORTS in rdf/

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

I haven't looked at xpcom code in too long to do an honest review. Benjamin is still as much a peer of rdf as I'm the owner, and looked at other patches in this flock.

Benjamin, can you do this review, too? Sorry.
Attachment #776178 - Flags: review?(axel) → review?(benjamin)
Attachment #776182 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 776162 [details] [diff] [review]
Part 3c: Use NS_DECL_THREADSAFE_ISUPPORTS in chrome/

Please reindent in nsChromeProtocolHandler.cpp
Attachment #776162 - Flags: review?(benjamin) → review+
Comment on attachment 776167 [details] [diff] [review]
Part 3g: Use NS_DECL_THREADSAFE_ISUPPORTS in embedding/

Please remove the hard tab in mac/nsPrintProgress.h and in the windows fork as well.
Attachment #776167 - Flags: review?(benjamin) → review+

Updated

6 years ago
Attachment #776168 - Flags: review?(benjamin) → review+

Updated

6 years ago
Attachment #776176 - Flags: review?(benjamin) → review+
Comment on attachment 776178 [details] [diff] [review]
Part 3r: Use NS_DECL_THREADSAFE_ISUPPORTS in rdf/

Does mailnews use RDF off the main thread? None of this code is actually threadsafe, and so I suspect that the correct answer here is to stop using threadsafe refcounting altogether in this code.
Attachment #776178 - Flags: review?(benjamin) → review-
(Assignee)

Comment 77

6 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #76)
> Comment on attachment 776178 [details] [diff] [review]
> Part 3r: Use NS_DECL_THREADSAFE_ISUPPORTS in rdf/
> 
> Does mailnews use RDF off the main thread? None of this code is actually
> threadsafe, and so I suspect that the correct answer here is to stop using
> threadsafe refcounting altogether in this code.

nsImapMailFolder, which inherits from nsRDFResource, needs to be marked threadsafe as it is legitimately used off the main thread. I know how to get mailnews off of inheriting from nsRDFResource, but that will take a few months; until then, it needs to be threadsafe.

Everything else I *think* could be dropped, but it would require a run on the try server first.
Comment on attachment 776185 [details] [diff] [review]
Part 3y: Use NS_DECL_THREADSAFE_ISUPPORTS in xpcom/

Please reindent all multiline NS_IMPL_THREADSAFE_ISUPPORTS rewrites in this patch.
Attachment #776185 - Flags: review?(benjamin) → review+
(Assignee)

Comment 79

6 years ago
This clears thread-safety on all but nsRDFResource, which needs to be threadsafe until comm-central stops abusing them. This appears to pass tests locally (in a debug build).
Attachment #778538 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Attachment #776166 - Flags: review?(bugs)
Comment on attachment 776166 [details] [diff] [review]
Part 3f: Use NS_DECL_THREADSAFE_ISUPPORTS in dom/

Indentation in 
* ArchiveInputStream
* AsyncConnectionHelper
* AsyncDeleteFileRunnable
* GetFileReferencesHelper
* TransactionThreadPool::TransactionQueue
* TransactionThreadPoolListener
* ContentParent
* nsPluginDirServiceProvider
* CheckQuotaHelper
* AsyncUsageRunnable
* GonkGPSGeolocationProvider
* nsVolumeService
* ScriptLoaderRunnable
* WorkerPrivate::MemoryReporter

Hrm, could you file a followup to make ProgressRunnable, CloseRunnable, 
DestroyRunnable and FinishHelper to inherit nsRunnable (which takes care of refcounting)
Also AsyncConnectionHelper, CheckPermissionsHelper, CommitHelper, AsyncDeleteFileRunnable, GetFileReferencesHelper, OpenDatabaseHelper,
FinishTransactionRunnable, TransactionQueue, CheckQuotaHelper,
AsyncUsageRunnable, WaitForTransactionsToFinishRunnable,
WaitForLockedFilesToFinishRunnable, ScriptLoaderRunnable and WorkerRunnable
should probably just extend nsRunnable.
Attachment #776166 - Flags: review?(jonas)
Attachment #776166 - Flags: review?(bugs)
Attachment #776166 - Flags: review+
Comment on attachment 776173 [details] [diff] [review]
Part 3m: Use NS_DECL_THREADSAFE_ISUPPORTS in ipc/

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

::: ipc/glue/RPCChannel.h
@@ +393,4 @@
>        public:
>          RefCountedTask(CancelableTask* aTask)
>          : mTask(aTask)
>          , mRefCnt(0) {}

This should be removed if we're using an auto-refcount.

@@ +406,5 @@
>          }
>  
>        private:
>          CancelableTask* mTask;
> +        ThreadSafeAutoRefCnt mRefCnt;

Actually, is there any reason why this can't just use NS_INLINE_DECL_THREADSAFE_REFCOUNTING?
Attachment #776173 - Flags: review?(bent.mozilla) → review+

Updated

6 years ago
Attachment #778538 - Flags: review?(benjamin) → review+
(Assignee)

Updated

6 years ago
Attachment #776178 - Attachment is obsolete: true
Comment on attachment 776190 [details] [diff] [review]
, Part 一: Use NS_DECL_THREADSAFE_ISUPPORTS in comm-central.

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

I've not reviewed whether or not these all need threadsafe, but I've confirmed the patch maintains the status quo. I'm assuming you've tested it.
Attachment #776190 - Flags: review?(mbanner) → review+
Attachment #776191 - Flags: review?(mbanner) → review+
https://hg.mozilla.org/mozilla-central/rev/955975fb2ca7
https://hg.mozilla.org/mozilla-central/rev/256cf2fd8c6f
https://hg.mozilla.org/mozilla-central/rev/b8a777009843
https://hg.mozilla.org/mozilla-central/rev/47c749323e60
https://hg.mozilla.org/mozilla-central/rev/34216d968a91
https://hg.mozilla.org/mozilla-central/rev/56450c7bbda6
https://hg.mozilla.org/mozilla-central/rev/eb49396e9def
https://hg.mozilla.org/mozilla-central/rev/8ba67ee6a81d
https://hg.mozilla.org/mozilla-central/rev/c073457f6b02
https://hg.mozilla.org/mozilla-central/rev/0f38536304e1
https://hg.mozilla.org/mozilla-central/rev/bc8cc4a1818d
https://hg.mozilla.org/mozilla-central/rev/cd4a15be6ea5
https://hg.mozilla.org/mozilla-central/rev/352cb42a9c7a
https://hg.mozilla.org/mozilla-central/rev/c00e38f175be
https://hg.mozilla.org/mozilla-central/rev/ed39337cc38f
https://hg.mozilla.org/mozilla-central/rev/7df0367427d3
https://hg.mozilla.org/mozilla-central/rev/f4087583ffc1
https://hg.mozilla.org/mozilla-central/rev/96104885ff18
https://hg.mozilla.org/mozilla-central/rev/14bd6018f287
https://hg.mozilla.org/mozilla-central/rev/3bd71784e8a0
https://hg.mozilla.org/mozilla-central/rev/81a52416513e
https://hg.mozilla.org/mozilla-central/rev/f3e3a8389cdf
https://hg.mozilla.org/mozilla-central/rev/bf72f0094528
https://hg.mozilla.org/mozilla-central/rev/58c8a357338c
https://hg.mozilla.org/mozilla-central/rev/050a1e634338
https://hg.mozilla.org/mozilla-central/rev/c027e7494a86
https://hg.mozilla.org/mozilla-central/rev/b3533aba6520
https://hg.mozilla.org/mozilla-central/rev/83c90466810a
https://hg.mozilla.org/mozilla-central/rev/c4d279b10128
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

6 years ago
Depends on: 897503
897053 became the #1 crasher after this merged.. maybe we need a backout while it is sorted out?
sorry - I meant 897503
(In reply to Patrick McManus [:mcmanus] from comment #86)
> 897053 became the #1 crasher after this merged.. maybe we need a backout
> while it is sorted out?

Shouldn't we just audit the network changes from bug 884061?  If there were something wrong with the new atomic refcounting, the whole platform would suffer.  This looks like a mistake in the network changes only.
(In reply to Honza Bambas (:mayhemer) from comment #88)
> (In reply to Patrick McManus [:mcmanus] from comment #86)
> > 897053 became the #1 crasher after this merged.. maybe we need a backout
> > while it is sorted out?
> 
> Shouldn't we just audit the network changes from bug 884061?  If there were
> something wrong with the new atomic refcounting, the whole platform would
> suffer.  This looks like a mistake in the network changes only.

you can if you like, but I know both you and I have other short term goals we are committed to. So I was hoping the author of the patch that caused the regression would do the usual thing of backing it out and sorting out the issue while it is out of the tree.

Comment 90

6 years ago
Joshua, we have an XPCOM component for FFOS that is using NS_INLINE_DECL_THREADSAFE_REFCOUNTING which doesn't compile anymore with this change of yours as ThreadSafeAutoRefCnt is not available if XPCOM_GLUE is available. Seems like on FFOS XPCOM_GLUE is enabled so our code doesn't compile anymore. I can fix the code to use the non thread safe macros but would like to use the thread safe macro. Could you please help?
Flags: needinfo?(Pidgeot18)
(Assignee)

Comment 91

6 years ago
(In reply to Anshul from comment #90)
> Joshua, we have an XPCOM component for FFOS that is using
> NS_INLINE_DECL_THREADSAFE_REFCOUNTING which doesn't compile anymore with
> this change of yours as ThreadSafeAutoRefCnt is not available if XPCOM_GLUE
> is available. Seems like on FFOS XPCOM_GLUE is enabled so our code doesn't
> compile anymore. I can fix the code to use the non thread safe macros but
> would like to use the thread safe macro. Could you please help?

This is best discussed in a followup bug. The reason why the threadsafe macros are unused in that situation is that <atomic> is unusable from such code because the STL wrappers forbid us from using them if XPCOM_GLUE is defined:
#if !defined(XPCOM_GLUE) && !defined(NS_NO_XPCOM) && !defined(MOZ_NO_MOZALLOC)
#  include "mozilla/mozalloc.h"
#else
#  error "STL code can only be used with infallible ::operator new()"
#endif

<atomic> was included in bug 873893, apparently because it's including a header which eventually includes <xutility> and prevents later wrapping code from working, despite the fact that the stuff we use in <atomic> is basically safe. I don't understand our STL-wrapping code to know why this is going on, so you'd need bsmedberg and/or glandium to help, most likely.
Flags: needinfo?(Pidgeot18)
Yes please file a new bug. I cannot think of any reason an XPCOM *component* should be built with XPCOM_GLUE in any case!

Updated

6 years ago
Depends on: 899453
You need to log in before you can comment on or make changes to this bug.