1371348 - formautofill shouldn't load anything before opening and painting the first browser window

Status: NEW

(Toolkit :: Form Autofill, defect, P3)

Description

[(no longer active)]

browser/base/content/test/performance/browser_startup.js indicates that the following scripts are loaded before opening the first window:

resource://formautofill/FormAutofillUtils.jsm
resource://formautofill/FormAutofillParent.jsm
resource://formautofill/FormAutofillContent.jsm

Comment 1

[Matthew N. [:MattN]]

Bug 1341008 is the reason for FormAutofillContent.jsm being loaded but I don't really understand why it's necessary. FAC in turn uses FormAutofillUtils.jsm too. It has something to do with preallocating processes I guess but it's not clear if there is a more proper solution to avoid eagerly loading FAC? Gabor?

Comment 2

[Matthew N. [:MattN]]

We may be able to load FormAutofillParent lazily by moving all of the listeners in FAP.init to bootstrap.js: https://dxr.mozilla.org/mozilla-central/rev/a49112c7a5765802096b3fc298069b9495436107/browser/extensions/formautofill/FormAutofillParent.jsm#77-88

This may not work with Sync though… we'll have to double-check that.

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #1)
> Bug 1341008 is the reason for FormAutofillContent.jsm being loaded but I
> don't really understand why it's necessary. FAC in turn uses
> FormAutofillUtils.jsm too. It has something to do with preallocating
> processes I guess but it's not clear if there is a more proper solution to
> avoid eagerly loading FAC? Gabor?

It's because of this line:
http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/browser/extensions/formautofill/FormAutofillContent.jsm#309

Let's say we start up a preallocated content process (so a content process without any frames) and then possibly a lot of time later we grab that process and start using it, load the FormAutofillContent.jsm from a frame script, then the Services.cpmm.initialProcessData.autofillEnabled might be outdated already because the "FormAutofill:enabledStatus" message listener were not active for a while and might have missed a state change.

The solution would be a tiny jsm loaded as a process script that only keeps an autofillEnabled flag updated and then FormAutofillContent.jsm could use that flag instead of initialProcessData. Does that make sense?

Comment 4

[Luke Chang [:lchang] (inactive)]

As discussed, this doesn't block MVP.

Comment 5

[Florian Quèze [:florian]]

(In reply to Luke Chang [:lchang] from comment #4)
> As discussed,

Where?

> this doesn't block MVP.

Why?

Comment 6

[Luke Chang [:lchang] (inactive)]

Sorry. I should have explained it clearly.

I discussed the MVP blockers with Matt couple months ago. We once talked about that this bug is definitely a blocker for photon but might not be a blocker for Form Autofill MVP. However, I might misunderstand something and should have confirmed before changing the flag.

On the other hand, as Address Autofill has been rolled out gradually on Fx56 and Credit Card Autofill is going to be enabled on Fx58 beta 5, the milestone, MVP, will soon draw to a close. Therefore, I was thinking to move bugs, which are supposed not to be uplifted to Fx58 or earlier, to our next milestone and track them there. That's why I removed "MVP" flag first.

While Matt has been working on it, we also have a plan to slightly refactor Form Autofill modules on Fx59 and will take this issue into account. I didn't mean to ignore this bug but just want to move this bug to the right milestone to track.

Sorry for not making it clear and please let me know if anything I did wrong. I'll fix it as soon as possible.

Comment 7

[Florian Quèze [:florian]]

Thanks for the explanation. I just want to ensure we don't lose track of performance bugs now that 57 has shipped :-).

Comment 8

[:Gijs (he/him)]

I don't see this in browser_startup.js . I don't see any version of browser_startup.js that ever had it. As a consequence, I am very confused. Is this still an issue?

Comment 9

[:Gijs (he/him)]

Eh, it's not obvious from comment #0, but this only shows up if looking at the output from running the test locally. I'm just going to mirror the priority Mike said earlier into the whiteboard to clear this out of triage.