Closed Bug 1268077 Opened 9 years ago Closed 9 years ago

Expose addonListeners through mozAddonManager

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox48 --- verified
firefox49 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: triaged)

Attachments

(2 files)

Bug 1268077: expose AddonListener through mozAddonManager
58 bytes, text/x-review-board-request
smaug
: review+
rhelmer
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1268077: Fix up MockInstall AddonListener events
58 bytes, text/x-review-board-request
rhelmer
: review+
Details
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.
Whiteboard: triaged
Assignee: nobody → aswan
Summary: Handle installs that require restart in mozAddonManager → Expose addonListeners through mozAddonManager
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)
oh, silly C++, or our silly requirements for override. I guess b2g stuff wasn't compiled using that.
bug 1276424 should fix the issue.
Flags: needinfo?(bugs)
Link up of the e10s disco pane issue: https://github.com/mozilla/addons-frontend/issues/495
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 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)
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?
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.
including AddonManagerWebAPI.h twice shouldn't lead to any error. Is the .h missing the usual ifndef... it is.
It should have something like #ifndef addonmanagerwebapi_h_ #define addonmanagerwebapi_h_ .... #endif
Oh right, I just carelessly assumed that file was auto-generated. Fixing...
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)
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 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-
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.
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 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+
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 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 on attachment 8759263 [details] Bug 1268077: Fix up MockInstall AddonListener events https://reviewboard.mozilla.org/r/57242/#review54108
Attachment #8759263 - Flags: review?(rhelmer) → review+
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...
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/
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
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
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?
Oops, meant to include in the uplift comment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=683cf961b432
status-firefox48: --- → affected
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)
Andrew, what is going to be the impact if we don't uplift this patch in 48?
Flags: needinfo?(sledru)
Flags: needinfo?(gchang)
Flags: needinfo?(aswan)
(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)
(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.
and would that be an issue?
(In reply to Sylvestre Ledru [:sylvestre] from comment #35) > and would that be an issue? Yes, it would affect discovery timelines and onboarding plans.
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 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+
Flags: qe-verify+
Flags: needinfo?(amckay)
Whoops, we'll also need bug 1276424 for this to build, does that need separate approval?
Flags: needinfo?(aswan)
Yes, please
...aswan is on it
Flags: needinfo?(amckay)
Could you please provide some details on how to test this?
Flags: needinfo?(aswan)
(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)
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
status-firefox48: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: