Closed Bug 991812 Opened 6 years ago Closed 6 years ago

Remove uses of RefCounted from code that only lives in Gecko

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: khuey, Assigned: khuey)

References

(Depends on 1 open bug)

Details

Attachments

(4 files)

Attached patch PatchSplinter Review
Rather than figure out all the ways in which this stuff is not at parity with the existing stuff, let's just use the thing that already works.
Attachment #8401449 - Flags: review?(ehsan)
Comment on attachment 8401449 [details] [diff] [review]
Patch

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

::: gfx/layers/Compositor.h
@@ +141,2 @@
>  {
> +  NS_INLINE_DECL_REFCOUNTING(CompositorBackendSpecificData)

This class is very weird, and is not directly usable with this change.  Also, as far as I can tell, it's actually dead code.  If this compiles, I guess it's fine, but please file a follow-up to remove it completely.
Attachment #8401449 - Flags: review?(ehsan) → review+
Attachment #8404209 - Flags: review?(ehsan) → review+
This landed right? I see 88ee33546b3a listed in hg log, so I assume so?
(In reply to comment #3)
> This landed right? I see 88ee33546b3a listed in hg log, so I assume so?

It bounced.
Attached patch A few moreSplinter Review
Remove a few more.
Attachment #8405007 - Flags: review?(ehsan)
Comment on attachment 8405007 [details] [diff] [review]
A few more

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

::: dom/devicestorage/DeviceStorageFileDescriptor.h
@@ +9,4 @@
>  
>  #include "mozilla/ipc/FileDescriptor.h"
>  
> +struct DeviceStorageFileDescriptor MOZ_FINAL

Please either make this a class again or switch the forward declarations to be a struct as well (MSVC will barf at this).

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +125,2 @@
>      if (cacheEntry->mStorageName.Equals(aStorageName)) {
> +      nsRefPtr<CacheEntry> addRefedCacheEntry = cacheEntry;

Nice!
Attachment #8405007 - Flags: review?(ehsan) → review+
Attached patch B2GSplinter Review
And in b2g code.
Attachment #8405691 - Flags: review?(ehsan)
Are you going to make a dev-platform post about this when all is said and done?
Comment on attachment 8405691 [details] [diff] [review]
B2G

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

::: dom/system/gonk/VolumeManager.h
@@ +82,2 @@
>  
> +  typedef nsTArray<RefPtr<Volume>> VolumeArray;

nsRefPtr please.
Attachment #8405691 - Flags: review?(ehsan) → review+
Sigh.  This built on Friday.
One place this change seems to fall down is when using LazyIdleThread.

The LazyIdleThread only ever has one thread at a time, but that thread can go away and a new thread can come back at some future time.

This in turn, can cause http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#57 to fire.

I ran into this in bug 998097. The easy fix seems to just be to make the ref count thread safe.

Any other uses of LazyIdleThread and non-threadsafe ref counts may also trip over this.
(In reply to comment #14)
> One place this change seems to fall down is when using LazyIdleThread.

Yeah I think we should make that refcount threadsafe as well...  Please file a follow-up?
1 - So - should refcounts used by LazyIdleThread's be threadsafe?

2 - Or should we consider it a bug that non-threadsafe refcounts cause an assert?

I have some test code which demonstrates that non-threadsafe refcounts assert when used from LazyIdleThread. If 2 is what you want, then I'll file a followup bug and attach the test code.

If 1 is the case, then bug 998097 is already filed, and I don't need a followup bug.
Flags: needinfo?(ehsan)
(In reply to Dave Hylands [:dhylands] from comment #16)
> 1 - So - should refcounts used by LazyIdleThread's be threadsafe?
> 
> 2 - Or should we consider it a bug that non-threadsafe refcounts cause an
> assert?
> 
> I have some test code which demonstrates that non-threadsafe refcounts
> assert when used from LazyIdleThread. If 2 is what you want, then I'll file
> a followup bug and attach the test code.
> 
> If 1 is the case, then bug 998097 is already filed, and I don't need a
> followup bug.

Wait a second, I don't think I correctly understand the issue.  Do you mind attaching a stack trace to the assertion that you're seeing please?
Flags: needinfo?(ehsan) → needinfo?(dhylands)
Do we need a mechanism to say that certain non-threadsafely-reference-counted objects can be transferred from one thread to another?  (I've thought for a while that we might.  It would require changing the DEBUG-only owning thread member variable from (should be but isn't) const to Atomic, which might be expensive enough that we'd want different variant of nsAutoOwningThread for only classes that need this.)
So we chatted on IRC about this.  This is a sucky problem.  I guess our only two options here are either to use thread-safe refcounting or introduce a third special type of non-checked non-threadsafe refcounting.  I'd take the former.

Can you please file a follow-up to fix all of the remaining cases for other consumers of LazyIdleThread?  Thankfully that class seems to only be used in 5 places.  Thanks!
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #18)
> Do we need a mechanism to say that certain
> non-threadsafely-reference-counted objects can be transferred from one
> thread to another?  (I've thought for a while that we might.  It would
> require changing the DEBUG-only owning thread member variable from (should
> be but isn't) const to Atomic, which might be expensive enough that we'd
> want different variant of nsAutoOwningThread for only classes that need
> this.)

I really fear that might be giving people too much rope...
I filed bug 999864
Flags: needinfo?(dhylands)
Depends on: 999880
Depends on: 999883
Depends on: 999884
Depends on: 999888
Depends on: 1000822
Depends on: 1004473
Depends on: 1004564
Depends on: 1005036
Depends on: 1028264
You need to log in before you can comment on or make changes to this bug.