1547463 - Delete/Rename Profile confirmation dialog buttons are missing in Profile Manager. And missing label in "Firefox is already running" dialog

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Alice0775 White]

Attachment: screenshot Good VS Bad

Reproducible: always

Sreps To Reproduce:

1. Start firefox.exe -P
2. Select a profile
3. Click on [Delete Profile...] button
   OR
   Click on [Rename Profile...] button

Actual Results:
Confirmation dialog buttons are missing label.

Expected Results:
Buttons label should appear properly.

Regression Window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a885e5f28d3141921d9235adfdd0b88552e507a2&tochange=63fcd49131d7303b868a34241024d4a72ed1aafc

Regressed by: Bug 1538983

FYI, Unfortunately, Bug 1547346 does not fixes this problem.

Comment 1

[Alice0775 White]

Attachment: image.png

"Firefox is already running" dialog is also affected

Comment 2

[Dave Townsend [:mossop]]

Andrew, can you take this?

Comment 3

[Andrew Swan [:aswan]]

Yep. It looks like commonDialog.xul needs to include customElements.js
I'll also see if I can find any other xul that uses buttons that does not already include customElements.js before uploading a patch, I can do that Monday.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Andrew Swan [:aswan] from comment #3)

Yep. It looks like commonDialog.xul needs to include customElements.js
I'll also see if I can find any other xul that uses buttons that does not already include customElements.js before uploading a patch, I can do that Monday.

I'm not sure that there's a good way to get this info since I assume a lot of these documents aren't well tested in automation, but it would be great to get a list of documents that can load before MainProcessSingleton starts running. We had similar issues in Bug 1501845 and Bug 1459985.

Comment 5

[Brian Grinstead [:bgrins]]

I wonder if there's a better place than MainProcessSingleton to load the customElements.js file (https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/toolkit/components/processsingleton/MainProcessSingleton.jsm#58). Somewhere that initializes earlier. Or port CustomElementsListener.jsm logic out of a JSM and into C++ directly when document-element-inserted is firing.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #5)

I wonder if there's a better place than MainProcessSingleton to load the customElements.js file (https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/toolkit/components/processsingleton/MainProcessSingleton.jsm#58). Somewhere that initializes earlier. Or port CustomElementsListener.jsm logic out of a JSM and into C++ directly when document-element-inserted is firing.

Boris, we are hitting cases like this bug and the bugs linked in Comment 4 where we aren't hooking into the document-element-inserted observer early enough (we are currently waiting until MainProcessSingleton is loaded and "app-startup" fires which seems to be after stuff like Profile Manager UI is loaded). There are fixes for this (add customElements.js as a <script> tag into affected documents) but I'm wondering if we could do better.

I know you've become familiar with some of this code (as per Bug 1528762) and you probably have context around if these are feasible and/or a good idea. What do you think about:

1. Make MainProcessSingleton load the CustomElementsListener earlier (before any documents are created). Maybe there's some existing option we could add to https://searchfox.org/mozilla-central/source/toolkit/components/processsingleton/ProcessSingleton.manifest to make this happen?
2. Port this code https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/toolkit/components/processsingleton/CustomElementsListener.jsm#14-25 directly into https://searchfox.org/mozilla-central/rev/66086345467c69685434dd1c5177b30a7511b1a5/dom/base/nsContentSink.cpp#1568-1569.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm. I don't know enough about startup to say whether we can add anything to the manifest to make MainProcessSingleton run earlier, but we can probably try the following things:

1. Explicitly instantiate the MainProcessSingleton service by contractid from nsContentUtils::Init or nsLayoutStatics::Initialize. This would certainly get run before we got any documents.

2. Do the instantiation by contractid lazily, triggered from the content sink code you linked in comment 6. that would ensure that it's instantiated before we dispatch the observer notification.

3. Inline the code into the content sink, as you said. It's a bit of a PITA because it needs various state set up that xpconnect is handling for us right now, but could be doable.

We'd still need to do the observer notification, because this is not the only listener for document-element-inserted.

I suspect option 1 or 2 is simpler (and a smaller difference from what we have right now) than option 3.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)

Hmm. I don't know enough about startup to say whether we can add anything to the manifest to make MainProcessSingleton run earlier, but we can probably try the following things:

1. Explicitly instantiate the MainProcessSingleton service by contractid from nsContentUtils::Init or nsLayoutStatics::Initialize. This would certainly get run before we got any documents.

2. Do the instantiation by contractid lazily, triggered from the content sink code you linked in comment 6. that would ensure that it's instantiated before we dispatch the observer notification.

3. Inline the code into the content sink, as you said. It's a bit of a PITA because it needs various state set up that xpconnect is handling for us right now, but could be doable.

We'd still need to do the observer notification, because this is not the only listener for document-element-inserted.

I suspect option 1 or 2 is simpler (and a smaller difference from what we have right now) than option 3.

Thanks Boris. I guess we could also do (1) or (2), but also load Custom Elements listener into a brand new component with it's own contractid instead of initializing it from MainProcessSingleton (registered in https://searchfox.org/mozilla-central/source/toolkit/components/processsingleton/components.conf). This would prevent any side effects from changing the timing on when MainProcessSingleton gets instantiated.

I guess it'd be a bit strange to register whichever singleton we are going to use as "app-startup" at https://searchfox.org/mozilla-central/source/toolkit/components/processsingleton/ProcessSingleton.manifest since we would explicitly instantiate it before that. I haven't worked much with this registration system so it's possible I'm missing some context here. :kmag - would registering a component in components.conf be enough for this use case or do we also need to include it in the .manifest file with a category?

Comment 9

[Brian Grinstead [:bgrins]]

Andrew, in the meantime we can fix this bug by including a <script> to customElements.js in the affected documents (as done in Bug 1501845 and Bug 1459985).

Comment 10

[Dave Townsend [:mossop]]

As far as I can tell the only windows that we open before the "app-startup" notification (which is what registers the process singleton) are:

"chrome://global/content/commonDialog.xul"
"chrome://mozapps/content/profile/profileDowngrade.xul"
"chrome://mozapps/content/profile/profileSelection.xul"
"chrome://browser/content/migration/migration.xul"

Plus any windows that those open, which seems to be just:

"chrome://mozapps/content/profile/createProfileWizard.xul"

So I wonder whether it is worth doing anything more than just one-offing those cases. That said these windows probably also are impossible to localize with fluent right now so....

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Dave Townsend [:mossop] (he/him) from comment #10)

As far as I can tell the only windows that we open before the "app-startup" notification (which is what registers the process singleton) are:

"chrome://global/content/commonDialog.xul"
"chrome://mozapps/content/profile/profileDowngrade.xul"
"chrome://mozapps/content/profile/profileSelection.xul"
"chrome://browser/content/migration/migration.xul"

Plus any windows that those open, which seems to be just:

"chrome://mozapps/content/profile/createProfileWizard.xul"

So I wonder whether it is worth doing anything more than just one-offing those cases. That said these windows probably also are impossible to localize with fluent right now so....

Curious, how'd you generate that list?

Comment 12

[Boris Zbarsky [:bzbarsky]]

I guess we could also do (1) or (2), but also load Custom Elements listener into a brand new component

Yes, we could.

Comment 13

[Andrew Swan [:aswan]]

Attachment: Bug 1547463 Explicit load customElements.js where necessary r=bgrins

commonDialog.xul and profileDowngrade.xul both may load early enough
during startup that they don't automatically get customElements.js.
The quick workaround here is just to load it explicitly in those documents.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d507bf325bf7
Explicit load customElements.js where necessary r=bgrins

Comment 15

[Kris Maglione [:kmag]]

(In reply to Brian Grinstead [:bgrins] from comment #8)

:kmag - would registering a component in components.conf be enough for this use case or do we also need to include it in the .manifest file with a category?

You can register category entries in components.conf as long as their value is the same as the component's contract ID.

In general, I'd prefer that we import new JSMs using do_ImportModule rather than registering them as components, but if you want to load them via a category registration, I guess a component is still the way to go.

Comment 16

[Dave Townsend [:mossop]]

(In reply to Brian Grinstead [:bgrins] from comment #11)

(In reply to Dave Townsend [:mossop] (he/him) from comment #10)

As far as I can tell the only windows that we open before the "app-startup" notification (which is what registers the process singleton) are:

"chrome://global/content/commonDialog.xul"
"chrome://mozapps/content/profile/profileDowngrade.xul"
"chrome://mozapps/content/profile/profileSelection.xul"
"chrome://browser/content/migration/migration.xul"

Plus any windows that those open, which seems to be just:

"chrome://mozapps/content/profile/createProfileWizard.xul"

So I wonder whether it is worth doing anything more than just one-offing those cases. That said these windows probably also are impossible to localize with fluent right now so....

Curious, how'd you generate that list?

I just read through all the cases that we initialise ScopedXPCOMStartup.

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d507bf325bf7