Closed Bug 1000922 Opened 8 years ago Closed 6 years ago

Use nsMainThreadPtrHandle instead of passing around already_AddRefed and calling forget everywhere

Categories

(Toolkit :: OS.File, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jdm, Assigned: chmanchester, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file)

The code in NativeOSFileInternals.cpp looks more complicated than necessary in my quick skim. As far as I can tell, we're passing around references to main-thread-only callback interfaces as already_AddRefed, storing them in nsCOMPtr and then forgetting them before a release happens, all to avoid forbidden refcounting on XPConnect wrappers. We can do this more simply and correctly by storing nsMainThreadPtrHandle types in the nsRunnable derivatives instead, and that will remove the need for all of the forget calls too.
Oh, we have such a thing as nsMainThreadPtrHandle? I couldn't find anything such when I wrote NativeOSFileInternals.cpp. Definitely, this can be used to simplify the code.
I could work on that if needed, even though it looks like a good bug for mentoring!

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?" - I'll be away on April 9th-16th) from comment #1)
> Oh, we have such a thing as nsMainThreadPtrHandle? I couldn't find anything
> such when I wrote NativeOSFileInternals.cpp. Definitely, this can be used to
> simplify the code.
You're always welcome to mentor it if you'd like!
Mentor: josh
Whiteboard: [lang=c++]
Bug 1000922 - Use nsMainThreadPtrHandle instead of already_AddRefed and forget for callbacks in NativeOSFileInternals.cpp r=jdm
Attachment #8653024 - Flags: review?(josh)
Comment on attachment 8653024 [details]
MozReview Request: Bug 1000922 - Use nsMainThreadPtrHandle instead of already_AddRefed and forget for callbacks in NativeOSFileInternals.cpp r=jdm

https://reviewboard.mozilla.org/r/16445/#review15771

Looks good! Thanks for doing this work!
Attachment #8653024 - Flags: review?(josh) → review+
https://hg.mozilla.org/mozilla-central/rev/8b3d5b3dad0e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.