Closed Bug 905404 Opened 11 years ago Closed 6 years ago

[mozprofile] Remove testing related preferences

Categories

(Testing :: Mozbase, defect, P1)

Product:
Testing
Component:
Mozbase
Type:
defect

Tracking

(firefox64 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(4 files, 4 obsolete files)

[mozprofile] Remove all default testing related preferences
5.09 KB, patch
ahal
: review+
Details | Diff | Splinter Review
[pgo] Add custom profile data in testing/profiles/profileserver
2.54 KB, patch
ahal
: review+
Details | Diff | Splinter Review
[marionette] WIP: Add missing preferences from mozprofile
1.44 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
[wpt] Add custom profile data in testing/profiles/web-platform
4.15 KB, patch
jgraham
: review+
Details | Diff | Splinter Review 
Once bug 905400 has been implemented we have to check which of the testing preferences on the FirefoxProfile class can be removed. One example might be the pref we added for bug 897891.
It's actually not only necessary for the preferences as added for Mozmill years ago, but for all preferences as used by automated tests. 

Now that we have profiles for test suites in place under `/testing/profiles` (bug 1451159) we should finally clean-up mozprofile.

https://dxr.mozilla.org/mozilla-central/source/testing/profiles
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Depends on: 1451159
No longer depends on: 905400
Priority: -- → P1
Summary: [mozprofile] Remove Mozmill related testing preferences from FirefoxProfile → [mozprofile] Remove testing related preferences
Blocks: 1398111
Also note that mozprofile is used outside of testing, so setting those preferences doesn't make sense at this place.
I wonder if we should just remove "FirefoxProfile" outright. I understand that not everything uses testing/profiles though, so maybe we can't do it just yet. But I think most things already use the Profile class.
(In reply to Andrew Halberstadt [:ahal] from comment #3)
> I wonder if we should just remove "FirefoxProfile" outright. I understand
> that not everything uses testing/profiles though, so maybe we can't do it
> just yet. But I think most things already use the Profile class.

I would defer this to later. It would be a larger change as I would expect.
So there are a lot of crashes happening for the try build. Especially the following assertion and crash is wondering me:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=638877e421df199cec5a8bec7a858419204b57c1&selectedJob=197634051

Assertion failure: aEditorBase, at /builds/worker/workspace/build/src/dom/events/IMEContentObserver.cpp:317

> [task 2018-09-05T15:25:26.205Z] 15:25:26     INFO - GECKO(1353) | Assertion failure: aEditorBase, at /builds/worker/workspace/build/src/dom/events/IMEContentObserver.cpp:317
> [task 2018-09-05T15:26:14.016Z] 15:26:14     INFO - GECKO(1353) | #01: mozilla::IMEContentObserver::InitWithEditor(nsPresContext*, nsIContent*, mozilla::EditorBase*) [dom/events/IMEContentObserver.cpp:0]
> [task 2018-09-05T15:26:14.018Z] 15:26:14     INFO - 
> [task 2018-09-05T15:26:14.019Z] 15:26:14     INFO - GECKO(1353) | #02: mozilla::IMEContentObserver::Init(nsIWidget*, nsPresContext*, nsIContent*, mozilla::EditorBase*) [dom/events/IMEContentObserver.cpp:0]
> [task 2018-09-05T15:26:14.023Z] 15:26:14     INFO - 
> [task 2018-09-05T15:26:14.024Z] 15:26:14     INFO - GECKO(1353) | #03: mozilla::IMEStateManager::CreateIMEContentObserver(mozilla::EditorBase*) [dom/events/IMEStateManager.cpp:1946]
> [task 2018-09-05T15:26:14.025Z] 15:26:14     INFO - 
> [task 2018-09-05T15:26:14.026Z] 15:26:14     INFO - GECKO(1353) | #04: mozilla::IMEStateManager::OnChangeFocusInternal(nsPresContext*, nsIContent*, mozilla::widget::InputContextAction) [dom/events/IMEStateManager.cpp:702]
> [task 2018-09-05T15:26:14.027Z] 15:26:14     INFO - 
> [task 2018-09-05T15:26:14.028Z] 15:26:14     INFO - GECKO(1353) | #05: mozilla::IMEStateManager::OnChangeFocus(nsPresContext*, nsIContent*, mozilla::widget::InputContextAction::Cause) [dom/events/IMEStateManager.cpp:453]
> [task 2018-09-05T15:26:14.029Z] 15:26:14     INFO - 
> [task 2018-09-05T15:26:14.030Z] 15:26:14     INFO - GECKO(1353) | #06: nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, nsIContent*) [dom/base/nsFocusManager.cpp:1981]
> [task 2018-09-05T15:26:14.032Z] 15:26:14     INFO - 
> [task 2018-09-05T15:26:14.034Z] 15:26:14     INFO - GECKO(1353) | #07: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [mfbt/RefPtr.h:79]

Masayuki, do you have an idea what this could be? I cannot find a bug for such an assertion or crash in Bugzilla. I wonder if this could be related to the changes to preferences, and that Firefox tries to access a remote address, which then crashes. But usually those crashes look different. Thanks!
Flags: needinfo?(masayuki)
Hm, maybe all those crashes happen because I set the `focusmanager.testmode` preference to `true`. I assume this makes it a crash which is somewhat hidden so far.

Andrew, I wonder if I should remove this preference. I'm actually not sure which test suites beside Marionette rely on it. Do you know?
Flags: needinfo?(ahal)
I pushed another try build without this preference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec38d7f1287f34e06499a7c8f0708b7e7b51e7eb

What puzzles me is that this preference was set in mozprofile, which all test suites should have inherited before. Not sure why this wasn't causing an issue before.
Afaict, the only major test suite using FirefoxProfile is web-platform-tests, everything else (save for a few minor exceptions) uses the base Profile class, and therefore doesn't have FirefoxProfile's prefs applied.

So you shouldn't re-add those prefs to testing/profiles/common/user.js, but rather create a new testing/profiles/web-platform-tests/user.js that has the missing ones.
Flags: needinfo?(ahal)
No longer blocks: 1398111
(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Afaict, the only major test suite using FirefoxProfile is
> web-platform-tests, everything else (save for a few minor exceptions) uses
> the base Profile class, and therefore doesn't have FirefoxProfile's prefs
> applied.

I checked that for wpt, and it is indeed:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py?q=firefox.py&redirect_type=direct#222

> So you shouldn't re-add those prefs to testing/profiles/common/user.js, but
> rather create a new testing/profiles/web-platform-tests/user.js that has the
> missing ones.

I see. So given that web-platform-tests use the common, and unittest profile I will just make a copy of the unittest one, and add those missing preferences.
(In reply to Henrik Skupin (:whimboo) from comment #6)
> Assertion failure: aEditorBase, at
> /builds/worker/workspace/build/src/dom/events/IMEContentObserver.cpp:317
> 
> Masayuki, do you have an idea what this could be? I cannot find a bug for

I actually filed bug 1489029 for this assertion and crash.
Flags: needinfo?(masayuki)
(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Afaict, the only major test suite using FirefoxProfile is
> web-platform-tests, everything else (save for a few minor exceptions) uses

Note that also profileserver is using FirefoxProfile. As such it would need the same changes as web-platform-tests. Otherwise I cannot see anything else using it.
The default profile data for web-platform-tests shouldn't
base on the ones from unittests because of extra preference
defaults.
Attachment #9006816 - Flags: review?(james)
The default profile data for profileserver shouldn't
base on the ones from unittests because of extra preference
defaults.
Attachment #9006817 - Flags: review?(ahal)
All preferences which were previously set for test harnesses are
part of testing/profiles now. As such they can all be removed.
Attachment #9006818 - Flags: review?(ahal)
So I am a little confused. Why is wpt the only thing using FirefoxProfile? Should wpt actually start using Profile? I don't necessarily object to us removing wpt from the general profile set (because it might indeed make sense to use a more minimal set of prefs for wpt and in particular to not enable features in the default prefs set, only on a per-test[-directory] basis) but I'm keen to understand the current situation first.
It's not only wpt, but also profileserver, and Marionette (which is not using the testing/profiles data yet). Also I don't want to expand this bug to get FirefoxProfile removed. We should really file a new bug on that if it is wanted. Instead this bug simply cleans-up mozprofile by moving all the user preferences to the appropriate test harnesses.
Right, I"m not saying we should expand this bug. I'm saying I don't understand the current situation, which makes is hard/impossible to review changes to it. What's the difference between FirefoxProfile and Profile? What specifically is being changed in the wpt preferences?
FirefoxProfile is based on Profile and was created ages ago for Firefox only specific default preferences. At that time of creation and even years later we were going to add preferences which were only related to automation. That actually should never have been done, because mozprofile is not only used for our test harnesses but also tools to just create a profile. And they all got those automation specific settings enabled.

In regards of the patches attached nothing in WPT changes. I picked the unittest preferences from /testing/profiles/unittest/user.js and only added those preferences which have been additionally set in mozprofile. Further some static preferences from firefox.py could be also moved over, given that web-platform-tests have it's own set of preferences now.

Any reduction of the preferences as used now, should also be done later. It would also require a good amount of investigation.
Comment on attachment 9006815 [details] [diff] [review]
[marionette] WIP: Add missing preferences from mozprofile

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

::: testing/marionette/client/marionette_driver/geckoinstance.py
@@ +569,5 @@
>          # tests that don't expect it to be there.
>          "browser.urlbar.userMadeSearchSuggestionsChoice": True,
>  
> +        # Don't warn when exiting the browser
> +        "browser.warnOnQuit": False,

According to
https://searchfox.org/mozilla-central/rev/a41fd8cb947266ea2e3f463fc6e31c88bfab9d41/testing/geckodriver/src/prefs.rs#64-66
this is deprecated and can safely be removed once the minimum
supported Firefox becomes version 61.

Do we need a similar comment to remind ourselves here?
Attachment #9006815 - Flags: review?(ato) → review+
Comment on attachment 9006817 [details] [diff] [review]
[profileserver] Add custom profile data in testing/profiles/profileserver

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

In case you didn't know this, there's a handy 'profile diff' tool under testing/profiles. You can use it like:

$ cd testing/profiles
$ ./profile diff ./profileserver ./unittest

You can think of the verbs it prints out as instructions for morphing from A -> B. So if it says "delete", that means those are prefs that exist in ./profileserver but not in ./unittest.

::: testing/profiles/profiles.json
@@ +1,3 @@
>  {
>      "mochitest": ["common", "unittest"],
> +    "profileserver": ["common", "profileserver"],

It looks like the vast majority of prefs between profileserver and unittest are the same. Could we instead make 'profileserver/user.js' inherit from both common and unittest? So:

"profileserver": ["common", "unittest", "profileserver"],

The last file would just contain the extra ones that used to be set on FirefoxProfile.
Attachment #9006817 - Flags: review?(ahal) → review-
Also you can use the 'profile' tool to display an overall suite. So a good way to make sure you didn't modify any prefs by accident is:

$ cd testing/profiles
$ hg up central
$ ./profile show profileserver > a.js  # note no leading '.' on 'profileserver'
$ hg up <rev>
$ ./profile show profileserver > b.js
$ diff a.js b.js

Then make sure only the prefs you expect (i.e the ones from FirefoxProfile) were changed.
Comment on attachment 9006818 [details] [diff] [review]
[mozprofile] Remove all default testing related preferences

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

Lgtm!
Attachment #9006818 - Flags: review?(ahal) → review+
Comment on attachment 9006816 [details] [diff] [review]
[wpt] Add custom profile data in testing/profiles/web-platform

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

I'm going to preemptively r- this for the same reason as I did for the other one. We want to make sure wpt uses the unittest profile so changes there are automatically picked up by web-platform-tests.

You I'd be fine with either two separate profiles ("wpt/user.js" and "profileserver/user.js") or a single one that is shared between the both wpt and profileserver (I can't really come up with a good name for this).
Attachment #9006816 - Flags: review?(james) → review-
(In reply to James Graham [:jgraham] from comment #18)
> So I am a little confused. Why is wpt the only thing using FirefoxProfile?
> Should wpt actually start using Profile? I don't necessarily object to us
> removing wpt from the general profile set (because it might indeed make
> sense to use a more minimal set of prefs for wpt and in particular to not
> enable features in the default prefs set, only on a per-test[-directory]
> basis) but I'm keen to understand the current situation first.

This patch wasn't actually changing any prefs for web-platform-tests. Though the fact that it is swapping away the 'unittest' profile is wrong. The "unittest" profile is meant to be for prefs that we want to set globally across all our test harnesses.

The 'FirefoxProfile' class was a very early attempt at having different prefs for different contexts and has been superseded by testing/profiles. So this patch is just a cleanup to move a handful of prefs that were still using that old method into the new testing/profiles method.
Hm, what about reftests? Are those special? Shouldn't those depend on the unittest prefs too? Why are those separate right now?
Flags: needinfo?(ahal)
They should. The current structure is just how things were shared with the old prefs-general.js file, I never got around to merging reftest in with unittest (possibly because it was fairly different from prefs-general).

The situation isn't perfect, but in general I'd like to move more towards shared prefs and less towards duplicating everything.
Flags: needinfo?(ahal)
Actually if you run "./profile diff ./reftest ./unittest" you'll see they actually have very little in common. So it's probably not worth merging them (maybe my earlier assertion that *all* harnesses should use it was false). But there's still no reason for web-platform-tests to duplicate all the shared prefs it has with mochitest et al.
FWIW I would quite like to remove all the feature setting prefs from the wpt set, because at the moment they are being applied to runs on wpt.fyi for stable browsers, which doesn't make sense. So depending on how you think that should be handled for mochitest, it may be a reason to move away from a totally shared set (or may be a reason to allow harnesses to depend on multiple sets of prefs, or something).
Oh sorry, the multiple sets of prefs thing already works. So that's the actual change we should be making here I think; factor the prefs into "things needed to make tests run deterministically" and "features that people want to enable in gecko CI but that don't ship".
Yeah agree. Ideally I'd like to have profiles for features rather than harnesses.. e.g a "media" profile for preferences needed to run media related tests.

But the scope of this bug is just "move the preferences that were previously set via FirefoxProfile to somewhere in testing/profiles". Re-factoring the web-platform-test prefs to their ideal state is better off as a follow-up bug.
Right, I was sort of tangentially agreeing with your r- on the basis that the changes proposed here don't match where we want to end up. I admit that wasn't obvious!
(In reply to Andreas Tolfsen ❲:ato❳ from comment #22)
> ::: testing/marionette/client/marionette_driver/geckoinstance.py
> @@ +569,5 @@
> >          # tests that don't expect it to be there.
> >          "browser.urlbar.userMadeSearchSuggestionsChoice": True,
> >  
> > +        # Don't warn when exiting the browser
> > +        "browser.warnOnQuit": False,
> 
> According to
> https://searchfox.org/mozilla-central/rev/
> a41fd8cb947266ea2e3f463fc6e31c88bfab9d41/testing/geckodriver/src/prefs.rs#64-
> 66
> this is deprecated and can safely be removed once the minimum
> supported Firefox becomes version 61.

That comment seems to be wrong. The preference still exists in mozilla-central and is set to `true` by default. I will remove the comment from geckodriver.

For the other recent comments I will work on getting the prefs refactored, and using the unittest ones.
(In reply to Andrew Halberstadt [:ahal] from comment #26)
> You I'd be fine with either two separate profiles ("wpt/user.js" and
> "profileserver/user.js") or a single one that is shared between the both wpt
> and profileserver (I can't really come up with a good name for this).

None have something in common, and as such shouldn't be based on the same profile. I'm not even sure if profileserver needs all the preferences as set by unittests, given that it only starts Firefox twice, and quits it immediately each time via the installed quitter extension:

https://dxr.mozilla.org/mozilla-central/rev/4df1ba9c741f1cb6661d9f205ad0fedf91d31925/build/pgo/profileserver.py#88-109

Also I wonder if the following preferences from the FirefoxProfile in mozprofile have to be set at all. None seem to be relevant, except for the two crash related prefs:

> // Don't restore the last open set of tabs if the browser has crashed
> userpref("browser.sessionstore.resume_from_crash", false);
> // Don't warn when exiting the browser
> userpref("browser.warnOnQuit", false);
> // Only install add-ons from the profile and the application scope
> // Also ensure that those are not getting disabled.
> // see: https://developer.mozilla.org/en/Installing_extensions
> userpref("extensions.autoDisableScopes", 10);
> // Don't open a dialog to show available add-on updates
> userpref("extensions.update.notifyUser", false);
> // Enable test mode to run multiple tests in parallel
> userpref("focusmanager.testmode", true);
> // Suppress automatic safe mode after crashes
> userpref("toolkit.startup.max_resumed_crashes", -1);

Mike, can you tell me which of those are really needed for the pgo profile server script? Thanks.
Flags: needinfo?(mh+mozilla)
maybe browser.warnOnQuit
Flags: needinfo?(mh+mozilla)
Add extra preferences for web-platform-tests on-top of
the ones as inheritted from unittests.
Attachment #9006816 - Attachment is obsolete: true
Add extra preferences for the profileserver script on-top of
the ones as inheritted from unittests.
Attachment #9006817 - Attachment is obsolete: true
Attachment #9006815 - Attachment is obsolete: true
Comment on attachment 9009189 [details] [diff] [review]
[marionette] WIP: Add missing preferences from mozprofile

Removed the irritating TODO comment for "browser.warnOnQuit" from prefs.rs. Otherwise no changes. So taking over r+ from Andreas.
Attachment #9009189 - Flags: review+
Attachment #9009187 - Flags: review?(james)
Attachment #9009188 - Flags: review?(ahal)
Comment on attachment 9009187 [details] [diff] [review]
[wpt] Add custom profile data in testing/profiles/web-platform

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

So there are a couple of issues here.

The main one is that I think this will break wpt.fyi running stable builds because it will use contemporary wptrunner and the user prefs file from stable (which it has to do because people keep changing pref names). So we can't actually remove the prefs until this change reaches stable. Also I wonder how many of these settings should really be applied to wpt only and not to all unittest suites? A number of them seem like they are generic testing settings?
Attachment #9009187 - Flags: review?(james) → review-
(In reply to James Graham [:jgraham] from comment #43)
> The main one is that I think this will break wpt.fyi running stable builds
> because it will use contemporary wptrunner and the user prefs file from
> stable (which it has to do because people keep changing pref names). So we
> can't actually remove the prefs until this change reaches stable. Also I

Hm, that's sad to see. So shall we move out the wpt profile changes to another bug? We could then uplift the patch to beta, and would only have to wait 5 or so weeks until it reaches the release branch and the next Firefox release.  

> wonder how many of these settings should really be applied to wpt only and
> not to all unittest suites? A number of them seem like they are generic
> testing settings?

As I mentioned earlier this is a clean-up for mozprofile. I'm not going to refactor all preferences for each type of profile in this bug.
Flags: needinfo?(james)
I think it would be acceptable to land this if you just add a comment to the wptrunner file that the prefs can be removed when 64 hits stable, instead of removing them.
Flags: needinfo?(james)
(In reply to James Graham [:jgraham] from comment #45)
> I think it would be acceptable to land this if you just add a comment to the
> wptrunner file that the prefs can be removed when 64 hits stable, instead of
> removing them.

Ok, so I will mark them with a comment instead. But we can still remove the following preferences because they are part of the common and unittest profiles for a while:

> -                                      "dom.disable_open_during_load": False,
> -                                      "dom.send_after_paint_to_content": True,

James, I assume you don't worry about the preferences as removed from mozprofile in attachment 9009189 [details] [diff] [review]?
Flags: needinfo?(james)
(In reply to Henrik Skupin (:whimboo) from comment #46)
> James, I assume you don't worry about the preferences as removed from
> mozprofile in attachment 9009189 [details] [diff] [review]?

Sorry, that should have been attachment 9006818 [details] [diff] [review].
Comment on attachment 9009188 [details] [diff] [review]
[pgo] Add custom profile data in testing/profiles/profileserver

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

Thanks, lgtm!
Attachment #9009188 - Flags: review?(ahal) → review+
Add extra preferences for web-platform-tests on-top of
the ones as inheritted from unittests.
Attachment #9009868 - Flags: review?(james)
Attachment #9009187 - Attachment is obsolete: true
Attachment #9009868 - Flags: review?(james) → review+
Flags: needinfo?(james)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21d3120b9e4c
[marionette] Add missing preferences from mozprofile. r=ato
https://hg.mozilla.org/integration/mozilla-inbound/rev/33386e97f198
[wpt] Add custom profile data in testing/profiles/web-platform. r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/560cee122174
[pgo] Add custom profile data in testing/profiles/profileserver. r=ahal
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac8690f31a1
[mozprofile] Remove all default testing related preferences. r=ahal
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13050 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
See Also: → 1742484
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: