Summary says it all.

Sorry, the last part is a bit non-trivial. I couldn't think of a good way to split it up any more.

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review135504 ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:427 (Diff revision 1) > + PropertyIter iter(cx, obj); > + MOZ_RELEASE_ASSERT(mAddons.reserve(iter.Length())); > + > + for (auto addonObj : iter) { > + JS::RootedValue value(cx, addonObj.Value()); > + > + if (value.isObject()) { > + Addon addon(*this, mCx, addonObj.Name(), &value.toObject()); > + mAddons.infallibleAppend(Move(addon)); > + } > + } This is all a bit less than ideal. I'd really rather have another iterator subclass that creates Addon instances rather than pre-create a vector of them, but the rooting requirements make it tricky. Trying to add another level of subclassing to the property iterators gave me a headache, and the only other option I had was to just duplicate all of the iterator code.

Some additional notes: My plan for the addonsStartup.json file is to eventually add more information that we may need early in startup for lazy initialization of add-ons. For WebExtensions, for instance, we'll probably need to store a list of content script and webRequest URL match patterns, to make sure that extensions are initialized in time to modify those resources. I'd also like to move the metadata we store in stage directories into this file, so we can get rid of those directory scans entirely, or deal with anything that's there unexpectedly during the sideload scan after startup is complete. My plan for the native add-on startup service is to make it easy for us to move expensive operations that we need at early startup between JS and C++ as necessary. Most of the code there at this point just makes it easy to access add-on data in the format it's stored in on the JS side.

Comment on attachment 8860706 [details] Bug 1358846: Part 1 - Remove old database migration code. https://reviewboard.mozilla.org/r/132644/#review135856 ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js (Diff revision 1) > - * > - * @param aLiteral > - * The RDF object to convert > - * @return a string if the object could be converted or null > - */ > -function getRDFValue(aLiteral) { Isn't this used by `loadManifestFromRDF`?

Comment on attachment 8860706 [details] Bug 1358846: Part 1 - Remove old database migration code. https://reviewboard.mozilla.org/r/132644/#review135856 > Isn't this used by `loadManifestFromRDF`? Nope. We have another copy of it in XPIProvider.jsm for that :(

Comment on attachment 8860707 [details] Bug 1358846: Part 2 - Allow using file compression with JSONFile.jsm. https://reviewboard.mozilla.org/r/132646/#review135860

Comment on attachment 8860708 [details] Bug 1358846: Part 3 - Trivial cleanups. https://reviewboard.mozilla.org/r/132648/#review135862

Comment on attachment 8860706 [details] Bug 1358846: Part 1 - Remove old database migration code. https://reviewboard.mozilla.org/r/132644/#review135976

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review135970 lgtm overall - one issue I have a question about, and as discussed in IRC I'd like someone more familar with the mozilla platform C++ code to take a look at that bit. ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js:111 (Diff revision 2) > function* testNoIconsParsing(manifest) { > yield promiseWriteWebManifestForExtension(manifest, profileDir); > > - yield promiseRestartManager(); > - if (!manifest.theme) > - yield promiseAddonStartup(); > + yield Promise.all([ > + promiseRestartManager(), > + manifest.theme || promiseAddonStartup(), Hm. Isn't this going to fail if `manifest.theme` is defined?

Comment on attachment 8860751 [details] Bug 1358846: Part 5 - Clean up some path manipulation code. https://reviewboard.mozilla.org/r/132678/#review135982

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review135970 > Hm. Isn't this going to fail if `manifest.theme` is defined? Nope. Promise.all() immediately returns the value of mPromise. Since all we care about is that we only wait for promiseAddonStartup() for non-themes, that does exactly what we need.

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. Tom, would you mind taking a look at the JSAPI portions of this? Some of the iterator stuff is a little hairy, so if you have any ideas about how to handle it better, I'd be happy to hear them.

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review136192 ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:233 (Diff revision 2) > + } > + > + if (val.isNumber()) { > + return val.toNumber(); > + } > + if (val.isInt32()) { isNumber already means isDouble or isInt32.

Jon is going to pick up this review, because there are some problem with the Rooting/GC.

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review136200 This is mostly fine apart from the use of Heap<> and CustomAutoRooter. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:32 (Diff revision 2) > +public: > + typedef T SelfType; > + > + PropertyType begin() const > + { > + PropertyType elem(*static_cast<const SelfType*>(this)); suggestion: factor out |static_cast<const SelfType*>(this)| into a separate method here and in BaseIterElem. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:70 (Diff revision 2) > + return mIter.Length(); > + } > + > + JS::Value Value() > + { > + JS::RootedValue value(mIter.mCx, JS::UndefinedValue()); Maybe assert mIndex < Lenth() here too. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:128 (Diff revision 2) > + PropertyIter(JSContext* cx, JS::HandleObject object) > + : BaseIter(cx, object) > + , mIds(cx, JS::IdVector(cx)) > + { > + if (!JS_Enumerate(cx, object, &mIds)) { > + JS_ClearPendingException(cx); Is it OK to ignore failures here? I guess so as this happens in a bunch of places below. ::: toolkit/mozapps/extensions/AddonManagerStartup-inlines.h:233 (Diff revision 2) > + > +protected: > + bool > + GetValue(JS::MutableHandleValue value) > + { > + return JS_GetElement(mIter.mCx, mIter.mObject, mIndex, value); Can we assert the index is in range here? ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:193 (Diff revision 2) > + JS::TraceEdge(trc, &mObject, "addon object"); > + } > + > +protected: > + JSContext* mCx; > + JS::Heap<JSObject*> mObject; JS::Heap doesn't go in a stack class. Can you use Rooted<JSObject*> here instead? ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:275 (Diff revision 2) > + } > + return nullptr; > +} > + > +template<class T> > +class MOZ_RAII RootedWrapper final : public T, public JS::CustomAutoRooter Please don't use CustomAutoRooter. You should be able to use Rooted<JSObject*> in the base class to take care of rooting objects. ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:302 (Diff revision 2) > + if (!JS_SetProperty(mCx, obj, "changed", val)) { > + JS_ClearPendingException(mCx); > + } > + } > + > + JS::GCVector<Addon>& Addons() { return mAddons.get(); } Can you make this return a const reference? ::: toolkit/mozapps/extensions/AddonManagerStartup.cpp:318 (Diff revision 2) > +typedef RootedWrapper<InstallLocation> RootedInstallLocation; > + > + > +class MOZ_STACK_CLASS Addon : public WrapperBase { > +public: > + Addon(InstallLocation& location, JSContext* cx, const nsAString& id, JSObject* object) nit: in js/src the style is to always pass JSontext* as the first arugment but that may not apply here.

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review136200 > Is it OK to ignore failures here? I guess so as this happens in a bunch of places below. Yes, though we should probably log a warning. > JS::Heap doesn't go in a stack class. Can you use Rooted<JSObject*> here instead? I initially used JS::Rooted, but switched to JS::Heap and CustomAutoRooter because I wound up storing Addon objects in a vector, and I was worried that they'd wind up being deinitialized in the wrong order, which JS::Rooted doesn't like. I refactored this to work with an iterator rather than a vector to avoid that problem, so I switched this back to Rooted.

Comment on attachment 8860709 [details] Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. https://reviewboard.mozilla.org/r/132650/#review138422

https://hg.mozilla.org/integration/mozilla-inbound/rev/889c487a5d41926a36482270131cad200a1bfc30 Bug 1358846: Part 1 - Remove old database migration code. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2518673c56cf3b45e4d4a8f2959191290c00d8 Bug 1358846: Part 2 - Allow using file compression with JSONFile.jsm. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/b9088593e34ff5abe9bc64dcd81f2f36ca7770b6 Bug 1358846: Part 3 - Trivial cleanups. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/98b57ff82a5477e3f0c0f880c0a72cc115cdf9af Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. r=rhelmer,jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/26825f1e33dd2ef48bef737d97d9ba9af700417f Bug 1358846: Part 5 - Clean up some path manipulation code. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/7dcb80a051a3c6d70561b2965795cdc886b6028b Bug 1358846: Part 6 - Clean up error handling. r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/b533d7f9b9c2c574175cbdd6a965bd85133a25ef Bug 1358846: Disable Jetpack child_process test. r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/96ea13bb00c5a058a8d571e30794e8d0385182ee Bug 1358846: Temporarily disable low-value tests that fail only on Windows. r=me

backed out for talos error like https://treeherder.mozilla.org/logviewer.html#?job_id=98637822&repo=mozilla-inbound&lineNumber=1131

Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ebad7f51280d Backed out 15 changesets (bug 1358846, bug 1356826) for talos error. a=backout

https://hg.mozilla.org/integration/mozilla-inbound/rev/2369dacfa29f2f3338fd4ed78b81751ca8c72d78 Bug 1358846: Part 1 - Remove old database migration code. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0d3538c00490c322fc3b1fe6b84fa66d284a02 Bug 1358846: Part 2 - Allow using file compression with JSONFile.jsm. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1662f1893440f6ddb391480fd19e715e9fb727 Bug 1358846: Part 3 - Trivial cleanups. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/b21481adbdca5fe92b21639ef28a3e97be0f70bd Bug 1358846: Part 4 - Merge various startup information stores into a single JSON file. r=rhelmer,jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7f79a7dbfc4d5b34f8d3d7f70562d280b4cf97 Bug 1358846: Part 5 - Clean up some path manipulation code. r=rhelmer https://hg.mozilla.org/integration/mozilla-inbound/rev/2e75d007bdc2205cb810cc14344eadb5e737480a Bug 1358846: Part 6 - Clean up error handling. r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/b1fdb7b1d6a9df6db0c0c677c8924e9261c64091 Bug 1358846: Disable Jetpack child_process test. r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/243eb19366c6aeedcf8217def1726dc78821b3d5 Bug 1358846: Temporarily disable low-value tests that fail only on Windows. r=me

https://hg.mozilla.org/mozilla-central/rev/2369dacfa29f https://hg.mozilla.org/mozilla-central/rev/9c0d3538c004 https://hg.mozilla.org/mozilla-central/rev/bf1662f18934 https://hg.mozilla.org/mozilla-central/rev/b21481adbdca https://hg.mozilla.org/mozilla-central/rev/cf7f79a7dbfc https://hg.mozilla.org/mozilla-central/rev/2e75d007bdc2 https://hg.mozilla.org/mozilla-central/rev/b1fdb7b1d6a9 https://hg.mozilla.org/mozilla-central/rev/243eb19366c6