Closed Bug 1348981 Opened 8 years ago Closed 8 years ago

validate built-in system add-ons against a list of known IDs

Categories

(Toolkit :: Add-ons Manager, defect, P1)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Project Flags:
Performance Impact none
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: rhelmer, Assigned: rhelmer)

References

Details

(Whiteboard: triaged)

Attachments

(5 files, 1 obsolete file)

Bug 1348981 - provide a list of valid system add-ons for packaging
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1348981 - only load system add-ons from a built-in list
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1348981 - add Cu.readURI helper for sync file reading on startup
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1348981 - provide method to re-initialize URLPreloader for xpcshell tests
59 bytes, text/x-review-board-request
kmag
: review+
Details
beta 57 uplift request
1 byte, text/plain
ritu
: approval-mozilla-beta+
Details
Currently it's possible to drop an unsigned add-on into the Firefox application directory. While this does tend to require administrative access, it makes it easier than we'd like for malware to install a hidden add-on. Eventually we want built-in system add-ons to be signed (bug 1277920) but in the meantime one mitigation to make this a little harder would be to ship a list of built-in system add-on IDs with Firefox, and refuse to load any unexpected add-on ID.
Blocks: 1336576
Whiteboard: triaged
Whiteboard: triaged → [qf]triaged
Whiteboard: [qf]triaged → [qf-]triaged
Another benefit of this is that we can stop scanning the app system add-ons directory, which we're trying to move away from generally for perf reasons (especially startup).
Blocks: 1357205
Blocks: 1371273
status-firefox57: --- → affected
Priority: -- → P1
Attachment #8908271 - Attachment is obsolete: true
Attachment #8908271 - Flags: review?(aswan)
Comment on attachment 8908272 [details] Bug 1348981 - only load system add-ons from a built-in list https://reviewboard.mozilla.org/r/179928/#review185504 This is close, just a small collection of things to sort out... ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:108 (Diff revision 3) > const PREF_EM_LAST_APP_BUILD_ID = "extensions.lastAppBuildId"; > > +// Specify a list of valid built-in add-ons to load. > +// Only works in automation; otherwise comes from BUILT_IN_ADDONS_URI. > +const PREF_BUILT_IN_ADDONS = "extensions.builtInAddons"; > +const BUILT_IN_ADDONS_URI = "resource:///chrome/browser/content/browser/built_in_addons.js"; .js -> .json ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6477 (Diff revision 3) > + constructor(name, directory, scope) { > + super(name, directory, scope); > + > + this.locked = true; > + this._IDToFileMap = {}; > + } The things in the body here are already done in the superclass constructor, I think you can just leave the constructor out altogether. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6491 (Diff revision 3) > + let url = Services.io.newURI(BUILT_IN_ADDONS_URI); > + let response = await fetch(url.spec); Why use `newURI()` at all? ie, why not just pass BUILT_IN_ADDONS_URI straight to `fetch()` ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6495 (Diff revision 3) > + // Only override built-in list if in automation and pref is set. > + if (Cu.isInAutomation) { > + let addons = Services.prefs.getStringPref(PREF_BUILT_IN_ADDONS, ""); > + if (addons) { > + index = JSON.parse(addons); > + } > + } Instead of this, would it work to have tests use SpecialPowers to override the resource: url? That would let us avoid having this test-only code here... ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6514 (Diff revision 3) > + logger.warn("No list of valid system add-ons found."); > + return; > + } > + > + for (let id of index.system) { > + let file = new nsIFile(this._directory.path); I'm surprised this works, I thought you need to instantiate LocalFile or something and that FileUtils was the approved way to do that safely. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6518 (Diff revision 3) > + for (let id of index.system) { > + let file = new nsIFile(this._directory.path); > + file.append(`${id}.xpi`); > + > + if (!file.exists()) { > + // if the XPI file can't be loaded, try the directory. I guess we need this for development builds? Could we do this only if an appropriate AppConstants symbol is set?
Attachment #8908272 - Flags: review?(aswan) → review-
Comment on attachment 8908272 [details] Bug 1348981 - only load system add-ons from a built-in list https://reviewboard.mozilla.org/r/179928/#review185504 > .js -> .json JSON doesn't seem to load via resource:// as far as I can tell, this is what I tried initially. I'll look into it a bit more. > Why use `newURI()` at all? ie, why not just pass BUILT_IN_ADDONS_URI straight to `fetch()` I was thinking about validating the URL but `fetch` will do the same thing and through a perfectly readable exception so I'll skip it. > Instead of this, would it work to have tests use SpecialPowers to override the resource: url? That would let us avoid having this test-only code here... Recapping our IRC convo: we should be able to leave mochitests alone and just load the default set, xpcshell tests should be able to override the resource:// URL
Comment on attachment 8908272 [details] Bug 1348981 - only load system add-ons from a built-in list https://reviewboard.mozilla.org/r/179928/#review185694 Mostly looks good. We just need to deal with the `readFile` thing. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6475 (Diff revision 6) > + constructor(name, directory, scope) { > + super(name, directory, scope); > + } Nit: This is a no-op. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6484 (Diff revision 6) > + /** > + * Read the index and build a mapping between ID and URI for each > + * installed add-on. > + */ > + _readAddons() { > + let index; Hm. When I see `index`, I think integer. Maybe `manifest`? ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6486 (Diff revision 6) > + let url = Services.io.newURI(BUILT_IN_ADDONS_URI); > + let file = ChromeRegistry.convertChromeURL(url).QueryInterface(Ci.nsIFileURL).file; > + let data = Cu.readFile(file); We've already talked about what needs to happen here. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6505 (Diff revision 6) > + for (let id of index.system) { > + let file = new FileUtils.File(this._directory.path); > + file.append(`${id}.xpi`); > + > + // Only attempt to load unpacked directory if unofficial build. > + if (!file.exists() && !AppConstants.MOZILLA_OFFICIAL) { Nit: Please reverse the order of these checks so we don't do sync IO when we don't have to. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6506 (Diff revision 6) > + file = new FileUtils.File(this._directory.path); > + file.append(`${id}`); Nit: `file.leafName = id;` ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6510 (Diff revision 6) > + if (!file.exists() && !AppConstants.MOZILLA_OFFICIAL) { > + file = new FileUtils.File(this._directory.path); > + file.append(`${id}`); > + } > + > + if (!file.exists()) { Please just skip this check and assume the file exists. If it doesn't, we'll just get some warnings at startup that we can ignore. ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1585 (Diff revision 6) > do_check_eq(dirs.length, expectedDirs); > > await checkInstalledSystemAddons(...finalState, distroDir); > > // Check that the new state is active after a restart > - await promiseRestartManager(); > + shutdownManager(); Nit: `promiseShutdownManager` ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1587 (Diff revision 6) > await checkInstalledSystemAddons(...finalState, distroDir); > > // Check that the new state is active after a restart > - await promiseRestartManager(); > + shutdownManager(); > + await overrideBuiltIns({ "system": ["system1@tests.mozilla.org", "system2@tests.mozilla.org", "system3@tests.mozilla.org", "system4@tests.mozilla.org", "system5@tests.mozilla.org"] }); > + startupManager(); Nit: `promiseStartupManager` ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1650 (Diff revision 6) > + let manifest = Services.io.newFileURI( > + FileUtils.getFile("TmpD", "builtins.manifest")); The manifest path doesn't really matter here, since you're using absolute URLs. May as well just generate the URI for the override.txt file and use it in both places. ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1655 (Diff revision 6) > + let manifest = Services.io.newFileURI( > + FileUtils.getFile("TmpD", "builtins.manifest")); > + let file = FileUtils.getFile("TmpD", "override.txt"); > + await OS.File.writeAtomic(file.path, > + new TextEncoder().encode(JSON.stringify(data))); > + aomStartup.registerChrome(manifest, [ You'll need to store this result somewhere. The entries are automatically removed when it's GCed. ::: toolkit/mozapps/extensions/test/xpcshell/test_system_allowed.js:26 (Diff revision 6) > +add_task(async function test_allowed_addons() { > + // 1 and 2 are allowed, 3 is not. > + let validAddons = { "system": ["system1@tests.mozilla.org", "system2@tests.mozilla.org"]}; > + await overrideBuiltIns(validAddons); > + > + startupManager(); Nit: `await promiseStartupManager()` ::: toolkit/mozapps/extensions/test/xpcshell/test_system_allowed.js:29 (Diff revision 6) > + await overrideBuiltIns(validAddons); > + > + startupManager(); > + > + let addon = await AddonManager.getAddonByID("system1@tests.mozilla.org"); > + do_check_neq(addon, null); Nit: `notEqual(addon, null)` Same for below. And `equal` instead of `do_check_eq`
Attachment #8908272 - Flags: review?(kmaglione+bmo)
Comment on attachment 8908270 [details] Bug 1348981 - provide a list of valid system add-ons for packaging https://reviewboard.mozilla.org/r/179924/#review186352 ::: browser/app/Makefile.in:109 (Diff revision 3) > endif > printf APPLMOZB > $(dist_dest)/Contents/PkgInfo > endif > + > +.PHONY: features > +tools features:: You'll need to add something like: default: $(TOPOBJDIR)/browser/app/features to config/faster/rules.mk to make `mach build faster` update the file.
Attachment #8908270 - Flags: review?(mh+mozilla)
Chris, Mike, you'll probably need to find a more proper way to do this for the longer term. AIUI, this is a hack to make this work for 57.
Comment on attachment 8908272 [details] Bug 1348981 - only load system add-ons from a built-in list https://reviewboard.mozilla.org/r/179928/#review185694 > Nit: `file.leafName = id;` Hm, I don't think that's right - I want a subdir of `file` (the terminal node is the "features" dir)
The current set of patches works for me in a local build, but the xpcshell tests are failing when Cu.readURL is called: addons.xpi WARN List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIXPCComponents_Utils.readURL]" nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)" location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _readAddons :: line 6480" data: no] Which I suspect is coming from http://searchfox.org/mozilla-central/source/js/xpconnect/loader/URLPreloader.cpp#495
Comment on attachment 8908272 [details] Bug 1348981 - only load system add-ons from a built-in list https://reviewboard.mozilla.org/r/179928/#review186916
Attachment #8908272 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8909960 [details] Bug 1348981 - add Cu.readURI helper for sync file reading on startup https://reviewboard.mozilla.org/r/181440/#review186888 ::: js/xpconnect/idl/xpccomponents.idl:710 (Diff revision 2) > + > + /* > + * Reads the given local file URL and returns its contents. This has the > + * same semantics of readFile. > + */ > + ACString readURL(in nsIURI url); Nit: Should be called `readURI` if it's taking an nsIURI arg.
Attachment #8909960 - Flags: review?(kmaglione+bmo) → review+
(In reply to Robert Helmer [:rhelmer] from comment #24) > The current set of patches works for me in a local build, but the xpcshell > tests are failing when Cu.readURL is called: > > Which I suspect is coming from > http://searchfox.org/mozilla-central/source/js/xpconnect/loader/URLPreloader. > cpp#495 We should probably just add a method to aomStartup to initialize the preloader for xpcshell tests. It normally gets initialized by the toolkit profile service at startup, but that doesn't happen in xpcshell. You basically just need to call URLPreloader::GetSingleton().
Comment on attachment 8908270 [details] Bug 1348981 - provide a list of valid system add-ons for packaging https://reviewboard.mozilla.org/r/179924/#review187004 ::: config/faster/rules.mk:116 (Diff revision 5) > > $(TOPOBJDIR)/build/application.ini: $(TOPOBJDIR)/buildid.h $(TOPOBJDIR)/source-repo.h > + > +# The manifest of allowed system add-ons should be re-built when using > +# "build faster". > +default: $(TOPOBJDIR)/browser/app/features this should be wrapped in a `ifeq ($(MOZ_BUILD_APP),browser/app)` section.
Attachment #8908270 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8910904 [details] Bug 1348981 - provide method to re-initialize URLPreloader for xpcshell tests https://reviewboard.mozilla.org/r/182378/#review187662 ::: js/xpconnect/loader/URLPreloader.h:100 (Diff revision 1) > friend class ScriptPreloader; > + friend class AddonManagerStartup; Nit: Please keep sorted. ::: js/xpconnect/loader/URLPreloader.h:107 (Diff revision 1) > > virtual ~URLPreloader(); > > Result<Ok, nsresult> WriteCache(); > > + static mozilla::StaticRefPtr<URLPreloader> sSingleton; Nit: This should be private. ::: js/xpconnect/loader/URLPreloader.cpp:86 (Diff revision 1) > } > > return NS_OK; > } > > +StaticRefPtr<URLPreloader> URLPreloader::sSingleton; Nit: Please declare this in the same place as `sInitialized` ::: js/xpconnect/loader/URLPreloader.cpp:161 (Diff revision 1) > > +URLPreloader& > +URLPreloader::ReInitialize() > +{ > + sSingleton = new URLPreloader(); > + ClearOnShutdown(&sSingleton); No need for this. `ClearOnShutdown` only needs to be called once. It keeps a reference to the storage location, not the value that's stored in it.
Attachment #8910904 - Flags: review?(kmaglione+bmo) → review+
OK! Finally worked through all the test infra issues, green try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93dd3b443601946ceef0ffbbfa1dbd391bc9df82 Going to push one more small patch that matches this try run then land.
Pushed by rhelmer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a943c88d0dd5 provide a list of valid system add-ons for packaging r=glandium https://hg.mozilla.org/integration/autoland/rev/b5149335d9ed add Cu.readURI helper for sync file reading on startup r=kmag https://hg.mozilla.org/integration/autoland/rev/dc6f6f59da82 provide method to re-initialize URLPreloader for xpcshell tests r=kmag https://hg.mozilla.org/integration/autoland/rev/af30bdde5572 only load system add-ons from a built-in list r=kmag
(In reply to Wes Kocher (:KWierso) from comment #54) > I had to back these out for a couple xpcshell failures: > https://treeherder.mozilla.org/logviewer.html#?job_id=132773064&repo=autoland > https://treeherder.mozilla.org/logviewer.html#?job_id=132773106&repo=autoland > > https://hg.mozilla.org/integration/autoland/rev/ > 3dbb197bcbd4e68be2e0f948046309b307731a86 Thanks.. looks like I need to fix up the sync and telemetry tests too.
Flags: needinfo?(rhelmer)
Pushed by rhelmer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/58431e2c37d7 provide a list of valid system add-ons for packaging r=glandium https://hg.mozilla.org/integration/autoland/rev/5e93d8760487 add Cu.readURI helper for sync file reading on startup r=kmag https://hg.mozilla.org/integration/autoland/rev/47866f942752 provide method to re-initialize URLPreloader for xpcshell tests r=kmag https://hg.mozilla.org/integration/autoland/rev/b1fe39ea6d5c only load system add-ons from a built-in list r=kmag
Is doing that kind of slow? Backed out in https://hg.mozilla.org/integration/autoland/rev/67c35c76e9b5 for timeouts on both Linux32 and Linux64 debug in xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_system_update.js, https://treeherder.mozilla.org/logviewer.html#?job_id=132826862&repo=autoland
(In reply to Phil Ringnalda (:philor) from comment #59) > Is doing that kind of slow? > > Backed out in https://hg.mozilla.org/integration/autoland/rev/67c35c76e9b5 > for timeouts on both Linux32 and Linux64 debug in > xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_system_update.js, > https://treeherder.mozilla.org/logviewer.html#?job_id=132826862&repo=autoland Thanks. I'd expect this patch to improve startup perf (however slightly), actually... the test has to do more, and this test has always been on the edge of timing out. I'll see if I can split it up quickly.
Pushed by rhelmer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b8b9f1dbeda3 provide a list of valid system add-ons for packaging r=glandium https://hg.mozilla.org/integration/autoland/rev/f397bfb365e1 add Cu.readURI helper for sync file reading on startup r=kmag https://hg.mozilla.org/integration/autoland/rev/199b3fe8114f provide method to re-initialize URLPreloader for xpcshell tests r=kmag https://hg.mozilla.org/integration/autoland/rev/8dda8bbcf385 only load system add-ons from a built-in list r=kmag
Attached file beta 57 uplift request —
Approval Request Comment [Feature/Bug causing the regression]: n/a [User impact if declined]: possibility for malware (with admin access) to install hidden add-ons, and/or users to drop them in and possibly breaking Firefox [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: Try dropping an unsigned XPI into the "features" folder, in the Firefox application folder [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: simple change (validate against a list produced by the build), extensive tests [String changes made/needed]: n/a
Attachment #8914928 - Flags: approval-mozilla-beta?
Backed out for failing xpcshell toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js: https://hg.mozilla.org/integration/autoland/rev/500ec356e44590783cba43878484bd67b9eed20f https://hg.mozilla.org/integration/autoland/rev/644aa554887b049258d6ee6be2b6ba132469ff09 https://hg.mozilla.org/integration/autoland/rev/f5379d9a6537182cf651edd5e6b7fa0ec709fdd9 https://hg.mozilla.org/integration/autoland/rev/32e74dda75a7e57715c8bffc590bb915f3e3873c Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8dda8bbcf38507bba732c325dc88e1af5846a0dd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134763403&repo=autoland [task 2017-10-03T22:24:00.900Z] 22:24:00 INFO - toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | Starting test_checkEnvironment [task 2017-10-03T22:24:00.903Z] 22:24:00 INFO - (xpcshell/head.js) | test test_checkEnvironment pending (2) [task 2017-10-03T22:24:00.905Z] 22:24:00 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | test_checkEnvironment - [test_checkEnvironment : 907] addons database is not loaded - true == false [task 2017-10-03T22:24:00.906Z] 22:24:00 INFO - /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:test_checkEnvironment:907 [task 2017-10-03T22:24:00.907Z] 22:24:00 INFO - exiting test
Flags: needinfo?(rhelmer)
Fixed the telemetry test, and re-running try with both the telemetry and sync tests included before re-landing...
Flags: needinfo?(rhelmer)
Pushed by rhelmer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/59d279deac34 provide a list of valid system add-ons for packaging r=glandium https://hg.mozilla.org/integration/autoland/rev/a8a2b3d99ed9 add Cu.readURI helper for sync file reading on startup r=kmag https://hg.mozilla.org/integration/autoland/rev/d96c54a045a4 provide method to re-initialize URLPreloader for xpcshell tests r=kmag https://hg.mozilla.org/integration/autoland/rev/fe9b8761d51f only load system add-ons from a built-in list r=kmag
Backed out for frequently failing xpcshell's toolkit/mozapps/extensions/test/xpcshell/test_system_update_empty.js on OS X 10.10 debug: https://hg.mozilla.org/integration/autoland/rev/83d470577ac48911401825f3f6de44aea4636fab https://hg.mozilla.org/integration/autoland/rev/e4623e5244a61bb672e3ef881aa80e29cc823c89 https://hg.mozilla.org/integration/autoland/rev/18fa0c044db59f77e2febf60db19c33e25e3a95b https://hg.mozilla.org/integration/autoland/rev/9afb9d8c2842b75a9bc98ffb07c5475dcdddeb3d After the job got added to the Try push, it also failed there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd679897bffede06cfcd91cdf103d4beb13d876f First push with failure (yours was busted by a previous push): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8321ffbbc47ba84d52228068f4ec425f28463c35&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134884525&repo=autoland 05:35:15 INFO - PID 5681 | 1507120514331 addons.xpi-utils DEBUG Add-on system3@tests.mozilla.org modified in app-system-addons 05:35:15 INFO - PID 5681 | 1507120514337 DeferredSave.extensions.json DEBUG Starting timer 05:35:15 INFO - "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "applySoftBlock"" {file: "resource://gre/modules/addons/XPIProvider.jsm" line: 5144}]" 05:35:15 INFO - "CONSOLE_MESSAGE: (info) 1507120514326 DeferredSave.extensions.json DEBUG Save changes" 05:35:15 INFO - "CONSOLE_MESSAGE: (info) 1507120514328 DeferredSave.extensions.json DEBUG Save changes" 05:35:15 INFO - "CONSOLE_MESSAGE: (info) 1507120514331 addons.xpi-utils DEBUG Add-on system3@tests.mozilla.org modified in app-system-addons" 05:35:15 INFO - "CONSOLE_MESSAGE: (info) 1507120514337 DeferredSave.extensions.json DEBUG Starting timer" 05:35:15 INFO - PID 5681 | 1507120514350 DeferredSave.extensions.json DEBUG Save changes 05:35:15 INFO - PID 5681 | 1507120514353 DeferredSave.extensions.json DEBUG Save changes 05:35:15 INFO - PID 5681 | 1507120514354 addons.xpi-utils DEBUG New add-on system1@tests.mozilla.org installed in app-system-defaults 05:35:15 INFO - PID 5681 | 1507120514363 DeferredSave.extensions.json DEBUG Save changes 05:35:15 INFO - PID 5681 | 1507120514363 addons.xpi-utils DEBUG New add-on system2@tests.mozilla.org installed in app-system-defaults 05:35:15 INFO - "CONSOLE_MESSAGE: (info) 1507120514350 DeferredSave.extensions.json DEBUG Save changes" 05:35:15 INFO - PID 5681 | 1507120514374 DeferredSave.extensions.json DEBUG Save changes 05:35:15 INFO - PID 5681 | 1507120514376 addons.manager DEBUG Registering startup change 'changed' for system2@tests.mozilla.org 05:35:15 INFO - PID 5681 | JavaScript strict warning: resource://gre/modules/addons/XPIProvider.jsm, line 2783: ReferenceError: reference to undefined property "version" 05:35:15 INFO - PID 5681 | 1507120514379 addons.xpi DEBUG Loading bootstrap scope from /var/folders/xv/qyd_w4w92w3dzk8k89k6bmvr00000w/T/xpc-profile-aXLyjx/features/prefilled/system2@tests.mozilla.org.xpi 05:35:15 INFO - PID 5681 | 1507120514382 addons.xpi DEBUG Calling bootstrap method install on system2@tests.mozilla.org version 2.0 05:35:15 WARNING - TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_system_update_empty.js | - false == true 05:35:15 INFO - /Users/cltbld/tasks/task_1507117654/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/head_addons.js:checkAddonNotInstalled:279 05:35:15 INFO - /Users/cltbld/tasks/task_1507117654/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/head_addons.js:observe:299 05:35:15 INFO - resource://xpcshell-data/BootstrapMonitor.jsm:notify:17 05:35:15 INFO - resource://gre/modules/addons/XPIProvider.jsm:callBootstrapMethod:4509 05:35:15 INFO - resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:processFileChanges:1689 05:35:15 INFO - resource://gre/modules/addons/XPIProvider.jsm:checkForChanges:3265 05:35:15 INFO - resource://gre/modules/addons/XPIProvider.jsm:startup:2182 05:35:15 INFO - resource://gre/modules/AddonManager.jsm:callProvider:263 05:35:15 INFO - resource://gre/modules/AddonManager.jsm:_startProvider:730 05:35:15 INFO - resource://gre/modules/AddonManager.jsm:startup:897 05:35:15 INFO - resource://gre/modules/AddonManager.jsm:startup:3082 05:35:15 INFO - jar:file:///Users/cltbld/tasks/task_1507117654/build/application/NightlyDebug.app/Contents/Resources/omni.ja!/components/addonManager.js:observe:65 05:35:15 INFO - resource://testing-common/AddonTestUtils.jsm:promiseStartupManager:585 05:35:15 INFO - exiting test
Flags: needinfo?(rhelmer)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #75) > Backed out for frequently failing xpcshell's > toolkit/mozapps/extensions/test/xpcshell/test_system_update_empty.js on OS X > 10.10 debug: > > https://hg.mozilla.org/integration/autoland/rev/ > 83d470577ac48911401825f3f6de44aea4636fab > https://hg.mozilla.org/integration/autoland/rev/ > e4623e5244a61bb672e3ef881aa80e29cc823c89 > https://hg.mozilla.org/integration/autoland/rev/ > 18fa0c044db59f77e2febf60db19c33e25e3a95b > https://hg.mozilla.org/integration/autoland/rev/ > 9afb9d8c2842b75a9bc98ffb07c5475dcdddeb3d > > After the job got added to the Try push, it also failed there: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=fd679897bffede06cfcd91cdf103d4beb13d876f > > First push with failure (yours was busted by a previous push): > https://treeherder.mozilla.org/#/ > jobs?repo=autoland&revision=8321ffbbc47ba84d52228068f4ec425f28463c35&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&filter-resultStatus=usercancel&filter- > resultStatus=runnable > Failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=134884525&repo=autoland Hm. So we already disabled this on Linux for this same failure - there's a race condition in this test. I'll take a quick shot at tracking down the underlying issue here, and disable it if it's still elusive.
Flags: needinfo?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #76) > (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on > intermittent or backout) from comment #75) > > Backed out for frequently failing xpcshell's > > toolkit/mozapps/extensions/test/xpcshell/test_system_update_empty.js on OS X > > 10.10 debug: > > > > https://hg.mozilla.org/integration/autoland/rev/ > > 83d470577ac48911401825f3f6de44aea4636fab > > https://hg.mozilla.org/integration/autoland/rev/ > > e4623e5244a61bb672e3ef881aa80e29cc823c89 > > https://hg.mozilla.org/integration/autoland/rev/ > > 18fa0c044db59f77e2febf60db19c33e25e3a95b > > https://hg.mozilla.org/integration/autoland/rev/ > > 9afb9d8c2842b75a9bc98ffb07c5475dcdddeb3d > > > > After the job got added to the Try push, it also failed there: > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=fd679897bffede06cfcd91cdf103d4beb13d876f > > > > First push with failure (yours was busted by a previous push): > > https://treeherder.mozilla.org/#/ > > jobs?repo=autoland&revision=8321ffbbc47ba84d52228068f4ec425f28463c35&filter- > > resultStatus=testfailed&filter-resultStatus=busted&filter- > > resultStatus=exception&filter-resultStatus=usercancel&filter- > > resultStatus=runnable > > Failure log: > > https://treeherder.mozilla.org/logviewer.html#?job_id=134884525&repo=autoland > > Hm. So we already disabled this on Linux for this same failure - there's a > race condition in this test. > > I'll take a quick shot at tracking down the underlying issue here, and > disable it if it's still elusive. I went ahead and disabled the test - the actual thing it is testing works fine, I'll track down the race condition here in a follow-up. I have high confidence that this is a race condition in the test and not the code.
Clean try run attached to the review - the split-up "empty" update test that's intermittent on all platforms (and perma-fails on Linux) is disabled with a comment now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=461f8b1d5a1e
Pushed by rhelmer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5ead3eb427c7 provide a list of valid system add-ons for packaging r=glandium https://hg.mozilla.org/integration/autoland/rev/f89549271025 add Cu.readURI helper for sync file reading on startup r=kmag https://hg.mozilla.org/integration/autoland/rev/388768fdc403 provide method to re-initialize URLPreloader for xpcshell tests r=kmag https://hg.mozilla.org/integration/autoland/rev/34663fd2712d only load system add-ons from a built-in list r=kmag
I'd like this to bake a bit on nightly before uplifting to beta. will most likely go in b7.
This brought noticeable improvements on Android! Thanks! == Change summary for alert #9840 (as of October 05 2017 00:12 UTC) == Improvements: 6% remote-blank summary android-4-4-armv7-api16 opt 806.91 -> 761.60 6% remote-twitter summary android-4-4-armv7-api16 opt 1,344.26 -> 1,269.34 6% remote-blank summary android-4-2-armv7-api16 opt 1,291.71 -> 1,220.69 5% remote-twitter summary android-7-1-armv8-api16 opt 825.11 -> 787.28 3% remote-nytimes summary android-7-1-armv8-api16 opt 1,184.83 -> 1,146.35 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9840
This bug is verified on Firefox 58.0a1 (20171005100211) under Windows 10 64-bit and Mac OS X 10.12.3.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Comment on attachment 8914928 [details] beta 57 uplift request must fix, Beta57+
Attachment #8914928 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This bug is verified on Firefox 57.0b7 (20171009192146) under Windows 10 64-bit and Mac OS X 10.12.3.
Depends on: 1409583
Depends on: 1464953
Performance Impact: --- → -
Whiteboard: [qf-]triaged → triaged
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: