1250487 - [e10s] FormSubmitObserver should be a singleton in a process script

Status: RESOLVED WONTFIX

(Firefox :: General, defect, P3)

Description

[:Gijs (out for now; he/him)]

All it does that would need to happen for every frame is sending up a "hidePopup" event when the root document fires a "pageshow" event. This could be done from the parent process.

Everything else is chained up from the observer notification, which means it could all be handled from a singleton observer in a process script, that registers the remaining listeners (input, blur, and unload to unregister those listeners) when it gets called for a content frame with the relevant observer notification.

Comment 1

[Eric Rahm [:erahm]]

Gijs, do you know if this is still an issue, if so what kind of overhead would you expect? It looks like we've at least moved it to a LazyModuleGetter [1].

If there's a way to specifically cause it to be lazy loaded we can probably do the measurement with the tool from bug 1463569. 

[1] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/browser/base/content/content.js#28

Comment 2

[:Gijs (out for now; he/him)]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #1)
> Gijs, do you know if this is still an issue, if so what kind of overhead
> would you expect? It looks like we've at least moved it to a
> LazyModuleGetter [1].
> 
> If there's a way to specifically cause it to be lazy loaded we can probably
> do the measurement with the tool from bug 1463569. 
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> a80651653faa78fa4dfbd238d099c2aad1cec304/browser/base/content/content.js#28

Looks like bug 1461247 changed this so we don't load the actual JSM for every tab anymore - we only import the "real" jsm when we get the observer notification, but we still add an observer in every tab.

At the moment, that means that even though observer notification are content-process-global, we add 1 observer for every content tab, so individual content processes will have N observers for their N tabs. Even with process-per-origin, we'll have N observers for every N tabs/frames from that origin that live in the same process -- and whenever any observer gets a notification, we need to check whether the notification actually pertains to the window/frame that that particular observer cares about.

So I still think that we should ideally change this to not use the observer service anymore. Presumably we could either move this code so it's more tightly integrated with document/window objects (either from C++/Rust or from JS via XPCOM) or try to make it piggyback on a (chrome-only?) DOM event or the message manager or IPC or something.

In terms of overhead, compared to using something other than the observer but keeping the same code, there's "only" runtime overhead - whether the observer service or e.g. the event listener manager is storing a pointer to the object that gets a notification presumably doesn't matter in terms of memory, "just" in terms of efficiency/runtime when we invoke observers, because now we waste time trying to find the "right one".

For the JS vs. C++/Rust tradeoff I expect memory overhead is already pretty minimal now that we load this JSM lazily.

So although there are still (smaller) efficiency gains to be had here, I doubt that any of them will have a significant impact on the fission work. Felipe, does that sound right?

Comment 3

[:Gijs (out for now; he/him)]

(In reply to :Gijs (he/him) from comment #2)
> In terms of overhead, compared to using something other than the observer
> but keeping the same code, there's "only" runtime overhead - whether the
> observer service or e.g. the event listener manager is storing a pointer to
> the object that gets a notification presumably doesn't matter in terms of
> memory, "just" in terms of efficiency/runtime when we invoke observers,
> because now we waste time trying to find the "right one".

Thinking about this some more, I suppose that currently, for N tabs in one content process, whenever *any* of the tabs/frames fires the observer notification, we load the jsm in that process (which is space-O(1) for N tabs because 1 process!) *all* the N tabs instantiate their formsubmitobserver (which is space-O(N) because N observers). So I guess that could be optimized -- but project fission will drastically reduce N for most users. So it's probably not worth focusing on at the moment, unless we find out this happens "often" in practice, which naively seems unlikely. Unless all those ad/tracker frames somehow use forms or something...

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Yeah, Gijs's analysis is correct. If we keep using observers for this, it's either a matter of the C++ observer service iterating through the observers, or the singleton observer function iterating through the tabs, so it's not much of a win in any case. The best change would be to move away from the observer service and to use an event dispatched to the document.

There's a relatively simple improvement to make in what Gijs noted in comment 3 about when once the invalidformsubmit notification happens, all FormSubmitObservers gets instantiated. But it should be tracked in a separate bug because it's not gonna get rid of the overhead noted here, and Fission actually improves alleviates this prob.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

The main motivation for this bug as proposed was due to the fact that this was all driven by an observer notification, which made this code load for all tabs (in the same process) whenever the notification fired.

In bug 1474143 Matt is fixing this by going in the other direction, changing the notification to a chrome-only event targeted to the right document, which will solve this problem in a different manner.