Closed
Bug 1268077
Opened 9 years ago
Closed 9 years ago
Expose addonListeners through mozAddonManager
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: aswan, Assigned: aswan)
References
Details
(Whiteboard: triaged)
Attachments
(2 files)
The only way to find out from the AddonManager if an install requires a restart is via arguments to onInstalling / onUninstalling in AddonListener. But mozAddonManager does expose AddonListeners. We'll need to expose them to eventually handle installing non-restartless add-ons.
Assignee | ||
Updated•9 years ago
|
Whiteboard: triaged
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → aswan
Assignee | ||
Updated•9 years ago
|
Summary: Handle installs that require restart in mozAddonManager → Expose addonListeners through mozAddonManager
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/55952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55952/
Assignee | ||
Comment 2•9 years ago
|
||
Eh, this builds for me locally but failed in try (https://treeherder.mozilla.org/logviewer.html#?job_id=21609385&repo=try)
e.g.:
15:57:57 INFO - /builds/slave/try-m64-0000000000000000000000/build/src/obj-firefox/x86_64/dist/include/mozilla/dom/AddonManagerBinding.h:472:8: error: 'EventListenerWasAdded' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
Flags: needinfo?(bugs)
Comment 3•9 years ago
|
||
oh, silly C++, or our silly requirements for override.
I guess b2g stuff wasn't compiled using that.
Comment 5•9 years ago
|
||
Link up of the e10s disco pane issue: https://github.com/mozilla/addons-frontend/issues/495
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55952/diff/1-2/
Attachment #8757515 -
Attachment description: MozReview Request: Bug 1268077 WIP: expose AddonListener through mozAddonManager → MozReview Request: Bug 1268077: expose AddonListener through mozAddonManager r?smaug
Attachment #8757515 -
Flags: review?(bugs)
Comment 7•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
https://reviewboard.mozilla.org/r/55952/#review53916
::: dom/webidl/AddonEvent.webidl:2
(Diff revision 2)
> +[Constructor(DOMString type, AddonEventInit eventInitDict)]
> +interface AddonEvent : Event {
This interface must have ChromeOnly or Pref=... or Func=... so that the interface name isn't exposed to the web.
Ah, the other interface has Func="mozilla::AddonManagerWebAPI::IsAPIEnabled"
so same should be used here, I think
::: toolkit/mozapps/extensions/addonManager.js:230
(Diff revision 2)
> case MSG_INSTALL_CLEANUP: {
> AddonManager.webAPI.clearInstalls(payload.ids);
> break;
> }
> +
> + case MSG_ADDON_EVENT_REQ: {
I think someone familiar with this code needs to review this part
::: toolkit/mozapps/extensions/amWebAPI.js:82
(Diff revision 2)
> this._promises.set(callbackID, { resolve, reject });
> Services.cpmm.sendAsyncMessage(MSG_PROMISE_REQUEST, { type, callbackID, args });
> });
> },
>
> + setAddonListener(callback) {
This looks like something which keeps 'this' alive forever. Nothing guarantees that setAddonListener(null) is ever called.
Leaks aren't good.
You may want to consider using addWeakMessageListener
::: toolkit/mozapps/extensions/test/browser/webapi_addon_listener.html:9
(Diff revision 2)
> +<body>
> +<p id="result"></p>
> +<script type="text/javascript">
> +let events = [];
> +let resultEl = document.getElementById("result");
> +[ "onEnabling",
event types don't start with "on" prefix.
Attachment #8757515 -
Flags: review?(bugs)
Assignee | ||
Comment 8•9 years ago
|
||
https://reviewboard.mozilla.org/r/55952/#review53916
> This looks like something which keeps 'this' alive forever. Nothing guarantees that setAddonListener(null) is ever called.
> Leaks aren't good.
>
> You may want to consider using addWeakMessageListener
Okay, so `eventListenerWasRemoved` doesn't get called for any active listeners when the page is unloaded?
I'll add it to the existing cleanup code.
> event types don't start with "on" prefix.
I know, we have a funny here between the existing standard and the AddonListener interface that's being exposed (https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/AddonListener) I figured it makes more sense to remain consistent with the existing names in AddonListener, is this just a question of taste or could it break something?
Assignee | ||
Comment 9•9 years ago
|
||
https://reviewboard.mozilla.org/r/55952/#review53916
> This interface must have ChromeOnly or Pref=... or Func=... so that the interface name isn't exposed to the web.
>
> Ah, the other interface has Func="mozilla::AddonManagerWebAPI::IsAPIEnabled"
> so same should be used here, I think
Adding the `Func=...` bit didn't work, as it ended up including AddonManagerWebAPI.h twice and it errored on the re-declaration of class AddonManagerWebAPI.
But in this case, I don't think we care about having the name AddonEvent available in content so ChromeOnly should be adequate.
Comment 10•9 years ago
|
||
including AddonManagerWebAPI.h twice shouldn't lead to any error. Is the .h missing the usual ifndef...
it is.
Comment 11•9 years ago
|
||
It should have something like
#ifndef addonmanagerwebapi_h_
#define addonmanagerwebapi_h_
....
#endif
Assignee | ||
Comment 12•9 years ago
|
||
Oh right, I just carelessly assumed that file was auto-generated.
Fixing...
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55952/diff/2-3/
Attachment #8757515 -
Attachment description: MozReview Request: Bug 1268077: expose AddonListener through mozAddonManager r?smaug → Bug 1268077: expose AddonListener through mozAddonManager
Attachment #8757515 -
Flags: review?(bugs)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Robert, can you take a look at the AddonManager bits (ie, the bits that aren't specifically about webidl)
Attachment #8757515 -
Flags: review?(rhelmer)
Comment 15•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
https://reviewboard.mozilla.org/r/55952/#review54032
::: dom/webidl/AddonManager.webidl:75
(Diff revision 3)
> */
> Promise<AddonInstall> createInstall(optional addonInstallOptions options);
> };
> +
> +// Mozilla Only
> +partial interface AddonManager {
well, isn't all of the API Mozilla only. So you could add the methods to AddonManager interface
::: toolkit/mozapps/extensions/amWebAPI.js:85
(Diff revision 3)
> },
>
> + setAddonListener(callback) {
> + this._eventListener = callback;
> + if (callback) {
> + Services.cpmm.addMessageListener(MSG_ADDON_EVENT, this);
This is still strong listener. What guarantees we don't leak everything?
::: toolkit/mozapps/extensions/test/browser/webapi_addon_listener.html:9
(Diff revision 3)
> +<body>
> +<p id="result"></p>
> +<script type="text/javascript">
> +let events = [];
> +let resultEl = document.getElementById("result");
> +[ "onEnabling",
So I still don't understand this list.
Event types shouldn't be onFoo, but just foo.
This test is just testing what is being dispatched, so I guess this.addonListener is doing wrong thing.
Attachment #8757515 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 16•9 years ago
|
||
https://reviewboard.mozilla.org/r/55952/#review54032
> This is still strong listener. What guarantees we don't leak everything?
If the listener unregisters itself, we call `setAddonListener(null)` which removes the listener.
If the page is unloaded while there is still an active listener, our own handler for unload calls `sendCleanup` which removes the listener.
> So I still don't understand this list.
> Event types shouldn't be onFoo, but just foo.
> This test is just testing what is being dispatched, so I guess this.addonListener is doing wrong thing.
Right but this bug is about exposing an existing API (https://developer.mozilla.org/en-US/Add-ons/Add-on_Manager/AddonListener) that already uses the onFoo naming scheme to (limited) content. The choice is to break the convention that event types don't start with "on" or have the names in content differ from names in the existing interface. Neither one is great of course but I chose the former.
Note that the users of this API are all within Mozilla and they are generally well acquainted with the existing AddonListener API so I think this makes sense in this case.
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55952/diff/3-4/
Attachment #8757515 -
Flags: review- → review?(bugs)
Comment 18•9 years ago
|
||
ah, I missed comment 8
Comment 19•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
https://reviewboard.mozilla.org/r/55952/#review54062
::: toolkit/mozapps/extensions/amWebAPI.js:93
(Diff revision 4)
> + Services.cpmm.removeMessageListener(MSG_ADDON_EVENT, this);
> + Services.cpmm.sendAsyncMessage(MSG_ADDON_EVENT_REQ, {enabled: false});
> + }
> + },
> +
> sendCleanup: function(ids) {
Since I don't know the setup here...
please verify this gets called when the browsing context is navigated or when the relevant tab or window is closed etc. to ensure we don't leak.
Attachment #8757515 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 20•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/57242/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57242/
Attachment #8759263 -
Flags: review?(rhelmer)
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55952/diff/4-5/
Comment 22•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
https://reviewboard.mozilla.org/r/55952/#review54112
::: toolkit/mozapps/extensions/amWebAPI.js:219
(Diff revision 5)
> this.allInstalls.push(installInfo.id);
> return install;
> }),
>
> + eventListenerWasAdded(type) {
> + if (this.listenerCount++ == 0) {
I think incrementing inside the `if` makes it harder to see than if it was on its own line, but I see we use this style elsewhere in extension manager code (and the style guide doesn't mention it) so nbd.
::: toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js:43
(Diff revision 5)
> +]);
> +
> +// Test disable of add-on requiring restart
> +add_task(function* test_disable() {
> + yield BrowserTestUtils.withNewTab(TESTPAGE, function*(browser) {
> + let [addon] = yield promiseAddonsByIDs([RESTART_ID]);
Why not `let addon = promiseAddonByID(RESTART_ID)` if there's only one? (same below)
Attachment #8757515 -
Flags: review?(rhelmer) → review+
Comment 23•9 years ago
|
||
Comment on attachment 8759263 [details]
Bug 1268077: Fix up MockInstall AddonListener events
https://reviewboard.mozilla.org/r/57242/#review54108
Attachment #8759263 -
Flags: review?(rhelmer) → review+
Assignee | ||
Comment 24•9 years ago
|
||
https://reviewboard.mozilla.org/r/55952/#review54112
> Why not `let addon = promiseAddonByID(RESTART_ID)` if there's only one? (same below)
Mostly laziness on my part, promiseAddonByID() is defined in the head.js in xpcshell but not the one here.
I can add it though...
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55952/diff/5-6/
Comment 26•9 years ago
|
||
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4fb284d4a7
Fix up MockInstall AddonListener events r=rhelmer
https://hg.mozilla.org/integration/mozilla-inbound/rev/291d7bedba4f
expose AddonListener through mozAddonManager r=rhelmer,smaug
Comment 27•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad4fb284d4a7
https://hg.mozilla.org/mozilla-central/rev/291d7bedba4f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Approval Request Comment
[Feature/regressing bug #]:
https://github.com/mozilla/addons-frontend/issues/467
[User impact if declined]:
This patch completes functionality needed by the new discovery pane (the page displayed when going to "about:addons" -> "Get Add-ons") which is slated to launch with the release of 48.
[Describe test coverage new/current, TreeHerder]:
Tests for the new functionality are included in the patch.
[Risks and why]:
Risks: minimal, the new functionality is only exposed to discovery.addons.mozilla.org (through a mechanism that is already present in 48). Why: this is needed to fully implement the discovery pane. Most of the functionality needed for the discovery pane landed before 48 went to aurora, the need for the functionality in this patch wasn't realized until later.
[String/UUID change made/needed]:
none
Attachment #8757515 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 29•9 years ago
|
||
Oops, meant to include in the uplift comment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=683cf961b432
Assignee | ||
Updated•9 years ago
|
status-firefox48:
--- → affected
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
Approval Request Comment
[Feature/regressing bug #]:
https://github.com/mozilla/addons-frontend/issues/467
[User impact if declined]:
This patch completes functionality needed by the new discovery pane (the page displayed when going to "about:addons" -> "Get Add-ons") which is slated to launch with the release of 48.
[Describe test coverage new/current, TreeHerder]:
Tests for the new functionality are included in the patch.
[Risks and why]:
Risks: minimal, the new functionality is only exposed to discovery.addons.mozilla.org (through a mechanism that is already present in 48). Why: this is needed to fully implement the discovery pane. Most of the functionality needed for the discovery pane landed before 48 went to aurora, the need for the functionality in this patch wasn't realized until later.
[String/UUID change made/needed]:
none
Attachment #8757515 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Hi guys, I reviewed this uplift request based on a ping from AndyM. It looks a bit big and I would prefer to let you make a decision here. Thanks!
Flags: needinfo?(sledru)
Flags: needinfo?(gchang)
Comment 32•8 years ago
|
||
Andrew, what is going to be the impact if we don't uplift this patch in 48?
Flags: needinfo?(sledru)
Flags: needinfo?(gchang)
Updated•8 years ago
|
Flags: needinfo?(aswan)
Comment 33•8 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> Andrew, what is going to be the impact if we don't uplift this patch in 48?
The impact will be the new discovery pane will not function.
Flags: needinfo?(aswan)
Comment 34•8 years ago
|
||
(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #33)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> > Andrew, what is going to be the impact if we don't uplift this patch in 48?
>
> The impact will be the new discovery pane will not function.
To expand on that further - it would mean we'd need to delay cutting over to it until 49.
Comment 35•8 years ago
|
||
and would that be an issue?
Comment 36•8 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #35)
> and would that be an issue?
Yes, it would affect discovery timelines and onboarding plans.
Assignee | ||
Comment 37•8 years ago
|
||
Also, re-iterating a point from comment 28 that may have been overlooked, the (non-test) code in this patch is only usable from addons.mozilla.org so unless there's some other subtle regression that has gone undetected by automated tests, there's no risk of this patch breaking anything existing.
Comment 38•8 years ago
|
||
Comment on attachment 8757515 [details]
Bug 1268077: expose AddonListener through mozAddonManager
OK, let's do it. Thanks for your explanations.
Should be in 48 beta 4
Attachment #8757515 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•8 years ago
|
Flags: qe-verify+
Comment 39•8 years ago
|
||
Comment 40•8 years ago
|
||
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1217414&repo=mozilla-beta
Flags: needinfo?(aswan)
Updated•8 years ago
|
Flags: needinfo?(amckay)
Assignee | ||
Comment 41•8 years ago
|
||
Whoops, we'll also need bug 1276424 for this to build, does that need separate approval?
Flags: needinfo?(aswan)
Comment 42•8 years ago
|
||
Yes, please
Comment 43•8 years ago
|
||
bugherder uplift |
Comment 45•8 years ago
|
||
Could you please provide some details on how to test this?
Flags: needinfo?(aswan)
Assignee | ||
Comment 46•8 years ago
|
||
(In reply to Paul Silaghi, QA [:pauly] from comment #45)
> Could you please provide some details on how to test this?
I guess it depends what you want to test, this is part of the new discovery pane project, details about how to test that end-to-end are here:
https://wiki.mozilla.org/Add-ons/Projects/DiscoveryImprovements#Testing
To do more targeted testing, I guess you could open the browser console while viewing about:addons or AMO and interact with navigator.mozAddonManager. You should be able to attach an AddonListener that does console.log() and then see log messages printed as you install/uninstall/enable/disable/etc addons.
Flags: needinfo?(aswan)
Comment 47•8 years ago
|
||
We tested the new discovery pane on Windows 7 x64, Windows 10 x86, Ubuntu LTS x64 and Mac OSX 10.9.5 using Firefox 48 Beta 7 (buildID: 20160711002726) and everything looks good. See here the testing details: https://public.etherpad-mozilla.org/p/discoverypane-exploratory
Updated•8 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•