xpcshell test failures with signing enabled

RESOLVED FIXED

Status

Testing
XPCShell Harness
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kmoir, Assigned: kmoir)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Summary: XPCshell test failures with signing enabled → xpcshell test failures with signing enabled
(Assignee)

Comment 1

2 years ago
jgriffin: who would be able to look at this since ahal is away?
Flags: needinfo?(jgriffin)
Dave, do you know what's going on here?
Flags: needinfo?(jgriffin) → needinfo?(dtownsend)
It's probably tripping up on system add-ons which don't need to be signed and so their signedState value is "undefined"
Flags: needinfo?(dtownsend)
(Assignee)

Comment 4

2 years ago
Okay, so what steps are needed to fix it?  Do we need to land unsigned addons in tree in this case, and which ones?
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend) → needinfo?(gfritzsche)
(Assignee)

Comment 6

2 years ago
:gfritzsche: would you be able to provide insight on this bug? If not, could you suggest someone who might be able to help?
I'm out this week, maybe Alessio has an idea in the mean-time.
Otherwise i'll follow up next week.
Flags: needinfo?(alessio.placitelli)
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
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 9

2 years ago
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?
Flags: needinfo?(alessio.placitelli)
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.
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 11

2 years ago
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?
Flags: needinfo?(alessio.placitelli)
(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.
Flags: needinfo?(alessio.placitelli)
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
(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?
Flags: needinfo?(gfritzsche) → needinfo?(kmoir)
(Assignee)

Comment 15

2 years ago
Yes, I'm looking at it, if I get stuck I'll talk to someone on the build team.
Flags: needinfo?(kmoir)
(Assignee)

Comment 16

2 years ago
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
(Assignee)

Comment 17

2 years ago
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.
(Assignee)

Comment 18

2 years ago
previous comment should have said xpinstall.signatures.required can be toggled
(Assignee)

Comment 19

2 years ago
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.
Flags: needinfo?(alessio.placitelli)
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?
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 21

2 years ago
gfritzsche:

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

Comment 22

2 years ago
This was related to issues with mozconfigs in bug 1186522 and I have new patch approved with successful try run
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Assignee: nobody → kmoir
You need to log in before you can comment on or make changes to this bug.