1197617 - TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | test_addonsAndPlugins - [test_addonsAndPlugins : 471] signedState must have the correct type. - "undefined" == "number"

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

This (and a few other failures we have) are due to unit tests checking for addon.signedState. When MOZ_ADDON_SIGNING is 0, then addon.signedState gets the value undefined. I've also found test_theme.js which specifically expects the signedState to be undefined, I guess because themes don't need signing. I am not quite sure what this undefined state is supposed to mean, to me it would make more sense for for it to always have a defined value.

There are two ways I can fix this bug. Option 1 would be to go through all tests and make checks for signedState depend on the value of MOZ_ADDON_SIGNING. For this option I'd also add documentation on the "undefined" state. Option 2 would be to change the default value for signedState from undefined to SIGNEDSTATE_MISSING, so that it always has a defined value. This seems to make some tests happy (but will still require others like test_theme.js to be changed).

In any case, I'd be moving ADDON_SIGNING and SIGNING_REQUIRED from XPIProvider to AddonManager so it can be used in tests that check for specific values of signedState although they are not tests specific to signing.

Dave, does this make sense? Do you have any other ideas on how to fix this, or would you prefer one of the above options?

Comment 2

[Dave Townsend [:mossop]]

(In reply to Philipp Kewisch [:Fallen] from comment #1)
> This (and a few other failures we have) are due to unit tests checking for
> addon.signedState. When MOZ_ADDON_SIGNING is 0, then addon.signedState gets
> the value undefined. I've also found test_theme.js which specifically
> expects the signedState to be undefined, I guess because themes don't need
> signing. I am not quite sure what this undefined state is supposed to mean,
> to me it would make more sense for for it to always have a defined value.
> 
> There are two ways I can fix this bug. Option 1 would be to go through all
> tests and make checks for signedState depend on the value of
> MOZ_ADDON_SIGNING. For this option I'd also add documentation on the
> "undefined" state. Option 2 would be to change the default value for
> signedState from undefined to SIGNEDSTATE_MISSING, so that it always has a
> defined value. This seems to make some tests happy (but will still require
> others like test_theme.js to be changed).

undefined is distinct from SIGNEDSTATE_MISSING because it has a different meaning, it means that this add-on doesn't need to be signed and so the signedState doesn't need to be checked. We could use another constant for that I guess but undefined has this nice property that no matter what value you compare to the result is always false, a fact that we use to our advantage in e.g. https://hg.mozilla.org/mozilla-central/file/tip/toolkit/mozapps/extensions/content/extensions.js#l3190

Given that we have places in the code testing with that assumption in mind and that I can't remember where they all are I think option 1 is the safest option. It also makes some sense, if the build doesn't support signing then we don't really care what the value of signedState is.

> In any case, I'd be moving ADDON_SIGNING and SIGNING_REQUIRED from
> XPIProvider to AddonManager so it can be used in tests that check for
> specific values of signedState although they are not tests specific to
> signing.

This sounds good, while you're there would you mind updating these locations to use that?

https://hg.mozilla.org/mozilla-central/file/tip/browser/components/nsBrowserGlue.js#l1212
https://hg.mozilla.org/mozilla-central/file/tip/toolkit/mozapps/extensions/content/extensions.js#l3772

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Dave Townsend [:mossop] from comment #2)
> (In reply to Philipp Kewisch [:Fallen] from comment #1)
> undefined is distinct from SIGNEDSTATE_MISSING because it has a different
> meaning, it means that this add-on doesn't need to be signed and so the
> signedState doesn't need to be checked.
Ah, thanks for the clarification. I was thinking maybe to add a SIGNEDSTATE_NOT_REQUIRED and give that the value undefined, what do you think? This way we can still use the comparison to our advantage, but don't use the magic value undefined directly.

> > In any case, I'd be moving ADDON_SIGNING and SIGNING_REQUIRED from
> > XPIProvider to AddonManager so it can be used in tests that check for
> > specific values of signedState although they are not tests specific to
> > signing.
> 
> This sounds good, while you're there would you mind updating these locations
> to use that?
I can do that. While writing the code I had some doubts about moving this to AddonManager in general. First of all, I was thinking evil code could do:

var backstagepass = Components.utils.import("resource://gre/modules/AddonManager.jsm");
backstagepass.AddonManager = { REQUIRE_SIGNING: false, /* and other properties */ };

This should be fixable by calling Object.freeze(this) at the end of the addon manager jsm.

The next doubt is with code importing the AddonManager with Cu.import() and wants to use REQUIRE_SIGNING. Imagine this:

var bsp = Components.utils.import("resource://gre/modules/addons/XPIProvider.jsm");
bsp.AddonManger = { REQUIRE_SIGNING: false, /* and other properties */ };

Now unless I've missed something the XPIProvider would think signing is not required. This could also be solved by freezing the global scope in XPIProvider, but then developers would have to make sure that any time REQUIRE_SIGNING/ADDON_SIGNING is used from the Addon Manager, the scope it is imported into is frozen. This might be easy to miss during a review.

I am not sure it works, but reading <http://mxr.mozilla.org/comm-central/source/mozilla/testing/xpcshell/head.js#1539> it looks like an alternative would be to check mozinfo.addon_signing in unit tests and leaving the ifdef'd defines as they are.

Let me know if I am being too cautious.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> This should be fixable by calling Object.freeze(this) at the end of the
> addon manager jsm.
Which won't work, because then XPCOMUtils.defineLazyModuleGetter won't be able to define anything.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> (In reply to Dave Townsend [:mossop] from comment #2)
> > (In reply to Philipp Kewisch [:Fallen] from comment #1)
> > undefined is distinct from SIGNEDSTATE_MISSING because it has a different
> > meaning, it means that this add-on doesn't need to be signed and so the
> > signedState doesn't need to be checked.
> Ah, thanks for the clarification. I was thinking maybe to add a
> SIGNEDSTATE_NOT_REQUIRED and give that the value undefined, what do you
> think? This way we can still use the comparison to our advantage, but don't
> use the magic value undefined directly.
> 
> > > In any case, I'd be moving ADDON_SIGNING and SIGNING_REQUIRED from
> > > XPIProvider to AddonManager so it can be used in tests that check for
> > > specific values of signedState although they are not tests specific to
> > > signing.
> > 
> > This sounds good, while you're there would you mind updating these locations
> > to use that?
> I can do that. While writing the code I had some doubts about moving this to
> AddonManager in general. First of all, I was thinking evil code could do:
> 
> var backstagepass =
> Components.utils.import("resource://gre/modules/AddonManager.jsm");
> backstagepass.AddonManager = { REQUIRE_SIGNING: false, /* and other
> properties */ };
> 
> This should be fixable by calling Object.freeze(this) at the end of the
> addon manager jsm.
> 
> The next doubt is with code importing the AddonManager with Cu.import() and
> wants to use REQUIRE_SIGNING. Imagine this:
> 
> var bsp =
> Components.utils.import("resource://gre/modules/addons/XPIProvider.jsm");
> bsp.AddonManger = { REQUIRE_SIGNING: false, /* and other properties */ };
> 
> Now unless I've missed something the XPIProvider would think signing is not
> required. This could also be solved by freezing the global scope in
> XPIProvider, but then developers would have to make sure that any time
> REQUIRE_SIGNING/ADDON_SIGNING is used from the Addon Manager, the scope it
> is imported into is frozen. This might be easy to miss during a review.

You could just define it as a getter on AddonManager itself which is already frozen, then it would be reasonably safe from this kind of attack. But there are probably already numerous ways to circumvent signing if you have code running in-product like this, we can protect against some of it but not all.

> I am not sure it works, but reading
> <http://mxr.mozilla.org/comm-central/source/mozilla/testing/xpcshell/head.
> js#1539> it looks like an alternative would be to check
> mozinfo.addon_signing in unit tests and leaving the ifdef'd defines as they
> are.

Huh, I didn't know that trick, that looks like a good idea!

Comment 6

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=comm-central&job_id=22994
repository: comm-central
start_time: 2015-08-30T04:13:44
who: mozilla[at]kewis[dot]ch
machine: tst-linux32-spot-347
buildname: Ubuntu VM 12.04 comm-central opt test xpcshell
revision: 8fe4ce806f4b

TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js | run_test - [run_test : 94] 0 == 1
TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/unit/test_context_menu.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | test_addonsAndPlugins - [test_addonsAndPlugins : 479] signedState must have the correct type. - "undefined" == "number"
TEST-UNEXPECTED-FAIL | all-test-dirs.list:toolkit/mozapps/extensions/test/xpcshell/test_hotfix_cert.js | xpcshell return code: 0
TEST-UNEXPECTED-TIMEOUT | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out
Automation Error: mozprocess timed out after 1000 seconds running ['/builds/slave/test/build/venv/bin/python', '-u', '/builds/slave/test/build/tests/xpcshell/runxpcshelltests.py', '--symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/thunderbird/tinderbox-builds/comm-central-linux/1440929080/thunderbird-43.0a1.en-US.linux-i686.crashreporter-symbols.zip', '--test-plugin-path=/builds/slave/test/build/application/thunderbird/plugins', '--log-raw=/builds/slave/test/build/bl
timed out after 1000 seconds of no output
Return code: -9
No tests run or test summary not found

Comment 7

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=comm-central&job_id=23000
repository: comm-central
start_time: 2015-08-30T05:20:53
who: mozilla[at]kewis[dot]ch
machine: tst-linux64-spot-713
buildname: Ubuntu VM 12.04 x64 comm-central debug test xpcshell
revision: 8fe4ce806f4b

TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js | run_test - [run_test : 94] 0 == 1
TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/unit/test_context_menu.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | test_addonsAndPlugins - [test_addonsAndPlugins : 479] signedState must have the correct type. - "undefined" == "number"
TEST-UNEXPECTED-FAIL | all-test-dirs.list:toolkit/mozapps/extensions/test/xpcshell/test_hotfix_cert.js | xpcshell return code: 0
TEST-UNEXPECTED-TIMEOUT | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out
Return code: 1

Comment 8

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=comm-central&job_id=23002
repository: comm-central
start_time: 2015-08-30T05:22:52
who: mozilla[at]kewis[dot]ch
machine: tst-linux32-spot-693
buildname: Ubuntu VM 12.04 comm-central debug test xpcshell
revision: 8fe4ce806f4b

TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js | run_test - [run_test : 94] 0 == 1
TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/unit/test_context_menu.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | test_addonsAndPlugins - [test_addonsAndPlugins : 479] signedState must have the correct type. - "undefined" == "number"
TEST-UNEXPECTED-FAIL | all-test-dirs.list:toolkit/mozapps/extensions/test/xpcshell/test_hotfix_cert.js | xpcshell return code: 0
TEST-UNEXPECTED-TIMEOUT | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out
TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out
Return code: 1

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

I went with the mozinfo solution now and didn't make any changes to where the variables are defined, because I didn't want to add more to potential security issues.

This patch fixes most of the related comm issues, there is one more with test_webextension but this may also be a different issue:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f8208bd2ac5c

I did a try run on m-c too, the two remaining oranges that didn't go away after a retrigger don't seem related:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d0bdc833055

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8654728 [details] [diff] [review]
Fix - v1

Nice work, thanks

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/616963581706

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/616963581706

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8654728 [details] [diff] [review]
Fix - v1

Approval Request Comment
[Feature/regressing bug #]: addon signing
[User impact if declined]: none, mostly test only (to fix comm tests)
[Describe test coverage new/current, TreeHerder]: try runs, green on m-c
[Risks and why]: none expected
[String/UUID change made/needed]: none

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8654728 [details] [diff] [review]
Fix - v1

I would prefer to uplift this fix to Aurora and confirm the intermittent test failures are gone before uplifting to Beta. Aurora42+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0018e75dd248

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out of aurora in https://hg.mozilla.org/releases/mozilla-aurora/rev/1d9d6a7a958a for xpcshell test failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=1100381&repo=mozilla-aurora

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8654728 [details] [diff] [review]
Fix - v1

No wonder you didn't know about the global mozinfo, it was just added 10 days ago in bug 1150818. This seemed so natural to me that I didn't even consider it might not have been around for long. I'll see if I can get that bug backported before I request this one again.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8ea21b86a82

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Bug 1150818 is test-only, so I went ahead and did the uplift so this can re-land. That said, I checked and they didn't apply cleanly to beta, so you may want to rebase those patches and attach them to the bug so they can land when this gets approval.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Patch for beta - v1

The original patch had rejections on test_webextension, because this test doesn't exist on beta yet. Here is the same patch but without that test changed.

Pushing this will require bug 1150818 to be backported to beta too.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8656845 [details] [diff] [review]
Patch for beta - v1

This patch is mostly to fix intermittent test failures. Seems safe to uplift to Beta41.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f4838a6318d