Telemetry Experiments: remaining AddonManager integration cleanups

VERIFIED FIXED in Firefox 31

Status

defect
VERIFIED FIXED
5 years ago
9 months ago

People

(Reporter: gps, Assigned: gfritzsche)

Tracking

(Blocks 1 bug)

unspecified
Firefox 31
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(18 attachments, 3 obsolete attachments)

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
Reporter

Description

5 years ago
+++ 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.
Assignee

Updated

5 years ago
Attachment #8398207 - Flags: review?(georg.fritzsche)
Reporter

Comment 2

5 years ago
Attachment #8398606 - Flags: review?(georg.fritzsche)
Assignee

Comment 3

5 years ago
(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, ...).
Reporter

Comment 4

5 years ago
Attachment #8398726 - Flags: review?(georg.fritzsche)
Reporter

Comment 5

5 years ago
Need a Toolkit peer for this one.
Attachment #8398728 - Flags: review?(benjamin)
Reporter

Comment 6

5 years ago
Attachment #8398729 - Flags: review?(georg.fritzsche)
Reporter

Comment 7

5 years ago
Attachment #8398730 - Flags: review?(georg.fritzsche)
Assignee

Updated

5 years ago
Attachment #8398729 - Flags: review?(georg.fritzsche) → review+
Assignee

Updated

5 years ago
Attachment #8398730 - Flags: review?(georg.fritzsche) → review+

Updated

5 years ago
Attachment #8398728 - Flags: review?(benjamin) → review+
Assignee

Updated

5 years ago
Attachment #8398726 - Flags: review?(georg.fritzsche) → review+
Assignee

Comment 8

5 years ago
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)
Reporter

Comment 10

5 years ago
Comment on attachment 8398606 [details]
Part 2: Associate ExperimentEntry with Experiments instance

Patch not necessary.
Attachment #8398606 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Attachment #8398726 - Attachment description: Part 3: Don't use a global logger → Part 2: Don't use a global logger
Reporter

Updated

5 years ago
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
Reporter

Updated

5 years ago
Attachment #8398729 - Attachment description: Part 5: Use a prefixing logger in Experiments.jsm → Part 4: Use a prefixing logger in Experiments.jsm
Reporter

Updated

5 years ago
Attachment #8398730 - Attachment description: Part 6: Prefix logging with object identifier → Part 5: Prefix logging with object identifier
Reporter

Updated

5 years ago
Attachment #8398824 - Attachment description: Part 7: Don't allow unknown experiments to be installed → Part 6: Don't allow unknown experiments to be installed
Reporter

Comment 11

5 years ago
Attachment #8398981 - Flags: review?(georg.fritzsche)
Reporter

Comment 13

5 years ago
Not asking for review yet because tests are failing.
Assignee

Comment 14

5 years ago
Anything we could take a look at to help? Any try push links?
Reporter

Updated

5 years ago
Attachment #8398985 - Attachment description: Part 8: Activate disabled experiments when necessary → Part X: Activate disabled experiments when necessary
Reporter

Comment 15

5 years ago
Touches a Makefile.in, so bsmedberg gets review.
Attachment #8399128 - Flags: review?(benjamin)
Reporter

Comment 16

5 years ago
Attachment #8399129 - Flags: review?(bmcbride)
Reporter

Comment 17

5 years ago
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
Assignee

Comment 18

5 years ago
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+
Reporter

Comment 20

5 years ago
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+
Reporter

Updated

5 years ago
Attachment #8399129 - Attachment description: Part 9: Remove unncessary bits from install.rdf → Part 7: Remove unncessary bits from install.rdf
Reporter

Updated

5 years ago
Attachment #8398824 - Attachment description: Part 6: Don't allow unknown experiments to be installed → Part 8: Don't allow unknown experiments to be installed
Reporter

Updated

5 years ago
Attachment #8398981 - Attachment description: Part 7: More thorough testing of add-on enable state → Part 9: More thorough testing of add-on enable state
Reporter

Comment 22

5 years ago
Comment on attachment 8398985 [details]
Part 10: Activate disabled experiments when necessary

Merged into previous patch.
Attachment #8398985 - Attachment is obsolete: true
Reporter

Comment 23

5 years ago
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+
Blocks: clobber
Assignee

Comment 26

5 years ago
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+
Reporter

Comment 27

5 years ago
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+

Updated

5 years ago
Blocks: 988873
Reporter

Updated

5 years ago
Blocks: 992396
Reporter

Updated

5 years ago
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?
Assignee

Comment 32

5 years ago
(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.
Assignee

Comment 33

5 years ago
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
Assignee

Comment 34

5 years ago
Posted 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?
Assignee

Updated

5 years ago
Attachment #8402629 - Flags: review?(benjamin)
Assignee

Updated

5 years ago
Flags: needinfo?(gps)
Reporter

Updated

5 years ago
Attachment #8402629 - Attachment is obsolete: true
Flags: needinfo?(gps)
Reporter

Comment 37

5 years ago
(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.
Assignee

Comment 39

5 years ago
(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)
Reporter

Comment 42

5 years ago
(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)
Assignee

Comment 43

5 years ago
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.
Reporter

Comment 44

5 years ago
Attachment #8404370 - Flags: review?(georg.fritzsche)
Reporter

Comment 45

5 years ago
r+ granted by gritzsche on RB.
Attachment #8404376 - Flags: review+
Assignee

Comment 47

5 years ago
What is actually left to do here?
Can we possibly start to file new bugs for actionable items?
Depends on: 994686
Assignee

Updated

5 years ago
Attachment #8404370 - Flags: review?(georg.fritzsche)

Updated

5 years ago
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-]
Reporter

Comment 50

5 years ago
Attachment #8404964 - Flags: review?(georg.fritzsche)
Reporter

Comment 51

5 years ago
Attachment #8404965 - Flags: review?(georg.fritzsche)
Reporter

Comment 52

5 years ago
Attachment #8404967 - Flags: review?(georg.fritzsche)
Reporter

Comment 54

5 years ago
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+
Assignee

Updated

5 years ago
Attachment #8404965 - Flags: review?(georg.fritzsche) → review+
Assignee

Updated

5 years ago
Attachment #8404967 - Flags: review?(georg.fritzsche) → review+
Assignee

Updated

5 years ago
Attachment #8405094 - Flags: review?(georg.fritzsche) → review+
Assignee

Comment 56

5 years ago
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+
Assignee

Updated

5 years ago
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-]

Updated

5 years ago
Assignee: gps → georg.fritzsche
Assignee

Comment 57

5 years ago
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
Assignee

Comment 58

5 years ago
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+
Assignee

Comment 63

5 years ago
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.
Reporter

Updated

5 years ago
Duplicate of this bug: 997188
Status: RESOLVED → VERIFIED
Depends on: 1030093

Updated

9 months ago
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.