Closed Bug 989137 Opened 7 years ago Closed 7 years ago

Telemetry Experiments: remaining AddonManager integration cleanups

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 31

People

(Reporter: gps, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

(Whiteboard: p=8 s=it-31c-30a-29b.3 [qa-])

Attachments

(18 files, 3 obsolete files)

37 bytes, text/x-review-board-request
Unfocused
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Details
37 bytes, text/x-review-board-request
benjamin
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Details
37 bytes, text/x-review-board-request
Unfocused
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Unfocused
: review-
Details
37 bytes, text/x-review-board-request
gps
: review+
Details
37 bytes, text/x-review-board-request
Unfocused
: review+
Details
37 bytes, text/x-review-board-request
Unfocused
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Details
37 bytes, text/x-review-board-request
gps
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Details
37 bytes, text/x-review-board-request
gfritzsche
: review+
Details
37 bytes, text/x-review-board-request
Unfocused
: review+
Details
37 bytes, text/x-review-board-request
Unfocused
: review+
gfritzsche
: review+
Details
4.21 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
+++ 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.
Attachment #8398207 - Flags: review?(georg.fritzsche)
Attachment #8398606 - Flags: review?(georg.fritzsche)
(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, ...).
Attachment #8398726 - Flags: review?(georg.fritzsche)
Need a Toolkit peer for this one.
Attachment #8398728 - Flags: review?(benjamin)
Attachment #8398729 - Flags: review?(georg.fritzsche)
Attachment #8398730 - Flags: review?(georg.fritzsche)
Attachment #8398729 - Flags: review?(georg.fritzsche) → review+
Attachment #8398730 - Flags: review?(georg.fritzsche) → review+
Attachment #8398728 - Flags: review?(benjamin) → review+
Attachment #8398726 - Flags: review?(georg.fritzsche) → review+
Comment on attachment 8398606 [details]
Part 2: Associate ExperimentEntry with Experiments instance

Cancelling this for now per the RB comment.
Attachment #8398606 - Flags: review?(georg.fritzsche)
Comment on attachment 8398606 [details]
Part 2: Associate ExperimentEntry with Experiments instance

Patch not necessary.
Attachment #8398606 - Attachment is obsolete: true
Attachment #8398726 - Attachment description: Part 3: Don't use a global logger → Part 2: Don't use a global logger
Attachment #8398728 - Attachment description: Part 4: Add a Log.jsm API for doing prefix logging → Part 3: Add a Log.jsm API for doing prefix logging
Attachment #8398729 - Attachment description: Part 5: Use a prefixing logger in Experiments.jsm → Part 4: Use a prefixing logger in Experiments.jsm
Attachment #8398730 - Attachment description: Part 6: Prefix logging with object identifier → Part 5: Prefix logging with object identifier
Attachment #8398824 - Attachment description: Part 7: Don't allow unknown experiments to be installed → Part 6: Don't allow unknown experiments to be installed
Attachment #8398981 - Flags: review?(georg.fritzsche)
Not asking for review yet because tests are failing.
Anything we could take a look at to help? Any try push links?
Attachment #8398985 - Attachment description: Part 8: Activate disabled experiments when necessary → Part X: Activate disabled experiments when necessary
Touches a Makefile.in, so bsmedberg gets review.
Attachment #8399128 - Flags: review?(benjamin)
Attachment #8399129 - Flags: review?(bmcbride)
Comment on attachment 8398985 [details]
Part 10: Activate disabled experiments when necessary

I could have kept X as the Roman numeral. Meh.
Attachment #8398985 - Attachment description: Part X: Activate disabled experiments when necessary → Part 10: Activate disabled experiments when necessary
Comment on attachment 8398981 [details]
Part 9: More thorough testing of add-on enable state

r+ with nit per RB.
Attachment #8398981 - Flags: review?(georg.fritzsche) → review+
Attachment #8398824 - Flags: review?(bmcbride) → review+
Attachment #8399129 - Flags: review?(bmcbride) → review+
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.
Attachment #8399128 - Attachment description: Part 8: Generate XPIs as part of the build → Part 6: Generate XPIs as part of the build
Attachment #8399128 - Flags: review?(benjamin) → review+
Attachment #8399129 - Attachment description: Part 9: Remove unncessary bits from install.rdf → Part 7: Remove unncessary bits from install.rdf
Attachment #8398824 - Attachment description: Part 6: Don't allow unknown experiments to be installed → Part 8: Don't allow unknown experiments to be installed
Attachment #8398981 - Attachment description: Part 7: More thorough testing of add-on enable state → Part 9: More thorough testing of add-on enable state
Comment on attachment 8398985 [details]
Part 10: Activate disabled experiments when necessary

Merged into previous patch.
Attachment #8398985 - Attachment is obsolete: true
Comment on attachment 8398981 [details]
Part 9: More thorough testing of add-on enable state

Merged with subsequent part and updated. Needs re-review.
Attachment #8398981 - Flags: review?(georg.fritzsche)
Attachment #8398981 - Flags: review?(bmcbride)
Attachment #8398981 - Flags: review+
Comment on attachment 8398981 [details]
Part 9: More thorough testing of add-on enable state

r+ pending one issue in RB.
Attachment #8398981 - Flags: review?(georg.fritzsche) → review+
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
Attachment #8398981 - Flags: review?(bmcbride) → review-
Attachment #8400982 - Flags: review?(bmcbride) → review+
Blocks: 988873
Blocks: 992396
Attachment #8400982 - Attachment description: Part 11: Establish AddonManagerTesting.jsm to hold common Addon Manager routines → Part 10: Establish AddonManagerTesting.jsm to hold common Addon Manager routines
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 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
Attached patch Part 11: Fix addon listener. (obsolete) — Splinter Review
Requesting sign-off for this simple fix so we can re-land the above if the try push is fine.
Attachment #8402629 - Flags: review?(benjamin)
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?
Attachment #8402629 - Flags: review?(benjamin)
Flags: needinfo?(gps)
Attachment #8402629 - Attachment is obsolete: true
Flags: needinfo?(gps)
(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 Georg Fritzsche [:gfritzsche] from comment #38)
> What about comment 11?

Hm, i meant comment 35
Whiteboard: [leave open] → [leave open] p=0 s=it-31c-30a-29b.2
Hi Greg, can you provide a point value for this bug.
Flags: needinfo?(gps)
(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.
Flags: needinfo?(gps)
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.
Attachment #8404370 - Flags: review?(georg.fritzsche)
r+ granted by gritzsche on RB.
Attachment #8404376 - Flags: review+
What is actually left to do here?
Can we possibly start to file new bugs for actionable items?
Depends on: 994686
Attachment #8404370 - Flags: review?(georg.fritzsche)
Whiteboard: [leave open] p=0 s=it-31c-30a-29b.2 → [leave open] p=8 s=it-31c-30a-29b.2
Whiteboard: [leave open] p=8 s=it-31c-30a-29b.2 → [leave open] p=8 s=it-31c-30a-29b.2 [qa?]
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.
QA Contact: kamiljoz
Whiteboard: [leave open] p=8 s=it-31c-30a-29b.2 [qa?] → [leave open] p=8 s=it-31c-30a-29b.2 [qa-]
Attachment #8404964 - Flags: review?(georg.fritzsche)
Attachment #8404965 - Flags: review?(georg.fritzsche)
Attachment #8404967 - Flags: review?(georg.fritzsche)
Attachment #8405014 - Flags: review?(bmcbride)
You won't believe how long it took me to make this patch work. I hate browser chrome testing so much.
Attachment #8405094 - Flags: review?(georg.fritzsche)
Attachment #8405094 - Flags: review?(bmcbride)
Flags: firefox-backlog+
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.
Attachment #8405014 - Flags: review?(bmcbride) → review+
Attachment #8405094 - Flags: review?(bmcbride) → review+
Attachment #8404965 - Flags: review?(georg.fritzsche) → review+
Attachment #8404967 - Flags: review?(georg.fritzsche) → review+
Attachment #8405094 - Flags: review?(georg.fritzsche) → review+
Comment on attachment 8404964 [details]
Part 13: Ability to ignore hash verification

r+, with ideally changed to using a pref.
Attachment #8404964 - Flags: review?(georg.fritzsche) → review+
Attachment #8404370 - Flags: review+
Whiteboard: [leave open] p=8 s=it-31c-30a-29b.2 [qa-] → [leave open] p=8 s=it-31c-30a-29b.3 [qa-]
Assignee: gps → georg.fritzsche
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
Attachment #8407607 - Flags: review?(benjamin)
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)`
Attachment #8407607 - Flags: review?(benjamin) → review+
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.
Duplicate of this bug: 997188
Status: RESOLVED → VERIFIED
Depends on: 1030093
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.