865136 - Calling nsIProfile->Remove twice removes all profiles

Status: RESOLVED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[:Gijs (he/him)]

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).

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Comment 2

[Benjamin Smedberg]

Comment on attachment 741221 [details] [diff] [review]
Patch

I think the better answer here is to make it throw NS_ERROR_NOT_INITIALIZED.

Comment 3

[:Gijs (he/him)]

Attachment: Patch

Like this? :-)

Comment 4

[:Gijs (he/him)]

(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 5

[Benjamin Smedberg]

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...

Comment 6

[:Gijs (he/him)]

Attachment: Patch v3

Comment 7

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6553478822f5

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6553478822f5