Closed Bug 1197617 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: aleth, Assigned: Fallen)

Details

Attachments

(2 files)

Fix - v1
8.63 KB, patch
mossop
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Patch for beta - v1
6.20 KB, patch
Fallen
: review+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
      No description provided.
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?
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Flags: needinfo?(dtownsend)
(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
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
(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.
(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!
Flags: needinfo?(dtownsend)
Attached patch Fix - v1 — — Splinter Review 
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
Attachment #8654728 - Flags: review?(dtownsend)
Component: General → Add-ons Manager
Product: Thunderbird → Toolkit
Comment on attachment 8654728 [details] [diff] [review]
Fix - v1

Nice work, thanks
Attachment #8654728 - Flags: review?(dtownsend) → review+
Keywords: intermittent-failure → checkin-needed
https://hg.mozilla.org/mozilla-central/rev/616963581706
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Attachment #8654728 - Flags: approval-mozilla-beta?
Attachment #8654728 - Flags: approval-mozilla-aurora?
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+
Attachment #8654728 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(philipp)
Attachment #8654728 - Flags: approval-mozilla-beta?
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.
status-firefox42: fixed → affected
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.
Attachment #8656845 - Flags: review+
Attachment #8656845 - Flags: approval-mozilla-beta?
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.
Attachment #8656845 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: