TEST-UNEXPECTED-FAIL | services/crypto/tests/unit/test_jwcrypto.js - TEST-UNEXPECTED-FAIL | services/crypto/tests/unit/test_crypto_service.js

RESOLVED FIXED in Thunderbird 54.0

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: aleth)

Tracking

({intermittent-failure})

Trunk
Thunderbird 54.0
intermittent-failure

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
First seen
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=0e51f36ee7eb906e9131ea71e0f67d5439bf6667

TEST-UNEXPECTED-FAIL | services/crypto/tests/unit/test_jwcrypto.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | services/crypto/tests/unit/test_crypto_service.js | xpcshell return code: 0

Wed Jan 25, 2017 6:19:47, so it must come from the M-C merge at Wed Jan 25 02:53:42 2017 or Wed Jan 25 02:44:36 2017 or Wed Jan 25 00:08:23 2017.

This looks like it comes from bug 1313045. BTW, those tests are skipped for Android.

Stefan, can you please take a look. I can see that you already provided a patch in bug 1318447.
Flags: needinfo?(stefanh)
(Reporter)

Updated

2 years ago
Keywords: intermittent-failure

Comment 1

2 years ago
Absolutely, I'll take a look when I get home from work (19:00 UTC+1). The odd thing is that these tests doesn't fail when executed in mozilla-central (in Nightly), so I'm a bit puzzled.

Comment 2

2 years ago
I couldn't let this go and I think I know why the tests fail: im/ and mail/ doesn't package services-crypto-component.xpt (suite does, though). When I removed toolkit/identity I merged what was used in identity.xpt into services-crypto-component.xpt. Note also that IIRC these tests actually was B2G-only before, but I enabled them for everything except android (the files are used, so it makes sense to have test-coverage of them).

This will be the first thing I check when I get home, but if you feel this is urgent you can add services-crypto-component.xpt to im/installer/package-manifest.in and mail/installer/package-manifest.in and push it as a bustage-fix.

Comment 3

2 years ago
Or the tests could be turned off (I'm not sure you use these files anyway).
Flags: needinfo?(stefanh)

Comment 4

2 years ago
(In reply to Stefan [:stefanh] from comment #3)
> Or the tests could be turned off (I'm not sure you use these files anyway).

Um, well... The files are in m-c (sorry, a bit distracted) so I guess you can't.
(Reporter)

Comment 5

2 years ago
This typically happens if C-C and M-C aren't using the same "technology" (like we had cache tests fail when M-C was using cache2 and C-C was still on cache(1), or C-C doesn't support WebExtensions).

The most common cause however is that TB is simply missing a preference. Looking at the log at
https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1485321648/comm-central_win7_ix_test-xpcshell-bm127-tests1-windows-build4.txt.gz I see:

INFO -  NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
INFO -  IdentityLogger@resource://gre/modules/services-crypto/LogUtils.jsm:22:17
INFO -  @resource://gre/modules/services-crypto/LogUtils.jsm:101:15
INFO -  @resource://gre/modules/services-crypto/jwcrypto.jsm:17:1

Looking at LogUtils.jsm:22 we see this._debug = Services.prefs.getBoolPref(PREF_DEBUG); and further up const PREF_DEBUG = "services.sync.log.cryptoDebug";

I'm not sure why TB isn't picking this up from here:
https://dxr.mozilla.org/mozilla-central/rev/6dccae211ae5fec6a1c1244b878ce0b93860154f/services/sync/services-sync.js#75

Oh, looking at the comments that came in while writing this, most likely the missing services-crypto-component.xpt.
(Reporter)

Comment 6

2 years ago
Oops, we do package that here:
https://dxr.mozilla.org/comm-central/rev/60e4648f97111ce2d5cc2fd46b49c00fdfeda06a/mail/installer/package-manifest.in#198

(In reply to Stefan [:stefanh] from comment #4)
> The files are in m-c (sorry, a bit distracted) so I guess you can't.
We typically turn off tests in M-C for Thunderbird if required, but we prefer to avoid that.
(Reporter)

Comment 7

2 years ago
Looks like we need to package services-sync.js, which suite already has.
(Reporter)

Comment 8

2 years ago
Created attachment 8830248 [details] [diff] [review]
1333692-sync-services.patch

I'll push this soon. The M-C sheriff told me that he's doing a merge in the next hour, so I'll push this right afterwards.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Reporter)

Comment 9

2 years ago
https://hg.mozilla.org/comm-central/rev/4f7f9152fbfb9446c4fed53e5e9cc841785acd65
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
(In reply to Jorg K (GMT+1) from comment #6)
> Oops, we do package that here:
> https://dxr.mozilla.org/comm-central/rev/
> 60e4648f97111ce2d5cc2fd46b49c00fdfeda06a/mail/installer/package-manifest.
> in#198

im/installer/package-manifest.in doesn't package it, though. Not sure how I could have missed the fact that mail/ had it, but I guess I just noticed that it was missing from im/ and then assumed mail/ didn't had it.
(Reporter)

Comment 11

2 years ago
(In reply to Jorg K (GMT+1) from comment #9)
> https://hg.mozilla.org/comm-central/rev/4f7f9152fbfb9446c4fed53e5e9cc841785acd65

Hmm, that was the wrong thing to do since this busted everything completely :-(

Error: c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\mail\installer\package-manifest:242: Missing file(s): bin/defaults/pref/services-sync.js
(Reporter)

Comment 12

2 years ago
What have I done wrong here?
Flags: needinfo?(aleth)
There is btw a bug for nuking LogUtils.jsm and instead use the Log.jsm approach: bug 1333485.
(Reporter)

Comment 15

2 years ago
Well, for now let me try what suite/ does in include services-sync.js:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f7312a219f08a1c07d479f48f71446296d57bfee
(Assignee)

Comment 16

2 years ago
I don't think TB builds sync (nor should it have to), so therefore packaging it will fail. 

What does services-crypto-component do? c-c does use web crypto (window.crypto and friends) so we don't want to disable tests for that.
Flags: needinfo?(aleth)
(Assignee)

Comment 17

2 years ago
(In reply to Jorg K (GMT+1) from comment #15)
> Well, for now let me try what suite/ does in include services-sync.js:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=f7312a219f08a1c07d479f48f71446296d57bfee

By the way, you can test packaging issues locally by running "mach package".
(Reporter)

Comment 18

2 years ago
So what's the way forward here?
Backout
https://hg.mozilla.org/comm-central/rev/4f7f9152fbfb9446c4fed53e5e9cc841785acd65
and let the test fail.

Or disable the test? Or wait until bug 1333485 switches to from LogUtils.jsm to Log.jsm?
Or define preference services.sync.log.cryptoDebug ourselves?
(Assignee)

Comment 19

2 years ago
Backing out the services-sync packaging patch seems the right thing to do, yes.

For the failing tests, one has to figure out 1) if they test anything TB needs and/or 2) why they depend on a services.sync pref (which seems wrong unless it's definitely a test for something only used by Firefox accounts/sync). Then it'll become clear what the correct fix is.

stefanh probably knows the answer to 2) ;)
Flags: needinfo?(stefanh)
(Reporter)

Comment 20

2 years ago
(In reply to Jorg K (GMT+1) from comment #15)
> Well, for now let me try what suite/ does in include services-sync.js:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f7312a219f08a1c07d479f48f71446296d57bfee

That actually built in green. So how about landing that? Let me add an X to it on try.
Comment hidden (obsolete)
(Assignee)

Comment 22

2 years ago
(In reply to Jorg K (GMT+1) from comment #20)
> (In reply to Jorg K (GMT+1) from comment #15)
> > Well, for now let me try what suite/ does in include services-sync.js:
> > https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f7312a219f08a1c07d479f48f71446296d57bfee
> 
> That actually built in green. So how about landing that? Let me add an X to
> it on try.

Sure, that will build sync. I don't see why this is a good solution though, as Thunderbird does not use it.
(Reporter)

Comment 23

2 years ago
Backout:
https://hg.mozilla.org/comm-central/rev/c59a7076fb4f9427516ad8b55469f9e8f0d51f79

So what's next? Wait for the M-C guys to refactor this into a usable form, or add pref services.sync.log.cryptoDebug ourselves?

Chiaki: Please don't SPAM bugs with unrelated logs that don't help us. In general, logs should be added as attachments.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Updated

2 years ago
Assignee: jorgk → nobody
(Assignee)

Comment 24

2 years ago
It looks to me like the only user of nsIIdentityCryptoService is jwcrypto, and the only user of jwcrypto is FxAccounts. So they or at least their tests should really be made conditional on whether FxAccounts is built and MOZ_SERVICES_SYNC is set.

FxAccounts is currently built but not packaged (this could be improved), and its tests are disabled on TB:
http://searchfox.org/comm-central/source/mozilla/services/fxaccounts/tests/xpcshell/xpcshell.ini#3

Therefore the 'quick fix' here would be to do the same here and disable the failing two tests on TB as well. Better would be to check for MOZ_SERVICES_SYNC before building/testing the jwcrypto-related files stefanh moved, as they depend on sync.
(Assignee)

Comment 25

2 years ago
Hmm, I was going to add a patch that made building Services.cryto dependent on whether MOZ_SERVICES_SYNC is set, but that won't work because FxAccounts depends on crypto and is also built when MOZ_SERVICES_SYNC is *not* set. The way to resolve this would be to only build FxAccounts when MOZ_SERVICES_SYNC is set, but I don't know enough to know whether that would be correct (i.e. why this wasn't done in the first place).
(Assignee)

Comment 26

2 years ago
Created attachment 8830398 [details] [diff] [review]
Disable nsIIdentityCryptoService and jwcrypto tests for TB as these modules are only used by FxAccounts

Quick fix
Attachment #8830398 - Flags: feedback?(stefanh)
(Assignee)

Updated

2 years ago
Assignee: nobody → aleth
Status: REOPENED → ASSIGNED
Comment on attachment 8830398 [details] [diff] [review]
Disable nsIIdentityCryptoService and jwcrypto tests for TB as these modules are only used by FxAccounts

It appears that Thunderbird doesn't want these 2 tests, mind take a look?
Flags: needinfo?(stefanh)
Attachment #8830398 - Flags: review?(MattN+bmo)
Attachment #8830398 - Flags: feedback?(stefanh)
Attachment #8830398 - Flags: feedback+
Comment on attachment 8830398 [details] [diff] [review]
Disable nsIIdentityCryptoService and jwcrypto tests for TB as these modules are only used by FxAccounts

Review of attachment 8830398 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this is fine but we could have instead used Preferences.jsm to fallback to false when the pref doesn't exist.
Attachment #8830398 - Flags: review?(MattN+bmo) → review+

Comment 29

2 years ago
Pushed by stefanh@inbox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfbdda3b1285
Disable nsIIdentityCryptoService and jwcrypto tests for TB as these modules are only used by FxAccounts. r=MattN.

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dfbdda3b1285
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Comment 31

2 years ago
(In reply to Jorg K (GMT+1) from comment #23)

> Chiaki: Please don't SPAM bugs with unrelated logs that don't help us. In
> general, logs should be added as attachments.

Sorry. I will follow the attachment route.
You need to log in before you can comment on or make changes to this bug.