Closed Bug 1333692 Opened 8 years ago Closed 8 years ago

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

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 54.0

People

(Reporter: jorgk-bmo, Assigned: aleth)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

1333692-sync-services.patch
(deleted), patch
Details | Diff | Splinter Review
Disable nsIIdentityCryptoService and jwcrypto tests for TB as these modules are only used by FxAccounts
(deleted), patch
MattN
: review+
stefanh
: feedback+
Details | Diff | Splinter Review
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)
Keywords: intermittent-failure
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.
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.
Or the tests could be turned off (I'm not sure you use these files anyway).
Flags: needinfo?(stefanh)
(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.
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.
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.
Looks like we need to package services-sync.js, which suite already has.
Attached patch 1333692-sync-services.patch (deleted) — — Splinter Review
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
https://hg.mozilla.org/comm-central/rev/4f7f9152fbfb9446c4fed53e5e9cc841785acd65
Status: ASSIGNED → RESOLVED
Closed: 8 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.
(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
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.
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
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)
(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".
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?
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)
(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.
(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.
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 → ---
Assignee: jorgk → nobody
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.
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: 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+
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.
https://hg.mozilla.org/mozilla-central/rev/dfbdda3b1285
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
(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.

Attachment

General

Creator:
Created:
Updated:
Size: