1250469 - [e10s] add-on SDK registers several observers for each tab

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P3)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1195386 +++

(In reply to The 8472 from bug 1195386 comment #3)
> real profile with ~1500 tabs and several addons:
> 
> 
> 28,001 (100.0%) -- observer-service-suspect
> ├───6,135 (21.91%) ── referent(topic=sdk:loader:destroy)
> ├───1,716 (06.13%) ── referent(topic=memory-pressure)
> ├───1,661 (05.93%) ── referent(topic=service-worker-get-client)
> ├───1,550 (05.54%) ── referent(topic=browser:purge-session-history)
> ├───1,527 (05.45%) ── referent(topic=app-theme-changed)
> ├───1,526 (05.45%) ── referent(topic=network:offline-status-changed)
> ├───1,521 (05.43%) ── referent(topic=dom-storage2-changed)
> ├───1,514 (05.41%) ── referent(topic=earlyformsubmit)
> ├───1,513 (05.40%) ── referent(topic=audio-playback)
> ├───1,511 (05.40%) ── referent(topic=browser:purge-domain-data)
> ├───1,510 (05.39%) ── referent(topic=dom-window-destroyed)
> ├───1,509 (05.39%) ── referent(topic=invalidformsubmit)
> ├───1,508 (05.39%) ── referent(topic=author-style-disabled-changed)
> ├───1,508 (05.39%) ── referent(topic=keyword-uri-fixup)
> ├───1,508 (05.39%) ── referent(topic=style-sheet-applicable-state-changed)
> └─────284 (01.01%) ── referent(topic=xpcom-shutdown)

The SDK shouldn't register a new observer (from a separate framescript) for every add-on for every tab (which is what it looks like happens right now). It should use 1 observer for the loader destroy that does the Right Thing depending on the observer notification that is being sent. Any data necessary for the notification to be handled (ie the relevant tab + add-on) could be on an nsISupports object passed as the subject.


Andy, can this be taken up by some of the folks working on add-ons these days? I'm happy to help with reviews but don't have cycles to write the patch myself.

Comment 1

[Andy McKay]

301 redirect over to Luca, would you have time to look at this next time you look at SDK stuff?

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Andy McKay [:andym] from comment #1)
> 301 redirect over to Luca, would you have time to look at this next time you
> look at SDK stuff?

sure, redirect received.

I'm going to look into it asap (I'm leaving the needinfo assigned to me as a reminder)

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

After taking a deeper look at how the Addon-SDK abstractions are applied to e10s cross-process scenarios, I've got the following elements:

- the Add-on SDK mirrors its module loading system in the Content process, and it seems to work with the same principles of the loader which is used in the Main Process: every Add-on SDK module is loaded into sandboxes which are created for the add-ons, then every addon has clearly its own copy of the modules

- the low level module "sdk/content/tab-events" [1] runs into the content process, it is loaded by the frame scripts injected in the content process by the Add-on SDK and like described above, is going to run in a sandbox created specifically for the add-on which is using it (indirectly through the higher level APIs) and 

- the low level module "sdk/content/tab-events" subscribes 6 observer events to monitor 3 phases of the tabs life cycle (create, load and ready),
  
From the mechanisms described in the above list, it seems that the issues can be related to the fact that every Add-on SDK addon has its own copy of the Add-on SDK modules which it is using, and every add-ons is subscribing its own observer service listeners to monitor the tab life cycle.

If it is, one strategy can be to reduce the number of observer service listeners needed to monitor the tabs lifecycle (especially because the "sdk/tabs" modules is probably used by a big part of the Addon SDK add-ons) by not subscribing directly the needed observer events from the SDK module running in the content process, but by adding a listener throught a JSM module which will subscribe the observer events once for all the Add-on SDK addons (and routes them to all the add-ons that needs to subscribe it).

I'm adding a needinfo assigned to Dave to ensure that I'm not missing something and that the above analysis of the Add-on SDK mechanisms described above is correct

[1]: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/tab-events.js

Comment 4

[:Gijs (he/him)]

(In reply to Luca Greco [:rpl] from comment #3)
> After taking a deeper look at how the Addon-SDK abstractions are applied to
> e10s cross-process scenarios, I've got the following elements:
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/
> content/tab-events.js

While the analysis in this comment sounds correct to me first-hand (NB: I am not an addon-sdk expert), the actual issue noted in comment #0 is not with any of the tab-events.js observers that are registered, but with the sdk:loader:destroy observer: https://dxr.mozilla.org/mozilla-central/search?q=sdk%3Aloader%3Adestroy&=mozilla-central&redirect=true

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Luca Greco [:rpl] from comment #3)
> > After taking a deeper look at how the Addon-SDK abstractions are applied to
> > e10s cross-process scenarios, I've got the following elements:
> > [1]:
> > https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/
> > content/tab-events.js
> 
> While the analysis in this comment sounds correct to me first-hand (NB: I am
> not an addon-sdk expert), the actual issue noted in comment #0 is not with
> any of the tab-events.js observers that are registered, but with the
> sdk:loader:destroy observer:
> https://dxr.mozilla.org/mozilla-central/
> search?q=sdk%3Aloader%3Adestroy&=mozilla-central&redirect=true

Thanks for immediately pointing it out it, then that part of the analysis is definitely wrong, let's try to re-apply part of the above analysis on the "sdk:loader:destroy" scenario:

- every add-on has its own instances of the Addon SDK modules

- every add-on manages its own "the loader is going to be destroyed" notification by subscribing/notify its own listeners to the custom Observer event topic "sdk:loader:destroy"

if the above analysis about the module loading mechanisms is right, then every add-on directly or indirectly uses most of the modules found in the above dxr search and so every add-on contributes to increase the number of listeners subscribed to the "sdk:loader:destroy" topic.

Comment 6

[:Gijs (he/him)]

(In reply to Luca Greco [:rpl] from comment #5)
> (In reply to :Gijs Kruitbosch from comment #4)
> > (In reply to Luca Greco [:rpl] from comment #3)
> > > After taking a deeper look at how the Addon-SDK abstractions are applied to
> > > e10s cross-process scenarios, I've got the following elements:
> > > [1]:
> > > https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/
> > > content/tab-events.js
> > 
> > While the analysis in this comment sounds correct to me first-hand (NB: I am
> > not an addon-sdk expert), the actual issue noted in comment #0 is not with
> > any of the tab-events.js observers that are registered, but with the
> > sdk:loader:destroy observer:
> > https://dxr.mozilla.org/mozilla-central/
> > search?q=sdk%3Aloader%3Adestroy&=mozilla-central&redirect=true
> 
> Thanks for immediately pointing it out it, then that part of the analysis is
> definitely wrong, let's try to re-apply part of the above analysis on the
> "sdk:loader:destroy" scenario:
> 
> - every add-on has its own instances of the Addon SDK modules
> 
> - every add-on manages its own "the loader is going to be destroyed"
> notification by subscribing/notify its own listeners to the custom Observer
> event topic "sdk:loader:destroy"
> 
> if the above analysis about the module loading mechanisms is right, then
> every add-on directly or indirectly uses most of the modules found in the
> above dxr search and so every add-on contributes to increase the number of
> listeners subscribed to the "sdk:loader:destroy" topic.

Yes, but it seems that the number of listeners there corresponds to roughly # of addons * # of tabs, apparently because of process scripts (maybe page scripts or whatever the add-on sdk calls those?)

Especially that last part is really bad. It also seems like this notification is dispatched by the SDK itself, and every notification will only apply to 1 out of the ~6000 listeners, which is really inefficient. :-)

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to :Gijs Kruitbosch from comment #6)
> Yes, but it seems that the number of listeners there corresponds to roughly
> # of addons * # of tabs, apparently because of process scripts (maybe page
> scripts or whatever the add-on sdk calls those?)

I'm going to run some experiment locally to get a better picture of exactly how many loaders 
and how many Observer service subscribers per add-on we are currently creating through the Add-on SDK
(e.g. from what I saw I expect one loader per process per add-on)
 
> Especially that last part is really bad. It also seems like this
> notification is dispatched by the SDK itself, and every notification will
> only apply to 1 out of the ~6000 listeners, which is really inefficient. :-)

I totally agree.

Comment 8

[Dave Townsend [:mossop]]

Your problem is probably that every single object in the SDK that extends Disposable adds an observer listener for the unload topic: https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/disposable.js#51.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1250469 - Reduce number of ObserverService observers subscribed by the SDK Disposable class.

Review commit: https://reviewboard.mozilla.org/r/37365/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37365/

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8725210 [details]
MozReview Request: Bug 1250469 - Reduce number of ObserverService observers subscribed by the SDK Disposable class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37365/diff/1-2/

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8725210 [details]
MozReview Request: Bug 1250469 - Reduce number of ObserverService observers subscribed by the SDK Disposable class.

(In reply to Dave Townsend [:mossop] from comment #8)
> Your problem is probably that every single object in the SDK that extends
> Disposable adds an observer listener for the unload topic:
> https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/
> disposable.js#51.

Thanks Dave, this confirm what I suspected, but I wanted to run a test to be sure that I wasn't been fooled by any of the used abstractions.

I've just pushed a small patch sketch for an initial feedback:

The patch removes any direct ObserverService usage from the disposable module and switch it to:
- register one ObserverService listener (using the Addon-SDK "sdk/system/events" module)
- keep the disposable's unloaders into a Map
- once the sdk:loader:destroy system event is received, run all the registered unloaders 

I'm trying to maintain the existent flow intact as much as possible and to apply a minimum amount of changes. 

As a first test, I'm running the existent test-disposable.js test cases to check that they all pass as expected, here is a push to try of the attached patch:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=c769abaa8f40

To produce a rough estimate of the changes in terms of 'number of registered "sdk:loader:destroy" observers', I've run the test-disposable.js addon-sdk test case with the NSPR_LOG_MODULES environment variable set to "ObserverService:5"  with both the original 'disposable' module and the one with this patch applied and finally look at the number of "sdk:loader:destroy" observers registered by filtering the produced logs (e.g. "cat nspr.log | grep sdk:loader:destroy | grep AddObserver | wc -l"):

- original "disposable" module: 32
- patched "disposable" module: 21

Comment 12

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/37365/#review35585

This breaks the automatic handling of disposables that support weak references. I think you'd be able to get around that with using Cu.getWeakReference on the disposable in that case and stashing that in a map where the unloader can get it rather than relying on the closure as it currently does.

::: addon-sdk/source/lib/sdk/core/disposable.js:61
(Diff revision 2)
> +    if (subject.wrappedJSObject == unloadSubject) {

You should do this in the listener above so it only happens once.

::: addon-sdk/source/lib/sdk/core/disposable.js:62
(Diff revision 2)
> +      addonUnloadListeners.delete(disposable, unloader);

If we're unloading there is no need to clear the Map one by one, just clear it at the end of the listener above.

::: addon-sdk/source/lib/sdk/core/disposable.js:108
(Diff revision 2)
> -observe.define(Disposable, (disposable, subject, topic, data) => {
> +onAddonUnload.define(Disposable, (disposable, subject, topic, data) => {

I don't think we need this indirection anymore, the unloader function created in setupDisposable can just call unload(disposable, reason) directly.

Comment 13

[:Gijs (he/him)]

(Adding this back to the metabug so I can keep track of what is/isn't filed.)

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8725210 [details]
MozReview Request: Bug 1250469 - Reduce number of ObserverService observers subscribed by the SDK Disposable class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37365/diff/2-3/

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Push to try of the updated patch attached above:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cfe4c9d3acb5fd5263a81fa8b55727ec65a4e6c

Comment 16

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8725210 [details]
MozReview Request: Bug 1250469 - Reduce number of ObserverService observers subscribed by the SDK Disposable class.

https://reviewboard.mozilla.org/r/37365/#review52444

Not much I can add to this patch, thanks Luca, great work!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/16161b126fe8

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16161b126fe8