Closed Bug 1248023 Opened 4 years ago Closed 4 years ago

Update uses of NS_ProxyRelease to take into account the changes in Bug 1164581

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(firefox47 affected)

RESOLVED FIXED
Thunderbird 47.0
Tracking Status
firefox47 --- affected

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 1 obsolete file)

(From bug 1164581 comment #0)
> Right now it accepts nsISupports*, nsCOMPtr<T>&, and nsRefPtr<T>&. This is a
> huge footgun. If, say, somebody uses one of our other smart pointer classes
> (like RefPtr), they'll end up with the nsISupports* overload, and the
> pointer will be freed twice.
> 
> Instead, we should have exactly declaration, which takes
> already_AddRefed<T>, and fix up callers to either .forget() their
> smartpointer or create an already_AddRefed out of a raw pointer via
> dont_AddRef.
Attached patch Proposed bustage fix (obsolete) — Splinter Review
I based this patch on http://hg.mozilla.org/mozilla-central/rev/c75f253f4335
Attachment #8718956 - Flags: feedback?(mozilla)
Attachment #8718956 - Flags: feedback?(frgrahl)
Comment on attachment 8718956 [details] [diff] [review]
Proposed bustage fix

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

Hey, great I've looked at this briefly but opted for a quiet Friday night ;-)
Can NS_ReleaseOnMainThread() always be used instead of NS_ProxyRelease(mainThread, ...)?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +978,5 @@
>        {
>          // Proxy the release of the channel to the main thread.  This is something
>          // that the xpcom proxy system should do for us!
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +        NS_ProxyRelease(mainThread, m_mockChannel.forget());

Why not NS_ReleaseOnMainThread here also? Also four more below.
Attachment #8718956 - Flags: feedback?(mozilla) → feedback+
Comment on attachment 8718956 [details] [diff] [review]
Proposed bustage fix

(In reply to Jorg K (GMT+1) from comment #2)

> Can NS_ReleaseOnMainThread() always be used instead of
> NS_ProxyRelease(mainThread, ...)?

I have no idea. I looked at http://hg.mozilla.org/mozilla-central/rev/c75f253f4335
When they use ReleaseOnMainThread I use ReleaseOnMainThread. When they use NS_ProxyRelease(mainThread, ...) I use NS_ProxyRelease(mainThread, ...)

Maybe we should ask Bobby Holley (busy)
Flags: needinfo?(bobbyholley)
Attachment #8718956 - Flags: feedback?(bobbyholley)
(In reply to Philip Chee from comment #3)
> I have no idea.
Well, if the target is the main tread, they use ReleaseOnMainThread.

The only times they still use ProxyRelease is when the target is not the main thread, like here:
http://hg.mozilla.org/mozilla-central/rev/c75f253f4335#l6.13
http://hg.mozilla.org/mozilla-central/rev/c75f253f4335#l15.16
http://hg.mozilla.org/mozilla-central/rev/c75f253f4335#l20.25
http://hg.mozilla.org/mozilla-central/rev/c75f253f4335#l21.18
etc.

However, they seem to have missed:
http://hg.mozilla.org/mozilla-central/rev/c75f253f4335#l8.16
http://hg.mozilla.org/mozilla-central/rev/c75f253f4335#l35.32

I'd go with ReleaseOnMainThread always and see what happens ;-)
Attachment #8718956 - Flags: feedback?(aidin)
Comment on attachment 8718956 [details] [diff] [review]
Proposed bustage fix

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

> Can NS_ReleaseOnMainThread() always be used instead of NS_ProxyRelease(mainThread, ...)?

Yes.
Attachment #8718956 - Flags: feedback?(bobbyholley)
Attachment #8718956 - Flags: feedback?(aidin)
This is Philip's/Ratty's patch but using NS_ReleaseOnMainThread everywhere.
(Clearing NI for Bobby Holley since he's already visited the bug).

Sorry about the r? SPAM, one review will do.
Attachment #8718956 - Attachment is obsolete: true
Attachment #8718956 - Flags: feedback?(frgrahl)
Flags: needinfo?(bobbyholley)
Attachment #8719050 - Flags: review?(rkent)
Attachment #8719050 - Flags: review?(Pidgeot18)
Attachment #8719050 - Flags: review?(Pidgeot18) → review+
https://hg.mozilla.org/comm-central/rev/01d0cf9ebf8d95bfb7a15e5b4bd59183f5bb3968
Bug 1248023 - Update uses of NS_ProxyRelease to take into account the changes in Bug 1164581. r=jcranmer a=aleth CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
You need to log in before you can comment on or make changes to this bug.