Last Comment Bug 787253 - Addons Manager XPCShellTests that set AddonManager.checkCompatibility too early are failing on Aurora 17 nightly builds
: Addons Manager XPCShellTests that set AddonManager.checkCompatibility too ear...
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: unspecified
: All All
: -- blocker (vote)
: mozilla18
Assigned To: Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
:
Mentors:
Depends on: 788378
Blocks: 741972
  Show dependency treegraph
 
Reported: 2012-08-30 16:08 PDT by Alex Keybl [:akeybl]
Modified: 2012-10-16 14:54 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
Patch v1 (3.97 KB, patch)
2012-09-02 19:55 PDT, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
dtownsend: review+
Details | Diff | Review

Description Alex Keybl [:akeybl] 2012-08-30 16:08:59 PDT
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) 2012-08-30 16:18:14 PDT
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) 2012-08-30 16:26:17 PDT
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: gavin@gavinsharp.com] 2012-08-30 16:29:46 PDT
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: gavin@gavinsharp.com] 2012-08-30 16:31:50 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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: gavin@gavinsharp.com] 2012-08-30 16:33:13 PDT
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] 2012-08-30 16:43:13 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] (mostly unavailable, needinfo open, reviews not) 2012-08-30 19:23:54 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] (mostly unavailable, needinfo open, reviews not) 2012-09-02 19:55:15 PDT
Created attachment 657755 [details] [diff] [review]
Patch v1
Comment 9 Phil Ringnalda (:philor) 2012-09-04 09:05:38 PDT
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] 2012-09-04 09:15:20 PDT
(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: gavin@gavinsharp.com] 2012-09-04 13:02:00 PDT
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] 2012-09-04 15:46:19 PDT
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] (mostly unavailable, needinfo open, reviews not) 2012-09-04 19:35:40 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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] (mostly unavailable, needinfo open, reviews not) 2012-09-04 20:23:47 PDT
https://hg.mozilla.org/integration/fx-team/rev/19450d9f0f4c
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ee9ade752e4
Comment 15 Tim Taubert [:ttaubert] 2012-09-05 04:21:11 PDT
https://hg.mozilla.org/mozilla-central/rev/19450d9f0f4c

Note You need to log in before you can comment on or make changes to this bug.