860886 - Make MOZ_CC_LOG_DIRECTORY work again

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 736441 [details] [diff] [review]
Patch (v1)

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

Thanks for fixing this!

Comment 2

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2d9eb84633

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa2d9eb84633

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

What is it exactly that this patch does, and what's the right fix to this code for bug 859817 to work?  You did exactly the opposite of what my patches try to do.  This will build with bug 859817 if I add a dont_AddRef, but I'd be very interested to know why an nsCOMPtr and getter_AddRefs doesn't work here.

Comment 5

[(no longer active)]

getter_AddRefs nulls out the nsCOMPtr before passing it as nsIFoo** to the callee.  This means that here, nsMemoryInfoDumper::OpenTempFile would never see a non-null argument (note that its second argument is in/out) and therefore would always disregard the nsIFile* opened from MOZ_CC_LOG_DIRECTORY.

I think the right way to fix this is to make OpenTempFile not expect an in/out argument, and just make it return an already_AddRefed<nsIFile>.  But I'm not sure why this code is breaking your changes in bug 859817 in the first place, since it theory I should only be using raw interface management, right?

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I figured that must be the reason after I commented.  The thing that broke stuff is "return logFile;" -- it needs to be "return dont_AddRef(logFile);" or something.  I'll see if I can fix it properly, or alternatively I'll work around it.

Comment 7

[(no longer active)]

(In reply to comment #6)
> I figured that must be the reason after I commented.  The thing that broke
> stuff is "return logFile;" -- it needs to be "return dont_AddRef(logFile);" or
> something.  I'll see if I can fix it properly, or alternatively I'll work
> around it.

I see, let me know if you want me to fix it for you, would be happy to!

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I'm just adding a dont_AddRef() in the return for now, since properly fixing the function looks like too much trouble for my purposes.  That works for me -- if you change this code back to the way it was, the cast can be removed.