999009 - mozregression should reset preferences to their previous value after running

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[William Lachance (:wlach)]

[importing from github]

msujaws:

Occasionally I will need to run mozregression on my default profile to reproduce a bug that I found.

It turns out that mozregression sets browser.sessionstore.resume_from_crash among other preferences, but doesn't return them to their pre-mozregression values.

mnoorenberghe:

The code which sets the prefs actually lives in mozprofile https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py which is used by mozregression. mozprofile is also used for tinderbox tests so it seems like we may need different sets of prefs for running vs. testing.
Either these prefs shouldn't be set by mozregression, or if mozregression needs them, then it should reset them back to their prior values.

This was the cause for https://bugzilla.mozilla.org/show_bug.cgi?id=865456

davehunt:

I've recently modified the Marionette test runner to clone the profile instead of using it directly. Perhaps mozregression should do something similar by default? See bug 857599

k0s:

We're hoping to revamp most of the mozprofile system (very) soon. That said, a workaround could be subclassing FirefoxProfile in mozregression and removing the pref.

Comment 1

[William Lachance (:wlach)]

I think a better approach would be to just make a copy of your profile for this kind of case. We could incorporate instructions on this into hypothetical mozregression documentation (see bug 1107602)

Comment 2

[Julien Pagès (:parkouss)]

Hmm thinking about this again, I agree with Dave here. I don't think that mozregression should have the side-effect of modifying a profile, so duplicating the profile before using it seems good to me if it is normal for mozprofile to make changes in an used profile.

Will ?

Comment 3

[William Lachance (:wlach)]

(In reply to Julien Pagès from comment #2)
> Hmm thinking about this again, I agree with Dave here. I don't think that
> mozregression should have the side-effect of modifying a profile, so
> duplicating the profile before using it seems good to me if it is normal for
> mozprofile to make changes in an used profile.
> 
> Will ?

Makes sense to me.

Comment 4

[Julien Pagès (:parkouss)]

I have some issues trying to implement this - the cloned profile are not removed (the should) but I am not maintaining an apparent reference...

Explicitly calling cleanup() on the profile works well (cloned or not). I opened a bug to have this implemented in a nice way: bug 1173335.

Comment 5

[jonathan.pigree]

It seems the bug has been handled on mozprofile side. We only needs to implement this into mozregression right? Can I take care of this?

Comment 6

[Julien Pagès (:parkouss)]

Hey Jonathan,

Yeah, if you are interested! I already started the patch in fact (from bug 1173335 comment 3), you can see it here: https://github.com/parkouss/mozregression/commit/f002b61b4de21d199742cd340087fbf6837724b1

So I think the core of the patch is here. But there is still the unit tests to adapt, also we should require mozprofile >= 0.24, and we should test (with a manual mozreg run) that using an existing profile with mozreg still works and does not alter it (no modifications at all in the profile tree).

Do you want to work on this ?

Comment 7

[jonathan.pigree]

Yes. I will do it.

Comment 8

[jonathan.pigree]

Attachment: PR

Comment 9

[jonathan.pigree]

It seems I managed to do it. I add many problems with profile directories cleanup so I explicitly delete them.

Tell me what you think of it.

Thanks.

Comment 10

[Julien Pagès (:parkouss)]

Comment on attachment 8643598 [details] [review]
PR

Seems good to me, thanks a lot for doing this. :)

I added some comments on the PR, if you could address them - also now I like to see the bug number in the commit summary so it is easy to track changes when I am doing a changelog. Could you write the first line of your commit like:

Bug 999009 - Clone exiting mozprofiles to prevent persistence


Thanks!

Comment 11

[jonathan.pigree]

Okay. Yes. I totally agree with this (bug id into commit message).

I just fixed everything.

Thanks for the review.

Comment 12

[Julien Pagès (:parkouss)]

Excellent!

Merged in https://github.com/mozilla/mozregression/commit/c2d7480ada638b061b3216ec4cc67e27b1f6b531