Resetting the profile invalidates baseurl as exposed by wptserve

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Andrei Eftimie, Assigned: Andrei Eftimie)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

(Whiteboard: [mozmill-2.1])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8461611 [details] [diff] [review]
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.
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.
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
(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?
Depends on: 1049676
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 970594
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 1056045
(Assignee)

Comment 6

4 years ago
Created attachment 8475889 [details] [diff] [review]
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.
Attachment #8461611 - Attachment is obsolete: true
Attachment #8475889 - Flags: review?(hskupin)
(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 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.
Attachment #8475889 - Flags: review?(hskupin) → review+
(Assignee)

Updated

4 years ago
Depends on: 1059761
(Assignee)

Updated

4 years ago
No longer depends on: 1056045
(Assignee)

Comment 9

4 years ago
Created attachment 8480571 [details] [diff] [review]
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.
Attachment #8475889 - Attachment is obsolete: true
Attachment #8480571 - Flags: review?(hskupin)
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.
Attachment #8480571 - Flags: review?(hskupin) → review+
Andrei, please fix the nit so we can also land this patch. Thanks.
(Assignee)

Comment 12

4 years ago
Created attachment 8481338 [details] [diff] [review]
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)
Attachment #8480571 - Attachment is obsolete: true
Attachment #8481338 - Flags: review?(hskupin)
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.
Attachment #8481338 - Flags: review?(hskupin) → review+
https://github.com/mozilla/mozmill/commit/2d516a9f2c34ec6d4abf87dcf899b4e98eac4bcf

Thanks Andrei!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [mozmill-2.1]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.