1264994 - xpcshell test failures with signing enabled

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Kim Moir [:kmoir] ET]

I ran a try run to figure to figure out why tests failed when signing was enabled.  The following xpcshell tests failed

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bd9b610af16202886ba9c8d73c18763db0ef798&selectedJob=19493604 

I talked to ahal in bug 1186522
https://bugzilla.mozilla.org/show_bug.cgi?id=1186522#c99

and he advised to open a bug to disable these on beta and fix on other branches

 TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | xpcshell return code: 0

1240868 TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js (perma-fail on comm-central)

    1143117 TEST-UNEXPECTED-TIMEOUT | Intermittent test_TelemetryEnvironment.js | Test timed out (Android)

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

1240868 TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js (perma-fail on comm-central)

    1143117 TEST-UNEXPECTED-TIMEOUT | Intermittent test_TelemetryEnvironment.js | Test timed out (Android)

TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | xpcshell return code: 0

    1200128 TEST-UNEXPECTED-TIMEOUT | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out

TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | - 2 == "undefined"

    1200128 TEST-UNEXPECTED-TIMEOUT | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_temporary.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_temporary.js | - 2 == "undefined"
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_proxy.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_proxy.js | - 2 == "undefined"
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | xpcshell return code: 0

    1200128 TEST-UNEXPECTED-TIMEOUT | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | - 2 == "undefined"

    1200128 TEST-UNEXPECTED-TIMEOUT | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension.js | Test timed out

Return code: 1

Comment 1

[Kim Moir [:kmoir] ET]

jgriffin: who would be able to look at this since ahal is away?

Comment 2

[Jonathan Griffin (:jgriffin)]

Dave, do you know what's going on here?

Comment 3

[Dave Townsend [:mossop]]

It's probably tripping up on system add-ons which don't need to be signed and so their signedState value is "undefined"

Comment 4

[Kim Moir [:kmoir] ET]

Okay, so what steps are needed to fix it?  Do we need to land unsigned addons in tree in this case, and which ones?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Kim Moir [:kmoir] from comment #4)
> Okay, so what steps are needed to fix it?  Do we need to land unsigned
> addons in tree in this case, and which ones?

It's probably fixing the test to allow for add-ons to not have a signedState. Georg might be able to help.

Comment 6

[Kim Moir [:kmoir] ET]

:gfritzsche: would you be able to provide insight on this bug? If not, could you suggest someone who might be able to help?

Comment 7

[Georg Fritzsche [:gfritzsche]]

I'm out this week, maybe Alessio has an idea in the mean-time.
Otherwise i'll follow up next week.

Comment 8

[Alessio Placitelli [:Dexter]]

I think this is a consequence of bug 1244357 and bug 1186522. As far as I can tell, bug 1244357 rightfully fakes the signed state and reports all the extensions to be signed.

In test_TelemetryEnvironment.js [1], we expect the signedState to be there only if |mozinfo.addon_signing| is true. This seems not to be the case, as both |mozinfo.addon_signing| and |mozinfo.required_signing| are false within this test.

So, when it comes to checking the expected values [2], we expect no |signedState| for addons but, instead, find them to be reported as "signed" (and thus have a valid |signedState|).

System addons seem to work as expected.

[1] - https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#1046
[2] - https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#589

Comment 9

[Kim Moir [:kmoir] ET]

Alessio, thanks for the update, I really appreciate it.  So in order to resolve this issue is the solution to change the test since the shim in bug 1244357 make the add-ons appear signed when they aren't?

Comment 10

[Alessio Placitelli [:Dexter]]

Don't mention it, glad to be helpful.

I'm trying to dig more info about |mozinfo.addon_signing| and |mozinfo.required_signing|: I'm not familiar with them and I would like to understand if they need to be consistent with the shim from bug 1244357.

If I'm not mistaken, we're always reporting addons as signed due to bug 1244357. If so, my doubt is: should |mozinfo.addon_signing| and |mozinfo.required_signing| report something other than false? If that's the case, we should probably fix those. Otherwise, we should change the tests.

Comment 11

[Kim Moir [:kmoir] ET]

So I'm not familiar with the tests as I don't work on the ateam.  Do you know the answer to this (or doing more digging) or you asking for my input?

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Kim Moir [:kmoir] from comment #11)
> So I'm not familiar with the tests as I don't work on the ateam.  Do you
> know the answer to this (or doing more digging) or you asking for my input?

No, I'm digging into that, sorry for the confusion.

Comment 13

[Alessio Placitelli [:Dexter]]

I had a deeper look at this, and I dumped the value of |substs.get('MOZ_ADDON_SIGNING')| and |substs.get('MOZ_REQUIRE_SIGNING')| from [1] and they seem to be "null", even though I export them in my .mozconfig.

This explains why |mozinfo.addon_signing| and |mozinfo.required_signing| are both false while, for example, AppConstants.MOZ_REQUIRE_SIGNING has the right value.

The defines are probably getting messed up somewhere, but I don't know much more about build config :(

[1] - https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/python/mozbuild/mozbuild/mozinfo.py#95-96

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #13)
> The defines are probably getting messed up somewhere, but I don't know much
> more about build config :(

Kim, can you check into that? Or do you know who could?

Comment 15

[Kim Moir [:kmoir] ET]

Yes, I'm looking at it, if I get stuck I'll talk to someone on the build team.

Comment 16

[Kim Moir [:kmoir] ET]

another try run, if this doesn't fix it I'll talk to someone with more mozconfig experience
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df64cfc1699e

Comment 17

[Kim Moir [:kmoir] ET]

So the try run from last night did not have xpcshell failures but it did have other failures

(the try run last night's was run again mozilla-beta code with the latest patches from bug ) The previous try run was on m-i

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

It looks like some addons could not be installed due to signing issues.  This makes we wonder if 1) all the signed addons that were landed on m-i were uplifted to beta or 2) if there is some issue with using beta for a try run.  

Also, if you look at the config of the resultant build xpinstall.signatures can still be toggled which I would have not have expected.

Comment 18

[Kim Moir [:kmoir] ET]

previous comment should have said xpinstall.signatures.required can be toggled

Comment 19

[Kim Moir [:kmoir] ET]

Dexter: How did you do this? I haven't done much with the build side of Firefox mostly tests.

I had a deeper look at this, and I dumped the value of |substs.get('MOZ_ADDON_SIGNING')| and |substs.get('MOZ_REQUIRE_SIGNING')| from [1] and they seem to be "null", even though I export them in my .mozconfig.

Comment 20

[Georg Fritzsche [:gfritzsche]]

As a quick hack, you could just error out when the build triggers that code:
https://pastebin.mozilla.org/8868906

I'd rather check changes based on m-c than m-i, as it should be more stable (less randomly broken).
We should probably first fix the mozinfo problem, then investigate any remaining issues afterwards?

Comment 21

[Kim Moir [:kmoir] ET]

gfritzsche:

Okay, I'll add that patch to the ones I submit and run another try run against m-c instead of m-i.

Comment 22

[Kim Moir [:kmoir] ET]

This was related to issues with mozconfigs in bug 1186522 and I have new patch approved with successful try run