1043391 - Resetting the profile invalidates baseurl as exposed by wptserve

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

Seems the final checks I did in bug 754847 comment 43 were off, mainly:
> - For the preference it seems safe to not re-set the pref after a profile reset,
> I have all tests passing. Not sure why earlier I had failures when not
> re-setting them. Tested it thouroughly and everything is green.

This is not true, resetting the profile with:
> controller.stopApplication(true)
Invalidates the preference where we save the `baseurl` value.

I'm not sure why it was working at that time. My environment might have been corrupt.

===

Anyway, one fix (as the code originally had) would be to set the preference again after we restart the runner. This works nicely. Henrik didn't like this approach (that's why it was removed), so I'll have another try to set this pref in a way that won't be removed when we reset the profile.

We can't set it the same way we set the other prefs because at that time the web server is not running yet, so we can't have a valid address.

Comment 1

[Andrei Eftimie]

Attachment: WIP1

As said above, this works fine and fixes the problem.
Also changed the restart test which catches this problem, so we do have a test for it not to regress.

Now, I upload this as a WIP to illustrate the problem and have a potential fix.
I'm looking into a way of setting it to not get deleted when we do a profile reset.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We store the default preferences as profile._preferences:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py#59

Then we call set_preferences to write those into user.js, which is the default file name. I think we should request an update of mozprofile, so it adds preferences written to user.js to the profile._preferences list.

That way a reset would correctly initialize the new profile instance with all the user defined preferences set before.

Comment 3

[Andrei Eftimie]

Note:

The preference is added both in `user.js` and in `prefs.js`.
While resetting the profile via stopApplication(true) clears `user.js`, the preference in `prefs.js` remains.

Running an affected test with a set profile actually carries over the server url preference:
> mozmill -m mutt/mutt/tests/js/testFrame/testHttpd/testRestartAvailability/manifest.ini -b /Applications/FirefoxNightly.app/ --profile=../profile/p1

If the profile is not set this will fail.

Comment 4

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> We store the default preferences as profile._preferences:
> http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/
> mozprofile/profile.py#59
> 
> Then we call set_preferences to write those into user.js, which is the
> default file name. I think we should request an update of mozprofile, so it
> adds preferences written to user.js to the profile._preferences list.
> 
> That way a reset would correctly initialize the new profile instance with
> all the user defined preferences set before.

I'm not sure how we should go about this since from the code this appears to be intended:
We have:
1) the default prefs per runner
2) prefs passed in at init time
3) additional prefs set later on

A reset now clears all set prefs, then re-sets the prefs from 1) and 2)
A change as laid out would invalidate the possibility of having (for the lack of another term) "temp" prefs.

If we'd go this route, how should we mark which prefs to keep as "default"?
All of them?

Comment 5

[Andrei Eftimie]

Once bug 1049676 lands, we'll need for both mozprofile and mozrunner a version bump and release, we can update them in this bug while also changing the set_preferences method and the test to not regress this at a later date.

Comment 6

[Andrei Eftimie]

Attachment: 0001-Bug-1043391-Use-the-new-set_persisted_preferences-me.patch

Using the new method available in a new mozprofile version.

We'll have to wait for mozprofile's version bump and up mozmill's dependency version bump first.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #6)
> We'll have to wait for mozprofile's version bump and up mozmill's dependency
> version bump first.

Has this bump already happened? If not would you mind to write the appropriate patch?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8475889 [details] [diff] [review]
0001-Bug-1043391-Use-the-new-set_persisted_preferences-me.patch

Review of attachment 8475889 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine but please explicitly add mozprofile to setup.py, given that we make direct use of it now. This would prevent some bad side-effects with version conflicts too given that mozrunner is not that strict in it. r=me with this update.

::: mozmill/mozmill/__init__.py
@@ +621,5 @@
>          self.http_server.router.register(['POST'], '*', wptserve.handlers.file_handler)
>  
>          # expose the URL as a pref
> +        self.runner.profile.set_persistent_preferences({'extensions.mozmill.baseurl':
> +                                                        self.http_server.get_url()})

nit: Can you please move the property and value into its own line? It's really long and exceeds the 80 char limit.

Comment 9

[Andrei Eftimie]

Attachment: 0002-Bug-1043391-Use-the-new-set_persisted_preferences-me.patch

The only dependency we have is a new mozprofile release (bug 1059761), once done this can land.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8480571 [details] [diff] [review]
0002-Bug-1043391-Use-the-new-set_persisted_preferences-me.patch

Review of attachment 8480571 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nit fixed.

::: mozmill/setup.py
@@ +14,1 @@
>          'mozinfo == 0.7',

nit: keep the alphabetical order.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andrei, please fix the nit so we can also land this patch. Thanks.

Comment 12

[Andrei Eftimie]

Attachment: 0003-Bug-1043391-Use-the-new-set_persisted_preferences-me.patch

Seems I forgot the alphabet :P

I've rerun all mutt test in a new environment to pick up mozprofile from pypi and its all good.
(there are 2 known mutt py failures, which are logged and have a different cause)

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8481338 [details] [diff] [review]
0003-Bug-1043391-Use-the-new-set_persisted_preferences-me.patch

Review of attachment 8481338 [details] [diff] [review]:
-----------------------------------------------------------------

I already gave r+ with the last patch. No need to request a review again. I will get this landed in a bit.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://github.com/mozilla/mozmill/commit/2d516a9f2c34ec6d4abf87dcf899b4e98eac4bcf

Thanks Andrei!