865583 - Use thread-safe ref-counting in the linker

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Mike Hommey [:glandium]]

Bug 830295 added a safeguard to avoid CustomElf destructor recursive calls, but this ends up being racy accross threads in the SIGSEGV handler with on-demand decompression.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Use thread-safe ref-counting in the linker

Note this depends on the thread-safe ref-counting implementation from bug 864035.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 741718 [details] [diff] [review]
Use thread-safe ref-counting in the linker

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

I thought the comments about ThreadSafeRefCounted describing RefCounted<LibHandle, Atomic<int>> were a little confusing, as the classes mentioned in the comment do not line up with the code.  Maybe you want to say something like:

"Specialize RefCounted<...>, which will be inherited by ThreadSafeRefCounted<...>, ..." ?

Or, on second thought, why are you not simply specializing ThreadSafeRefCounted<LibHandle>::{ThreadSafeRefCounted,Release}?  Seems like that'd be a lot easier to describe.

::: mozglue/linker/ElfLoader.h
@@ +206,5 @@
>  };
>  
>  /**
> + * Specialized ThreadSafeRefCounted<LibHandle>::Release. Under normal
> + * operation, when * refCnt reaches 0, the LibHandle is deleted. Its refCnt

Bad comment reflow here for |* refCnt|?

Comment 3

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Or, on second thought, why are you not simply specializing
> ThreadSafeRefCounted<LibHandle>::{ThreadSafeRefCounted,Release}?  Seems like
> that'd be a lot easier to describe.

Specializing the destructor would not replace RefCounted's, which is the intent. As for Release, you can only specialize an explicitly declared member, which it isn't (it's only implicit from inheritance from RefCounted).

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Nathan Froyd (:froydnj) from comment #2)
> > Or, on second thought, why are you not simply specializing
> > ThreadSafeRefCounted<LibHandle>::{ThreadSafeRefCounted,Release}?  Seems like
> > that'd be a lot easier to describe.
> 
> Specializing the destructor would not replace RefCounted's, which is the
> intent.

Ah, and ThreadSafeRefCounted has to call that anyway, so we might as well specialize the base class rather than the derived class?

> As for Release, you can only specialize an explicitly declared
> member, which it isn't (it's only implicit from inheritance from RefCounted).

Bleh.  Would it be too much to ask that we do explicitly declare it so people can do tricks like this if they want to?

Comment 5

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > (In reply to Nathan Froyd (:froydnj) from comment #2)
> > > Or, on second thought, why are you not simply specializing
> > > ThreadSafeRefCounted<LibHandle>::{ThreadSafeRefCounted,Release}?  Seems like
> > > that'd be a lot easier to describe.
> > 
> > Specializing the destructor would not replace RefCounted's, which is the
> > intent.
> 
> Ah, and ThreadSafeRefCounted has to call that anyway, so we might as well
> specialize the base class rather than the derived class?

~ThreadSafeRefCounted would call ~RefCounted and we don't want that.
 
> > As for Release, you can only specialize an explicitly declared
> > member, which it isn't (it's only implicit from inheritance from RefCounted).
> 
> Bleh.  Would it be too much to ask that we do explicitly declare it so
> people can do tricks like this if they want to?

I'd rather only do this if someone else needs it.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Use thread-safe ref-counting in the linker

Refreshed after new version of bug 864035.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 748903 [details] [diff] [review]
Use thread-safe ref-counting in the linker

Carrying over r+

Comment 8

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68ec60cf14a9

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/68ec60cf14a9