1369443 - WebContentConverter.js is loaded too early during startup

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement, P2)

Description

[Florian Quèze [:florian]]

WebContentConverter.js is currently loaded during app-startup only to register a browser-ui-startup-complete observer.

We should push this at least to after first paint (maybe instantiate this component from nsBrowserGlue.js?), but it would be even better to load this lazily when it's actually needed. Bug 539869 explored this a bit several years ago, and concluded that it was difficult, but things may have changed a bit now.

Paolo, IIRC you touched related code relatively recently, do you have an opinion about when/where we should initialize this?

Comment 1

[:Paolo Amadini]

So, "browser-ui-startup-complete" is kind of a misleading name since this is currently called in "_finalUIStartup" which runs "before the first command line handler is invoked (i.e. before the first window is opened)".

Moving the registerFactory calls made by WebContentConverter.js to a later arbitrary point, especially after paint, might mean that we add a race condition for which our handlers are not registered when we need them for the process described in bug 1362564. This might easily happen for the first loaded page. The process in bug 1362564 happens for all docshell loads in general, including the browser chrome, and so moving the initialization there would make us do things earlier and not later, hence the WONTFIX in bug 539869.

I think the latest safe point where we can register the handlers is before we start our first page load. I'm not sure this is much later than now.

Of course, we could call the initialization directly from nsBrowserGlue.js without going through "browser-ui-startup-complete". The XPCOM module however is registered on startup to implement the registerContentHandler API, so to have any performance gain from this, the two responsibilities would have to be split into different modules.

Comment 2

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #1)

> I think the latest safe point where we can register the handlers is before
> we start our first page load. I'm not sure this is much later than now.

I think I still don't understand what prevents us from initializing the handlers the first time we need to find the handler for something.

> The XPCOM module
> however is registered on startup to implement the registerContentHandler
> API, so to have any performance gain from this, the two responsibilities
> would have to be split into different modules.

I think this is implemented using messages coming from the content process, so nsBrowserGlue could listen for these messages and initialize the component in charge of registerContentHandler the first time it's actually needed (similar to what bug 1358921 did for lots of modules).

Comment 3

[:Paolo Amadini]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> I think I still don't understand what prevents us from initializing the
> handlers the first time we need to find the handler for something.

Actually, it depends on whether we'd try to find a handler for our early chrome loads. According to bug 1362564 this may happen if we use a stream converter for the type, because it's done after we already tried to find a handler. In this case, we'd initialize the module earlier and not later than now.

Comment 4

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #3)
> (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > I think I still don't understand what prevents us from initializing the
> > handlers the first time we need to find the handler for something.
> 
> Actually, it depends on whether we'd try to find a handler for our early
> chrome loads. According to bug 1362564 this may happen if we use a stream
> converter for the type, because it's done after we already tried to find a
> handler. In this case, we'd initialize the module earlier and not later than
> now.

Unless I'm misremembering, "application/rss+xml" and "application/atom+xml" are the only content types handled by this file, I don't expect us to load an rss feed during early startup. And if some users set an rss feed as their homepage, I'm fine with Firefox starting slightly slower for them :-).

Comment 5

[:Paolo Amadini]

Maybe we're talking about two different things. I think you were suggesting replacing the registerFactory calls made by WebContentConverter.js with a different system, invoked in step 6, that does not require us to make these calls in advance?

Comment 6

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #5)
> Maybe we're talking about two different things. I think you were suggesting
> replacing the registerFactory calls made by WebContentConverter.js with a
> different system, invoked in step 6, that does not require us to make these
> calls in advance?

Maybe that was implied when I said "initializing the handlers the first time we need to find the handler for something" in comment 2, but I didn't look that deep in the code.

All I'm saying for now is that unless we load an rss feed or a link with a protocol that's not http/https/ftp/about/data, I see no reason for us to need anything from WebContentConverter.js to be initialized.

Comment 7

[:Paolo Amadini]

Actually, I chatted about this with Felipe and it seems this is probably dead code, at least if there are no add-ons making obsolete handler registrations using preferences directly:

https://dxr.mozilla.org/mozilla-central/source/browser/components/feeds/WebContentConverter.js#731-734

If actually confirmed, we won't need to initialize the module on startup at all!

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Tentatively marking this fxperf:p2 seeing as how it impacts florian's startup area of concentration.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Sorry, didn't mean to needinfo.

Comment 10

[:Gijs (he/him)]

For registering things, we should just get instantiated as-needed by Navigator.cpp . I don't *think* the module needs to be loaded for it to redirect any registered protocol/content handlers, but I haven't looked in detail so maybe I'm wrong. I'll try and get to this later this week or the next.

Comment 11

[:Gijs (he/him)]

OK, so for context, WebContentConverter.js does a few things:

1. provide some implementation of registerProtocolHandler. This can get instantiated by Navigator.cpp on an as-needed basis. Some of the checks could likely move into Navigator.cpp anyway, reducing the number of times we'd need to load the JS. In fact, given that at the end of the checks, all the JS does is send a message to the parent, perhaps that too (ie the message sending) should move into Navigator.cpp . The parent side of things shows browser UI so we probably want to keep a delegated browser implementation so it can show a notification.

Registered protocol handlers are stored in the uriloader's handler service, which presumably also takes care of resuscitating them on next startup.

2. provide some implementation of registerContenthandler. This is disabled from 62 onwards (bug 1460481). We register handlers with the stream converter service and point them to a factory at the top of the file, which creates new instances of a mini-implementation of the stream converter, which just looks up the associated webnavigation, and loads the registered URL if it finds any. We store registered handlers in the prefs (*wince*). Which brings us to...

3. from startup, read prefs for registered content handlers, and re-register them in both processes.

The first part, as noted, doesn't need anything loaded immediately. The second doesn't really, either - the issue is the third one.

While I could spend some time writing a solution here (e.g. making some C++ component responsible for restoring these handlers out of the prefs, which would be fairly straightforward)... there's not really any point because we've already disabled stuff on release.

Jonathan, what's the future of registerContentHandler? It looks to me like it was disabled (behind an about:config pref) for 62, which we haven't yet released. I'm a little twitchy about writing a full removal of this stuff now, because it seems possible to me that for some reason we will have to back things out.

On the other hand, we're looking at removing our own feed handling code, and part of that decision is the death of RSS/Atom generally, so I don't see anybody actually using this stuff... Looking through searchcode.com I found only 1 use in a repo that's still maintained ( I filed https://github.com/samuelclay/NewsBlur/issues/1116 ) .

I could conceivably put this on hold until 64 (or perhaps work on extracting some of the registerProtocolHandler stuff so removal of this particular module becomes easier). Or we could just forge ahead and rip it out in the hope that 62 rolls around without us feeling like we suddenly need to resuscitate this stuff...

Comment 12

[:Gijs (he/him)]

Note that registerContentHandler is already (and has already for a long time been) limited to feed types, so you could only register to convert rss/atom feeds, which means it's not exactly a universally appealing tool to render arbitrary content as HTML...

Comment 13

[Jonathan Kingston [:jkt] he/him]

> what's the future of registerContentHandler? It looks to me like it was disabled (behind an about:config pref) for 62, which we haven't yet released. I'm a little twitchy about writing a full removal of this stuff now, because it seems possible to me that for some reason we will have to back things out.

We have no future for this. I was going to rip this code out as soon as we release 62. If it's getting in the way I don't think there is harm in expediting this timeline. No other browsers support this and there wasn't pushback.

If I were you I would write the patch with the assumption that about:reader and registerContenthandler don't work imo.

The alternative might be something more conservative that wraps the steps of init() and the observer registration with a build flag for Nightly that are no longer needed.

> given that at the end of the checks, all the JS does is send a message to the parent, perhaps that too (ie the message sending) should move into Navigator.cpp

This might be a good follow up, I think once 64 hits there isn't much need for this being in JS at all.

Comment 14

[:Gijs (he/him)]

Fixed by removing most of that service in bug 1477670.