361608 - possible leak in Safari profile migrator

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

This is related to bug 344284: if ParseBookmarksFolder fails, we return early without calling ::CFRelease(safariBookmarks). Doesn't this leak safariBookmarks? Should we have helper classes to hold these objects and avoid the manual refcounting (like webkit's RetainPtr)?

Comment 1

[(not reading, please use [e-mail] instead)]

more related to bug #361624.

thanks for spotting this and logging it, gavin.

Comment 2

[Asaf Romano (gone)]

Attachment: patch

Comment 3

[Mark Mentovai]

Comment on attachment 246568 [details] [diff] [review]
patch

|return rv| will return an uninitialized value if:

- MOZ_PLACES_BOOKMARKS is not defined
- aReplace is true
- some other failure occurs before reaching ParseBookmarksFolder

I suggest explicit initialization of rv in the !defined(MOZ_PLACES_BOOKMARKS ) code by using the two-argument form of do_GetService both times, adding NS_ENSURE_SUCCESS,  and getting rid of NS_ENSURE_TRUE.  You can choose an alternate reasonable solution to the problem if you like something else better.

Comment 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Asaf Romano (gone)]

mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp 1.34

Comment 6

[Jay Patel [:jay]]

Comment on attachment 246896 [details] [diff] [review]
patch

Not critical for 2.0.0.2, maybe next time.