Merge extensions.ini, extensions.xpiState, extensions.bootstrappedAddons, and extensions.enabledAddons into a single JSON file

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
6 months ago
a month ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

Summary says it all.
Duplicate of this bug: 793143
Duplicate of this bug: 1075193
Duplicate of this bug: 1075190
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 13

6 months ago
mozreview-review
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`?
Attachment #8860706 - Flags: review?(rhelmer)
(Assignee)

Comment 14

6 months ago
mozreview-review-reply
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 15

6 months ago
mozreview-review
Comment on attachment 8860707 [details]
Bug 1358846: Part 2 - Allow using file compression with JSONFile.jsm.

https://reviewboard.mozilla.org/r/132646/#review135860
Attachment #8860707 - Flags: review?(rhelmer) → review+

Comment 16

6 months ago
mozreview-review
Comment on attachment 8860708 [details]
Bug 1358846: Part 3 - Trivial cleanups.

https://reviewboard.mozilla.org/r/132648/#review135862
Attachment #8860708 - Flags: review?(rhelmer) → review+

Comment 17

6 months ago
mozreview-review
Comment on attachment 8860706 [details]
Bug 1358846: Part 1 - Remove old database migration code.

https://reviewboard.mozilla.org/r/132644/#review135976
Attachment #8860706 - Flags: review+

Comment 18

6 months ago
mozreview-review
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?
Attachment #8860709 - Flags: review?(rhelmer) → review+

Comment 19

6 months ago
mozreview-review
Comment on attachment 8860751 [details]
Bug 1358846: Part 5 - Clean up some path manipulation code.

https://reviewboard.mozilla.org/r/132678/#review135982
Attachment #8860751 - Flags: review?(rhelmer) → review+
(Assignee)

Comment 20

6 months ago
mozreview-review-reply
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.
Attachment #8860709 - Flags: review?(evilpies)

Comment 22

6 months ago
mozreview-review
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.
Attachment #8860709 - Flags: review?(evilpies)
Jon is going to pick up this review, because there are some problem with the Rooting/GC.

Comment 24

6 months ago
mozreview-review
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.

Updated

6 months ago
Whiteboard: triaged
(Assignee)

Comment 25

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

6 months ago
mozreview-review
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
Attachment #8860709 - Flags: review?(jcoppeard) → review+
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

Comment 35

5 months ago
backed out for talos error like https://treeherder.mozilla.org/logviewer.html#?job_id=98637822&repo=mozilla-inbound&lineNumber=1131
Flags: needinfo?(kmaglione+bmo)

Comment 36

5 months ago
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
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1364878

Updated

5 months ago
Depends on: 1365134

Updated

5 months ago
No longer depends on: 1365134

Updated

5 months ago
Depends on: 1365256

Updated

5 months ago
Blocks: 1365640

Updated

5 months ago
Depends on: 1365706
Blocks: 1365717
No longer blocks: 1365640
Depends on: 1365640
Depends on: 1365568
Depends on: 1366656
Blocks: 1367823

Updated

5 months ago
Depends on: 1368232
Flags: needinfo?(kmaglione+bmo)

Updated

3 months ago
Duplicate of this bug: 1347618
Depends on: 1389160

Updated

a month ago
Depends on: 1382281
You need to log in before you can comment on or make changes to this bug.