Closed Bug 1547463 Opened 11 months ago Closed 11 months ago

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

Categories

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

68 Branch
Desktop
All
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox68 --- fixed

People

(Reporter: alice0775, Assigned: aswan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Attached image 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.

Flags: needinfo?(aswan)
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
OS: Windows 10 → All
Hardware: x86_64 → Desktop
Attached image image.png

"Firefox is already running" dialog is also affected

Summary: Delete/Rename Profile confirmation dialog buttons are missing in Profile Manager → Delete/Rename Profile confirmation dialog buttons are missing in Profile Manager. And missing label in "Firefox is already running" dialog

Andrew, can you take this?

Priority: -- → P1

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.

Assignee: nobody → aswan
Flags: needinfo?(aswan)

(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.

See Also: → 1501845, 1459985

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.

(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.
Flags: needinfo?(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.

Flags: needinfo?(bzbarsky)

(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?

Flags: needinfo?(kmaglione+bmo)

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).

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....

(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 guess we could also do (1) or (2), but also load Custom Elements listener into a brand new component

Yes, we could.

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.

Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d507bf325bf7
Explicit load customElements.js where necessary r=bgrins

(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.

Flags: needinfo?(kmaglione+bmo)

(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.

Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Duplicate of this bug: 1547703
You need to log in before you can comment on or make changes to this bug.