+++ This bug was initially created as a clone of Bug #985084 +++ Opening a new bug to track remaining work on AddonManager/XPIProvider integration once all the other patches land. (I want to land the existing patches and close out the bugs rather than have bugs linger in a semi-opened state.) This is assigned to me and is my #1 priority until it lands.

(In reply to Gregory Szorc [:gps] from comment #2) > Created attachment 8398606 [details] > Part 2: Associate ExperimentEntry with Experiments instance Hm, sorry if that sounds too sceptical, but why do we need to make the relation-ship bi-directional? I'd much prefer to keep it unidirectional if possible (for testability, reasoning, ...).

Need a Toolkit peer for this one.

Comment on attachment 8398606 [details] Part 2: Associate ExperimentEntry with Experiments instance Cancelling this for now per the RB comment.

Comment on attachment 8398606 [details] Part 2: Associate ExperimentEntry with Experiments instance Patch not necessary.

Parts 1-5: https://hg.mozilla.org/integration/fx-team/rev/60423f4aeda2 https://hg.mozilla.org/integration/fx-team/rev/0857d467b4ed https://hg.mozilla.org/integration/fx-team/rev/5b408704c998 https://hg.mozilla.org/integration/fx-team/rev/c27967f904b6 https://hg.mozilla.org/integration/fx-team/rev/0efaafae5c60

Not asking for review yet because tests are failing.

Anything we could take a look at to help? Any try push links?

Touches a Makefile.in, so bsmedberg gets review.

Comment on attachment 8398985 [details] Part 10: Activate disabled experiments when necessary I could have kept X as the Roman numeral. Meh.

Comment on attachment 8398981 [details] Part 9: More thorough testing of add-on enable state r+ with nit per RB.

https://hg.mozilla.org/mozilla-central/rev/60423f4aeda2 https://hg.mozilla.org/mozilla-central/rev/0857d467b4ed https://hg.mozilla.org/mozilla-central/rev/5b408704c998 https://hg.mozilla.org/mozilla-central/rev/c27967f904b6 https://hg.mozilla.org/mozilla-central/rev/0efaafae5c60

Comment on attachment 8399128 [details] Part 6: Generate XPIs as part of the build bsmedberg granted review on RB. I moved this up in the series.

Parts 6 and 7 landed: https://hg.mozilla.org/integration/fx-team/rev/5ab138b718fe https://hg.mozilla.org/integration/fx-team/rev/b52300a56c21

Comment on attachment 8398985 [details] Part 10: Activate disabled experiments when necessary Merged into previous patch.

Comment on attachment 8398981 [details] Part 9: More thorough testing of add-on enable state Merged with subsequent part and updated. Needs re-review.

https://hg.mozilla.org/mozilla-central/rev/5ab138b718fe https://hg.mozilla.org/mozilla-central/rev/b52300a56c21

This needed a clobber: http://hg.mozilla.org/mozilla-central/rev/1417d180a1d8

Comment on attachment 8398981 [details] Part 9: More thorough testing of add-on enable state r+ pending one issue in RB.

What a weird clobber. rm -f '../../dist/bin/browser/modules/experiments/Experiments.jsm' /builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_virtualenv/bin/python -m mozbuild.action.preprocessor --depend .deps/Experiments.jsm.pp -DNO_NSPR_10_SUPPORT -DX_DISPLAY_MISSING='1' -DHAVE_X86_AVX2='1' -DHAVE_64BIT_OS='1' -DMOZ_ENABLE_PROFILER_SPS='1' -DMOZ_INSTRUMENTS='1' -DMOZ_PROFILING='1' -DMOZILLA_VERSION='"31.0a1"' -DMOZILLA_VERSION_U='31.0a1' -DMOZILLA_UAVERSION='"31.0"' -DXP_MACOSX='1' -DXP_DARWIN='1' -DD_INO='d_ino' -DMOZ_DEBUG_SYMBOLS='1' -DSTDC_HEADERS='1' -DHAVE_SSIZE_T='1' -DHAVE_ST_BLKSIZE='1' -DHAVE_SIGINFO_T='1' -DHAVE_VISIBILITY_HIDDEN_ATTRIBUTE='1' -DHAVE_VISIBILITY_ATTRIBUTE='1' -DHAVE_DIRENT_H='1' -DHAVE_GETOPT_H='1' -DHAVE_MEMORY_H='1' -DHAVE_UNISTD_H='1' -DHAVE_NL_TYPES_H='1' -DHAVE_X11_XKBLIB_H='1' -DHAVE_CPUID_H='1' -DHAVE_SYS_TYPES_H='1' -DHAVE_NETINET_IN_H='1' -DHAVE_SIN_LEN='1' -DHAVE_SCONN_LEN='1' -DHAVE_SIN6_LEN='1' -DHAVE_SA_LEN='1' -DINCLUDE_MOZILLA_DTRACE='1' -DHAVE_SYS_CDEFS_H='1' -DHAVE_DLADDR='1' -DNO_X11='1' -DHAVE_STAT64='1' -DHAVE_LSTAT64='1' -DHAVE_LOCALTIME_R='1' -DHAVE_LANGINFO_CODESET='1' -DVA_COPY='va_copy' -DHAVE_VA_COPY='1' -DHAVE_VA_LIST_AS_ARRAY='1' -DMALLOC_H='<malloc/malloc.h>' -DHAVE_POSIX_MEMALIGN='1' -DHAVE_VALLOC='1' -DHAVE_I18N_LC_MESSAGES='1' -DHAVE_LOCALECONV='1' -DNS_ATTR_MALLOC='__attribute__((malloc))' -DNS_WARN_UNUSED_RESULT='__attribute__((warn_unused_result))' -DNIGHTLY_BUILD='1' -DMOZ_UPDATE_CHANNEL='default' -DEARLY_BETA_OR_EARLIER='1' -DMOZ_PHOENIX='1' -DMOZ_BUILD_APP='browser' -DMOZ_WIDGET_COCOA='1' -DMOZ_INSTRUMENT_EVENT_LOOP='1' -DNS_PRINTING='1' -DNS_PRINT_PREVIEW='1' -DMOZ_DISTRIBUTION_ID='"org.mozilla"' -DIBMBIDI='1' -DACCESSIBILITY='1' -DMOZ_WEBRTC='1' -DMOZ_WEBRTC_ASSERT_ALWAYS='1' -DMOZ_WEBRTC_SIGNALING='1' -DMOZ_PEERCONNECTION='1' -DMOZ_SCTP='1' -DMOZ_SRTP='1' -DMOZ_SAMPLE_TYPE_FLOAT32='1' -DMOZ_WEBSPEECH='1' -DMOZ_RAW='1' -DATTRIBUTE_ALIGNED_MAX='64' -DMOZ_WEBM='1' -DMOZ_APPLEMEDIA='1' -DMOZ_MEDIA_NAVIGATOR='1' -DMOZ_VPX='1' -DMOZ_VPX_ERROR_CONCEALMENT='1' -DVPX_X86_ASM='1' -DMOZ_WAVE='1' -DMOZ_VORBIS='1' -DMOZ_OPUS='1' -DMOZ_WEBM_ENCODER='1' -DENABLE_SYSTEM_EXTENSION_DIRS='1' -DMOZ_WEBGL='1' -DMOZ_WEBGL_CONFORMANT='1' -DMOZ_GAMEPAD='1' -DMOZ_CRASHREPORTER='1' -DMOZ_CRASHREPORTER_ENABLE_PERCENT='100' -DLIBJPEG_TURBO_X64_ASM='1' -DMOZ_WEBAPP_RUNTIME='1' -DMOZ_SIGNING='1' -DMOZ_ENABLE_SIGNMAR='1' -DMOZ_UPDATER='1' -DGTEST_HAS_RTTI='0' -DMOZ_CONTENT_SANDBOX_REPORTER='1' -DMOZ_FEEDS='1' -DMOZ_SAFE_BROWSING='1' -DMOZ_URL_CLASSIFIER='1' -DGL_PROVIDER_='1' -DMOZ_LOGGING='1' -DMOZ_MEMORY='1' -DMOZ_MEMORY_DARWIN='1' -DJS_CRASH_DIAGNOSTICS='1' -DJSGC_INCREMENTAL='1' -DJSGC_USE_EXACT_ROOTING='1' -DJSGC_GENERATIONAL='1' -DMOZ_PAY='1' -DMOZ_ACTIVITIES='1' -DMOZ_SERVICES_FXACCOUNTS='1' -DHAVE___CXA_DEMANGLE='1' -DHAVE__UNWIND_BACKTRACE='1' -DJS_DEFAULT_JITREPORT_GRANULARITY='3' -DMOZ_OMNIJAR='1' -DMOZ_USER_DIR='"Mozilla"' -DMOZ_TREE_PIXMAN='1' -DHAVE_STDINT_H='1' -DHAVE_INTTYPES_H='1' -DMOZ_TREE_CAIRO='1' -DMOZ_ENABLE_SKIA='1' -DUSE_SKIA='1' -DUSE_SKIA_GPU='1' -DMOZ_XUL='1' -DMOZ_PROFILELOCKING='1' -DENABLE_MARIONETTE='1' -DBUILD_CTYPES='1' -DMOZ_PLACES='1' -DMOZ_SOCIAL='1' -DMOZ_SERVICES_COMMON='1' -DMOZ_SERVICES_CRYPTO='1' -DMOZ_SERVICES_HEALTHREPORT='1' -DMOZ_SERVICES_METRICS='1' -DMOZ_SERVICES_SYNC='1' -DMOZ_JSDOWNLOADS='1' -DMOZ_MACBUNDLE_ID='org.mozilla.nightly' -DMOZ_B2G_VERSION='"1.0.0"' -DMOZ_B2G_OS_NAME='""' -DMOZ_APP_UA_NAME='""' -DMOZ_APP_UA_VERSION='"31.0a1"' -DFIREFOX_VERSION='31.0a1' -DMOZ_TELEMETRY_DISPLAY_REV='2' -DMOZ_TELEMETRY_REPORTING='1' -DMOZ_TELEMETRY_ON_BY_DEFAULT='1' -DMOZ_DATA_REPORTING='1' -DMOZ_DLL_SUFFIX='".dylib"' -DXP_UNIX='1' -DA11Y_LOG='1' -DEXPOSE_INTL_API='1' -DENABLE_INTL_API='1' -DU_STATIC_IMPLEMENTATION='1' -DU_USING_ICU_NAMESPACE='0' -DMOZ_STATIC_JS='1' -DNDEBUG -DTRIMMED '/builds/slave/m-cen-osx64-000000000000000000/build/browser/experiments/Experiments.jsm' -o '../../dist/bin/browser/modules/experiments/Experiments.jsm' set -e; \ ../../dist/bin/nsinstall -D /builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell; \ for dir in /builds/slave/m-cen-osx64-000000000000000000/build/browser/experiments/test/addons/*; do \ base=`basename $dir`; \ (cd $dir && zip -qr /builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/$base.xpi *); \ done zip error: Zip file structure invalid (/builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/experiment-1.xpi) zip error: Zip file structure invalid (/builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/experiment-1a.xpi) zip error: Zip file structure invalid (/builds/slave/m-cen-osx64-000000000000000000/build/obj-firefox/x86_64/_tests/xpcshell/browser/experiments/test/xpcshell/experiment-2.xpi) make[6]: *** [libs] Error 3 make[5]: *** [browser/experiments/libs] Error 2 make[4]: *** [libs] Error 2 make[3]: *** [default] Error 2 make[2]: *** [realbuild] Error 2 make[1]: *** [realbuild] Error 2 make: *** [build] Error 2

https://hg.mozilla.org/integration/fx-team/rev/7e410c1d61e6 https://hg.mozilla.org/integration/fx-team/rev/d3053c4e4c51 https://hg.mozilla.org/integration/fx-team/rev/831a3ccce100 Parts 8, 9, 10. (I reordered some patches.)

Backed out 8/9/10 in https://hg.mozilla.org/integration/fx-team/rev/317a809acdc8 You did fine, xpcshell was green on your push and the next two. Then we closed, then there was a downtime this morning, a downtime where releng had been talking about creating a new image and recreating spot slave images to increase their size. Now, suddenly, you're frequently failing like https://tbpl.mozilla.org/php/getParsedLog.php?id=37325506&tree=Fx-Team. Is that just coincidence, or is something getting left behind by the test (or some other test) which makes it fail when run a second time on the same slave, and since they (might have) created an image from a slave which ran on one of those earlier pushes, now some large percentage of the slaves "have run it before" even though they themselves haven't?

(In reply to Gregory Szorc [:gps] from comment #29) > https://hg.mozilla.org/integration/fx-team/rev/7e410c1d61e6 > https://hg.mozilla.org/integration/fx-team/rev/d3053c4e4c51 > https://hg.mozilla.org/integration/fx-team/rev/831a3ccce100 > > Parts 8, 9, 10. (I reordered some patches.) What happened to the part 10 we discussed in RB? https://reviewboard.allizom.org/r/55/

(In reply to Phil Ringnalda (:philor) from comment #30) > Now, suddenly, you're frequently failing like > https://tbpl.mozilla.org/php/getParsedLog.php?id=37325506&tree=Fx-Team. Seeing the same thing locally after a clobber with current fxteam & the patches from comment 29 applied.

The interesting part: > 20:05:17 INFO - 1396753516349 addons.manager WARN InstallListener threw exception when calling onInstallStarted That might also have been what blocked me on bug 992258. Pushed fix to try: https://tbpl.mozilla.org/?tree=Try&rev=10fe591aaf97

Requesting sign-off for this simple fix so we can re-land the above if the try push is fine.

Comment on attachment 8402629 [details] [diff] [review] Part 11: Fix addon listener. I don't understand why this method returns a set at all. We only ever expect one experiment addon to be installed at a time, and that comes from _evaluateExperiments. Can we store the to-be-installed experiment in a similar way we store the to-be-uninstalled experiment (in this._pendingUninstall) and avoid the complexity with a map?

https://hg.mozilla.org/integration/fx-team/rev/0e9ff2ea5085 https://hg.mozilla.org/integration/fx-team/rev/7ade80b09c73 https://hg.mozilla.org/integration/fx-team/rev/d7cbb56f75e1 Pushed part 8 with minor fixup (essentially part 11 rolled in).

(In reply to Georg Fritzsche [:gfritzsche] from comment #31) > What happened to the part 10 we discussed in RB? > https://reviewboard.allizom.org/r/55/ It has been bumped to part 11. Or 12. I can't remember any more. Personally, I find maintaining part numbers to be silly. But, our current review tooling dictates it because it otherwise makes it impossible to sort out patch order.

(In reply to Gregory Szorc [:gps] from comment #36) > https://hg.mozilla.org/integration/fx-team/rev/0e9ff2ea5085 > https://hg.mozilla.org/integration/fx-team/rev/7ade80b09c73 > https://hg.mozilla.org/integration/fx-team/rev/d7cbb56f75e1 > > Pushed part 8 with minor fixup (essentially part 11 rolled in). What about comment 11?

(In reply to Georg Fritzsche [:gfritzsche] from comment #38) > What about comment 11? Hm, i meant comment 35

https://hg.mozilla.org/mozilla-central/rev/0e9ff2ea5085 https://hg.mozilla.org/mozilla-central/rev/7ade80b09c73 https://hg.mozilla.org/mozilla-central/rev/d7cbb56f75e1

Hi Greg, can you provide a point value for this bug.

(In reply to Georg Fritzsche [:gfritzsche] from comment #39) > (In reply to Georg Fritzsche [:gfritzsche] from comment #38) > > What about comment 11? > > Hm, i meant comment 35 I ignored comment 35 because that would invalidate the r+. I don't think Set vs scalar makes much of a difference. If nothing else, I feel Set is more robust because it isn't susceptible to funky JavaScript equivalence issues. YAGNI applies, but I bet we'll someday have multiple experiments active at the same time.

Hm, it's just string comparison and the possibility of changes later doesn't seem like a great argument here. The whole feature is designed for "only one experiment active", so we shouldn't worry about multiple experiments in single places yet. But i guess this can be dealt with later.

r+ granted by gritzsche on RB.

What is actually left to do here? Can we possibly start to file new bugs for actionable items?

Marking as [qa-] for now as it doesn't appear this needs QA attention. Kamil, please mark as [qa+] and test this if you find cause to do so.

You won't believe how long it took me to make this patch work. I hate browser chrome testing so much.

Anthony, Sounds good. I asked IRC channel if there was anything that could be tested here, most of this will be covered via automation. Once everything lands in this ticket, I'll go through this at a higher level and create any issues that are found.

Comment on attachment 8404964 [details] Part 13: Ability to ignore hash verification r+, with ideally changed to using a pref.

This landed up to part 11. I got part 12 to part 17 fixed up and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=492051acc4af

Fixes issues i stumbled upon via rebase issues today: * we shouldn't throw errors as part of a normal uninit() * we should probably not skip pending cache saves on shutdown https://tbpl.mozilla.org/?tree=Try&rev=206c7e911fa4

Comment on attachment 8407607 [details] [diff] [review] Part 18: Fix experiments uninit behavior. I'm fine with all of this except for the changes to _main. In _main, the finally clause could eat real exceptions (if _saveToCache threw an error). I'm also skeptical that in general we should be writing the cache if there was an error here. Does this patch without the _main changes solve the test errors? r=me without that if so, or we should talk about changing the finally clause here to `catch (e if e instanceof AlreadyShutdownError)`

I re-ordered some patches and landed some. https://hg.mozilla.org/integration/fx-team/rev/1ab6bbd62c76 https://hg.mozilla.org/integration/fx-team/rev/5c3a195a107e https://hg.mozilla.org/integration/fx-team/rev/73da7e611e92 https://hg.mozilla.org/integration/fx-team/rev/2c1a0b0551fb

https://hg.mozilla.org/mozilla-central/rev/1ab6bbd62c76 https://hg.mozilla.org/mozilla-central/rev/5c3a195a107e https://hg.mozilla.org/mozilla-central/rev/73da7e611e92 https://hg.mozilla.org/mozilla-central/rev/2c1a0b0551fb

* backed out the conflicting landings from comment 60 in favor of a known-good patch-stack that includes all the remaining patches here * dropped the _main changes in part 18 that were discussed in comment 59 * should be done here now! https://hg.mozilla.org/integration/fx-team/rev/b960c2026325 https://hg.mozilla.org/integration/fx-team/rev/cb9a5809f673 https://hg.mozilla.org/integration/fx-team/rev/9d5d5da02bf2 https://hg.mozilla.org/integration/fx-team/rev/27e2a7c2086e https://hg.mozilla.org/integration/fx-team/rev/1648035f5d7b https://hg.mozilla.org/integration/fx-team/rev/71fe0bf5ef84 https://hg.mozilla.org/integration/fx-team/rev/57bc0a610103 https://hg.mozilla.org/integration/fx-team/rev/36bc9e63aba7

Hm, and now i see that the imported patches didn't have the proper author information :( I'm not keen on going back for this, let me know if i should.

https://hg.mozilla.org/mozilla-central/rev/b960c2026325 https://hg.mozilla.org/mozilla-central/rev/cb9a5809f673 https://hg.mozilla.org/mozilla-central/rev/9d5d5da02bf2 https://hg.mozilla.org/mozilla-central/rev/27e2a7c2086e https://hg.mozilla.org/mozilla-central/rev/1648035f5d7b https://hg.mozilla.org/mozilla-central/rev/71fe0bf5ef84 https://hg.mozilla.org/mozilla-central/rev/57bc0a610103 https://hg.mozilla.org/mozilla-central/rev/36bc9e63aba7