garburator rewrites .forget incorrectly

RESOLVED FIXED

Status

()

Core
Rewriting and Analysis
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Benjamin Smedberg, Assigned: (dormant account))

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

10 years ago
automatic-garburator from XPCOMGC:

 95495 -  nsCOMPtr<nsIUnicodeDecoder> decoder;
 95496 +  nsIUnicodeDecoder* decoder = nsnull;
...
 95509 @@ -281,7 +281,7 @@ nsCharsetConverterManager::GetUnicodeDec
 95510    }
 95511    NS_ENSURE_SUCCESS(rv, NS_ERROR_UCONV_NOCONV);
 95512  
 95513 -  decoder.forget(aResult);
 95514 +  decoder;
 95515    return rv;
 95516  }
 95517  


If you want to see all 156k lines, see http://hg.mozilla.org/users/bsmedberg_mozilla.com/xpcomgc-patches/index.cgi/file/2975de671d13/automatic-garburator#l95509

I think the correct rewriting needs to be

-  decoder.forget(aResult);
+  *aResult = decoder; decoder = nsnull;
(Reporter)

Comment 1

10 years ago
I think this may be caused by a new signature for forget() which was added after garburator was written:

http://hg.mozilla.org/mozilla-central/index.cgi/rev/9f2ba4b2a873

So, calls to already_AddRefed<T> comptr.forget() should remain as they were

calls to void comptr.forget(T** outparam) should be rewritten as above.
(Assignee)

Comment 2

10 years ago
How did you find this bug, by debugging or with dmandelin's outparam checker?
Status: NEW → ASSIGNED
(Reporter)

Comment 3

10 years ago
Debugging... I haven't hooked up dmandelin's stuff to XPCOMGC yet.
(Assignee)

Comment 4

10 years ago
pushed, i only tested on my testcase, it *should* work correctly.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.