991812 - Remove uses of RefCounted from code that only lives in Gecko

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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.

Comment 1

[(no longer active)]

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.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for AtomicRefCounted

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

This landed right? I see 88ee33546b3a listed in hg log, so I assume so?

Comment 4

[(no longer active)]

(In reply to comment #3)
> This landed right? I see 88ee33546b3a listed in hg log, so I assume so?

It bounced.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: A few more

Remove a few more.

Comment 7

[(no longer active)]

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!

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: B2G

And in b2g code.

Comment 9

[Nathan Froyd [:froydnj]]

Are you going to make a dev-platform post about this when all is said and done?

Comment 10

[(no longer active)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/376496720a0e

https://tbpl.mozilla.org/php/getParsedLog.php?id=37777059&tree=Mozilla-Inbound

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Sigh.  This built on Friday.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b54fdf8bb486
https://hg.mozilla.org/mozilla-central/rev/076cdafa93f5
https://hg.mozilla.org/mozilla-central/rev/0c55cac422f2
https://hg.mozilla.org/mozilla-central/rev/3d7bfc590ccd

Comment 14

[Dave Hylands [:dhylands]]

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.

Comment 15

[(no longer active)]

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

Comment 16

[Dave Hylands [:dhylands]]

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.

Comment 17

[(no longer active)]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 19

[(no longer active)]

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!

Comment 20

[(no longer active)]

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

Comment 21

[Dave Hylands [:dhylands]]

I filed bug 999864