Closed Bug 865136 Opened 6 years ago Closed 6 years ago

Calling nsIProfile->Remove twice removes all profiles

Categories

(Core Graveyard :: Profile: BackEnd, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla23

People

(Reporter: Gijs, Assigned: Gijs)

Details

Attachments

(1 file, 2 obsolete files)

STR:

0. Create a profile
1. Remove it
2. Remove it again

Expected: NS_ERROR_FAILURE? Silent no-op? Either would work.

Actual: All the profiles get removed from profiles.ini.


Here's why:

http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp?rev=63d127cca94c&mark=249,252,257,258#224

For the first removal, mNext and mPrev are set to null.
For the second removal, the if condition (mPrev) on line 249 is not fulfilled, and so the code assumes that we're the first profile in the profile service, and sets the service's mFirst to our mNext - but that's also null because we just set it to null in the previous removal.

Note that we can't just check if both mNext and mPrev are null, because that's a legitimate case (namely, removing the only profile currently known to the profile service in order to migrate it or create a new one or whatever).
Attached patch Patch (obsolete) — Splinter Review
Attachment #741221 - Flags: review?(benjamin)
Comment on attachment 741221 [details] [diff] [review]
Patch

I think the better answer here is to make it throw NS_ERROR_NOT_INITIALIZED.
Attachment #741221 - Flags: review?(benjamin) → review-
Attached patch Patch (obsolete) — Splinter Review
Like this? :-)
Attachment #741221 - Attachment is obsolete: true
Attachment #741310 - Flags: review?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 741221 [details] [diff] [review]
> Patch
> 
> I think the better answer here is to make it throw NS_ERROR_NOT_INITIALIZED.

Hrm, I suppose I assumed that you would still want to do that only after removing the data directories if someone passed the argument to do that... unless we don't want to support that and bail out immediately?
Comment on attachment 741310 [details] [diff] [review]
Patch

Indeed, I don't think we ought to be removing the directories in this case. Calling remove twice on the same profile is just broken behavior...
Attachment #741310 - Flags: review?(benjamin) → review-
Attached patch Patch v3Splinter Review
Attachment #741310 - Attachment is obsolete: true
Attachment #742233 - Flags: review?(benjamin)
Attachment #742233 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/6553478822f5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.