space around the colon in the syntax used for setting prefs is significant

RESOLVED FIXED in Firefox 67

Status

defect
RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: Ehsan, Assigned: KWierso)

Tracking

unspecified
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
If you have the following in an ini file:

prefs: [foo.bar: true]

You will be setting the pref value to |" true"| not to |true| which is what the author probably expects.  For boolean prefs, non-empty string values are interpreted as true, so this does what the author actually expects by accident.

But once you do:

prefs: [foo.bar: false]

we will set the pref to |" false"| which is also interpreted as true.
Assignee

Comment 2

6 months ago
This patch just strips the spaces out from the pref names and values prior to using them to set Firefox prefs.

And a try run to see if that breaks things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=785bcb2b17ebaee6d114c4357ea649e55d7bc037

Are the test metadata files linted anywhere? I know the manifest and the tests themselves have linters...
Assignee: nobody → wkocher
Assignee

Updated

6 months ago
Attachment #9033053 - Flags: review?(james)
Assignee

Comment 3

5 months ago

Also fix in-tree instances of leading/trailing whitespaces in pref values.

Assignee

Comment 4

5 months ago

Comment on attachment 9040297 [details]
Bug 1228678 - Remove leading/trailing whitespace in wpt pref values, also strip whitespace in prefs when used

This version fixes the in-tree instances of leading/trailing whitespaces in pref values and makes leading/trailing whitespace in pref values fatal to the test run by printing out an error message and stopping execution.

At the moment, encountering a leading/trailing whitespace makes the test run fail like this in Treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&revision=fb8b3d974ba8cca2f92b95c6a011413a2e1815dc&selectedJob=225098069

And the full failure log includes the message about the messed up preference: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=225098069&repo=try&lineNumber=990

BUT...

  1. It doesn't display the proper message as a failure line in TH's main UI. I assume I need to import and set up a logger rather than print()ing to stdout or stderr?

  2. It just calls sys.exit(1) to stop the test run after printing out the error message. There must be some way for me to trigger the end of the test run, right? Or should I print out the error message and let the test run continue despite the whitespace?

  3. I don't think manifestexpected.py always has a reference to the filename/path of the manifest with the whitespaced prefs. I think it might be that DirectoryManifest manifests don't have the "id" property while TestNode manifests do? Is there some way to get that information for all of the manifests so the error message can be made to point to the offending file?

Just f?you for now to see if this is going in the right direction and to help me resolve 1-3.

Attachment #9040297 - Flags: feedback?(james)
Attachment #9040297 - Attachment description: Bug 1228678 - Make a leading/trailing whitespace in pref values fatal → Bug 1228678 - Remove leading/trailing whitespace in wpt pref values, also strip whitespace in prefs when used
Assignee

Updated

5 months ago
Attachment #9033053 - Attachment is obsolete: true
Assignee

Updated

5 months ago
Attachment #9040297 - Flags: review?(james)

Comment 5

5 months ago
Pushed by wkocher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5f18fc8b26b
Remove leading/trailing whitespace in wpt pref values, also strip whitespace in prefs when used r=jgraham
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15258 for changes under testing/web-platform/tests
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee

Comment 9

5 months ago

This is to fix an upstream test that started failing in comment 6's PR. In the test, one of the values for prefs is "@Reset" which is apparently of type 'object' and can't be strip()'d. So lets just not attempt to strip() those.

I'm not entirely sure how these things get synced back upstream. Will this new patch just fix things there? Do I need to revert the original patch and submit a combined one?

Comment 10

5 months ago
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/e80ee365c75a
Followup to fix failing upstream wpt test r=jgraham
You need to log in before you can comment on or make changes to this bug.