Use thread-safe ref-counting in the linker

RESOLVED FIXED in mozilla24

Status

()

Core
mozglue
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla24
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 741718 [details] [diff] [review]
Use thread-safe ref-counting in the linker

Note this depends on the thread-safe ref-counting implementation from bug 864035.
Attachment #741718 - Flags: review?(nfroyd)
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|?
Attachment #741718 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 3

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

Comment 5

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

Comment 6

5 years ago
Created attachment 748903 [details] [diff] [review]
Use thread-safe ref-counting in the linker

Refreshed after new version of bug 864035.
(Assignee)

Updated

5 years ago
Attachment #741718 - Attachment is obsolete: true
(Assignee)

Comment 7

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

Carrying over r+
Attachment #748903 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/68ec60cf14a9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.