Closed Bug 865583 Opened 7 years ago Closed 7 years ago

Use thread-safe ref-counting in the linker

Categories

(Core :: mozglue, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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+
(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?
(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.
Refreshed after new version of bug 864035.
Attachment #741718 - Attachment is obsolete: true
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.