787253 - Addons Manager XPCShellTests that set AddonManager.checkCompatibility too early are failing on Aurora 17 nightly builds

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Alex Keybl [:akeybl]]

Philor just pointed out that XPCShellTest is failing with PGO builds on Aurora 17 since the merge:

https://tbpl.mozilla.org/?tree=Mozilla-Aurora&onlyunstarred=1

We're planning on enabling Aurora builds tomorrow morning, so we need to decide whether this should be a blocker for the release. My feeling is no, if QA qualification is successful. But I also think Gavin will have a more informed opinion.

Comment 1

[Matt Brubeck (:mbrubeck)]

Not all PGO builds are affected; only nightly builds:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=e101e4c1dcf8

Both the "B" (dep) and "N" (nightly) builds use the same .mozconfig (as you can verify from the "cat .mozconfig" step in the build log) which has PGO enabled (the PROFILE_GEN_SCRIPT variable is set).

This happens even if the dep build is a forced clobber, as in the Win opt B here:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=380fdb7249aa&jobname=win

Comment 2

[Matt Brubeck (:mbrubeck)]

The .mozconfig has a couple of variables that could lead to different configuration depending on the build environment; in particular MOZ_UPDATE_CHANNEL="default" for dep builds and MOZ_UPDATE_CHANNEL="aurora" for nightly builds.  So probably some code or test is breaking when the update channel is set to "aurora".

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I think this was caused by bug 741972. The checkCompatibility setter is being called while PREF_EM_CHECK_COMPATIBILITY is undefined (i.e. AddonManagerInternal.startup hasn't been called), so we get:

resource://gre/modules/AddonManager.jsm:1951: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIPrefBranch.setBoolPref]

This is only a problem on Aurora (and beta, esr, nightly) because MOZ_COMPATIBILITY_NIGHTLY isn't defined there, and so PREF_EM_CHECK_COMPATIBILITY doesn't have a default value.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> This is only a problem on Aurora (and beta, esr, nightly) because

Er, not nightly :) It's a problem for any build where MOZ_UPDATE_CHANNEL is one of "aurora beta release esr", per http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/Makefile.in#12.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

AFAICT this is a test-only issue, so it doesn't need to block our re-enabling Aurora updates tomorrow.

Comment 6

[Alex Keybl [:akeybl]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> AFAICT this is a test-only issue, so it doesn't need to block our
> re-enabling Aurora updates tomorrow.

Thanks Gavin!

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> I think this was caused by bug 741972. The checkCompatibility setter is
> being called while PREF_EM_CHECK_COMPATIBILITY is undefined (i.e.
> AddonManagerInternal.startup hasn't been called), so we get:

Ah, yes, that would make sense. And PREF_EM_CHECK_COMPATIBILITY needs to be set in startup() because the tests need to change the application version between startup and shutdown. Test fix coming up.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 9

[Phil Ringnalda (:philor)]

Looks like somebody decided to close the tree over it, so even though it doesn't block enabling nightlies, it does block landing anything.

Comment 10

[Ed Morley [:emorley]]

(In reply to Phil Ringnalda (:philor) from comment #9)
> Looks like somebody decided to close the tree over it, so even though it
> doesn't block enabling nightlies, it does block landing anything.

I closed aurora due to people landing on unstarred orange & due to the issue being unknown. Now that we have a bug & patch, I have reopened.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 657755 [details] [diff] [review]
Patch v1

Would it make sense to have the setter initialize the service as needed, rather than just adjusting all the callers? I guess this is an xpcshell-only problem so that's not worth it?

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 657755 [details] [diff] [review]
Patch v1

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

Can you also add a guard to make the checkCompatibility setter throw if we haven't been through startup yet, that should make sure we don't accidentally hit this again.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #11)
> Would it make sense to have the setter initialize the service as needed,
> rather than just adjusting all the callers? I guess this is an xpcshell-only
> problem so that's not worth it?

I want to avoid doing that, as it's an unexpected consequence. But Dave's suggestion would also solve the issue, and fits in nicely with recent work adding similar guards (bug 782881).

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/19450d9f0f4c
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ee9ade752e4

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/19450d9f0f4c