Closed Bug 1358846 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

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

Details

(Whiteboard: triaged)

Attachments

(5 files)

Summary says it all.
Duplicate of this bug: 793143
Duplicate of this bug: 1075193
Duplicate of this bug: 1075190
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`?
Attachment #8860706 - Flags: review?(rhelmer)
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
Attachment #8860707 - Flags: review?(rhelmer) → 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 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 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 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+
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 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 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.
Whiteboard: triaged
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
Attachment #8860709 - Flags: review?(jcoppeard) → review+
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
Depends on: 1365134
No longer depends on: 1365134
Depends on: 1365256
Blocks: 1365640
Depends on: 1365706
Blocks: 1365717
Depends on: 1366656
Depends on: 1368232
Flags: needinfo?(kmaglione+bmo)
Duplicate of this bug: 1347618
Depends on: 1389160
Depends on: 1382281
You need to log in before you can comment on or make changes to this bug.