[I tripped over this bug when testing patches for bug 837211.] The testcase test_property_database.html uses the function "SpecialPowers.getBoolPref" to check whether properties are enabled. *But*, this API throws (and causes a test failure) if a property does not exist. So right now, we are (unintentionally) requiring that all pref-controlled properties *have their pref set to something* during mochitest runs. This seems like an undesirable requirement. I think we should use a helper-function that wraps the getBoolPref in try{}catch{} to allow for unset pref-controlling properties.

(In reply to Daniel Holbert [:dholbert] from comment #0) > The testcase test_property_database.html uses the function > "SpecialPowers.getBoolPref" to check whether properties are enabled. *But*, > this API throws (and causes a test failure) if a property does not exist. Sorry -- I meant to end that sentence with "...if a pref does not exist." (s/property/pref/)

As an example, here's a patch that triggers this failure, simply by removing layout.css.grid.enabled from all.js and the mochitest-specific prefs file. With this patch, I get these failures when I run test_property_database.html: { 355 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | uncaught exception - uncaught exception: Error getting pref 'layout.css.grid.enabled' at :0 simpletestOnerror@SimpleTest/SimpleTest.js:1517:11 OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1504:1 356 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'background-blend-mode' listed in gCSSProperties @layout/style/test/test_property_database.html:40:1 357 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'box-decoration-break' listed in gCSSProperties @layout/style/test/test_property_database.html:40:1 358 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'contain' listed in gCSSProperties @layout/style/test/test_property_database.html:40:1 359 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | uncaught exception - uncaught exception: Error getting pref 'layout.css.grid.enabled' at :0 } The first failure is the issue described in comment 0. The latter failures are from a similar issue when processing property_database.js (which also uses one-off getBoolPref() calls to check prefs).

The patch just (1) creates a new utility function "isBoolPrefSetAndEnabled" which lives in property_database.js (2) Performs s/SpecialPowers.getBoolPref/isBoolPrefSetAndEnabled/ in property_database.js and test_property_database.html With that, the earlier attached demo-patch no longer causes uncaught exceptions / test-failures in test_property_database.html. (And similarly, future pref-controlled CSS properties whose prefs happen to be not-set-by-default will no longer break test_property_database.html.)

Comment on attachment 8665529 [details] [diff] [review] fix v1 I think you should report a test failure in case of an exception (i.e., inside the catch). Otherwise we risk not taking the pref-guarded chunks out of the pref-guard and leaving things untested, no? (Especially for prefs that aren't entire properties.) Maybe call the function CSSPropertyPrefEnabled or something like that? r=dbaron with that

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #5) > I think you should report a test failure in case of an exception (i.e., > inside the catch). So, I initially filed this bug to *avoid* the test-failure that was being caused by this exception, because I was tripping over it with the patch stack in bug 837211 (which currently add some CSS properties behind a hidden pref). And reporting a test-failure inside of the 'catch' kind of defeats that goal, though it is a bit cleaner than leaning on the harness to catch the exception. But you've convinced me that we really *should* be considering this scenario (pref-check for a missing pref) a test-failure, particularly in cases like you mentioned where we remove a pref (because a property is fully & solidly supported) and forget to remove its pref-guarded tests. So, I'll make the changes you requested. (And I'll have to be sure that bug 837211's patches actually add the pref that controls the new properties there, or else it'll run afoul of this test failure.)

Here's the updated fix with a test-failure added, and with the function renamed to IsCSSPropertyPrefEnabled. Test-failures used to look like comment 2; now with this patch, they'll look like this: { 2479 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | Failed to look up property-controlling pref 'layout.css.grid.enabled' (Error getting pref 'layout.css.grid.enabled') IsCSSPropertyPrefEnabled@layout/style/test/property_database.js:22:5 @layout/style/test/property_database.js:5822:5 }

Green try run (w/ a few intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f295274a2d3 Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/e801ecdbd5f9