1670147 - Firefox AccessibleHandler gets clobbered when Thunderbird is installed/updated

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

AccessibleHandler.dll has to be registered in the registry with various CLSIDs and IIDs. We have a bunch of different ids which we use for different channels, local builds, etc., which prevents multiple installations of different types from clobbering each other. However, Thunderbird ends up using the same ids as Firefox for the same channel/type; e.g. Firefox nightly and Thunderbird nightly use the same ids. This means that whenever Thunderbird is installed or updated, it clobbers Firefox AccessibleHandler, causing severe performance (and other) problems for Firefox. The reverse is also true, but because Thunderbird doesn't depend on e10s anywhere near as much as Firefox (if at all?), it doesn't hurt Thunderbird so much.

Note that users are encountering this in the wild and wondering why Firefox performance suddenly goes to hell one day.

Given how severe this is for Firefox (and that Firefox is the heaviest user of Gecko by far), I propose we define specific ids for Firefox. Thunderbird could choose to define different ids as well, but that's not something I'm going to address here.

Comment 1

[James Teh [:Jamie]]

To make this distinction, we'll probably go with CONFIG['MOZ_APP_NAME'] == 'firefox' in moz.build. I considered a couple other options, but they're less precise:

1. MOZ_THUNDERBIRD and MOZ_SUITE defines, but there might be other apps which use Gecko and don't define these; e.g. Instantbird or Waterfox.
2. CONFIG['MOZ_BUILD_APP'] == 'browser', but there might be other apps which set this; e.g. Waterfox.

We'll need to tweak accessible/ipc/win/handler/moz.build to add a USE_FIREFOX_UUID define and have accessible/ipc/win/handler/handlerData.idl check that.

It looks like the Firefox uninstaller also defines the AccessibleHandler CLSID, so that will need to be tweaked as well:
https://searchfox.org/mozilla-central/rev/919607a3610222099fbfb0113c98b77888ebcbfb/browser/installer/windows/nsis/defines.nsi.in#63

Comment 2

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #1)

To make this distinction, we'll probably go with CONFIG['MOZ_APP_NAME'] == 'firefox' in moz.build.

Or not. From Glandium on Matrix:

I'd say the uuid should just be defined in branding
that would remove a lot of problems from channel being wrong, etc.
that would also force downstream to be concious about it
I feel like the right thing would be to add the necessary variables to $MOZ_BUILD_APP/branding/*/configure.sh, AC_SUBST() them in old-configure.in, and use their value in the necessary places, with a fallback to the local uuids for DEVELOPER_OPTIONS.

Comment 3

[Rob Lemley [:rjl]]

Attachment: Bug 1670147 - Set values for AccessibleHandler.dll CLSID/IIDs at configure time. r?jamie,#firefox-build-system-reviewers

Allow for downstream projects such as Thunderbird to set different UUIDs for
AccessibleHandler to avoid clashes when both applications are installed.
The IDs themselves can be defined in confvars.sh or in branding/configure.sh
depending on the specific needs of the application.

Comment 4

[Rob Lemley [:rjl]]

Hopefully I understand this correctly, and allowing HANDLER_CLSID, IHANDLERCONTROL_IID, ASYNCIHANDLERCONTROL_IID, and IGECKOBACKCHANNEL_IID to be set at configure time is what you had in mind.

For testing, I've added new IDs to Thunderbird's confvars.sh and tried to replicate the logic from HandlerData.idl.

Obviously this work is incomplete, looking for feedback on the strategy.

Comment 5

[Nick Alexander :nalexander [he/him]]

Based on this searchfox query for CLSID, I expect that the installers (for each product) will need to be updated to accommodate this change. I can help with this if needed.

And it's not clear to me how exactly this will work on update. How does this work on update? Ah, I see that this work is considered incomplete -- Install/Update team can help with working on this strategy. :agashlin, can you give this a skim and evaluate this change?

Comment 6

[Rob Lemley [:rjl]]

The last upload to Phabricator should be what's needed for the Firefox installer. The Thunderbird changes are in bug 1689791. For Firefox I tried to replicate what was happening in HandlerData.idl into the branding configure.sh files using the existing IDs. For Thunderbird I generated new IDs.

For updates, after the files are updated, helper.exe /PostUpdate runs. helper.exe is built from uninstaller.nsi. That's unregistering the handler then it's re-registered when Firefox/Thunderbird restarts(?) from https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/accessible/ipc/win/handler/AccessibleHandler.cpp#2123.

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Executive summary: The patch as it stands is probably fine, if the GUIDs change it will leave around the old registrations.

(In reply to Rob Lemley [:rjl] from comment #6)

(In reply to Nick Alexander :nalexander [he/him] from comment #5)

Based on this searchfox query for CLSID, I expect that the installers (for each product) will need to be updated to accommodate this change. I can help with this if needed.

The last upload to Phabricator should be what's needed for the Firefox installer. The Thunderbird changes are in bug 1689791. For Firefox I tried to replicate what was happening in HandlerData.idl into the branding configure.sh files using the existing IDs. For Thunderbird I generated new IDs.

Yeah, just changing AccessibleHandlerCLSID as in the current patch should get you most of the way there, mostly the installer/uninstaller just need to register/unregister the DLL, which knows its own GUIDs. The only place it's explicitly needed is the check to make sure we only unregister the DLL from our own installation.

PostUpdate if the GUIDs change is a different story, though...

(In reply to Rob Lemley [:rjl] from comment #6)

For updates, after the files are updated, helper.exe /PostUpdate runs. helper.exe is built from uninstaller.nsi. That's unregistering the handler then it's re-registered when Firefox/Thunderbird restarts(?) from https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/accessible/ipc/win/handler/AccessibleHandler.cpp#2123.

helper.exe /PostUpdate doesn't unregister the DLL [1], this wouldn't work from PostUpdate anyway since the DLL doesn't know the old GUID. PostUpdate does register the DLL, this is what runs the DLL's DllRegisterServer() that you linked above. I think this results in an old registration hanging around, though I don't know if this is harmful. We could unregister and re-register it every update, but a) this might not be harmless, b) unregistering is not straightforward if we don't know the old GUIDs.

If we want to unregister the old GUIDs, some options:

1. Unregister the DLLs before update (though what if update fails), or
2. Look through every registration in order to find any other entries in HKCR with our path and remove them on PostUpdate (unwieldy, tricky with IIDs), or
3. Store the "old" GUIDs somewhere so they could be explicitly removed (if they match this installation) on PostUpdate

I don't know how to test this properly. I tried with NVDA, but it seemed like it always works regardless of whether the DLLs are registered and even seemed to behave the same if I deleted them entirely. Inspect.exe said that I was always using an MSAA proxy, so it's possible the handler just wasn't working at all on the builds I tried.

---

[1] It's true that helper.exe is built from uninstaller.nsi, but that's misleading: uninstaller.ini/helper.exe is actually an installer. When it is run without command line options (such as /PostUpdate) it writes out an uninstaller and runs it. Only in this true uninstaller do we get to Section "Uninstall", which is the only place that uses UnregisterDLL. (There's also a use in un.UnRegDLLsCallback, I believe this is unused along with un.ParseUninstallLog, and was only for uninstall anyway; I filed bug 1717070 to remove it.)

Comment 8

[Rob Lemley [:rjl]]

Thanks for clarifying Adam. In theory, I kept the GUIDs for Firefox, just moved them into branding. The logic is different though and warrants scrutiny. The Thunderbird patch to go with this in bug 1689791 has newly generated GUIDs.

IIUC, Thunderbird will update, leave behind dangling DLL registrations. If Firefox updates after that happens, it will run DllRegisterServer(). Doesn't that remove the dangling Thunderbird entries? If that's the case then they're not really a problem.

Comment 9

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Rob Lemley [:rjl] from comment #8)

IIUC, Thunderbird will update, leave behind dangling DLL registrations. If Firefox updates after that happens, it will run DllRegisterServer(). Doesn't that remove the dangling Thunderbird entries? If that's the case then they're not really a problem.

Yes, I expect it would replace those entries.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8a02b23b2c5
Set values for AccessibleHandler.dll CLSID/IIDs at configure time. r=Jamie,firefox-build-system-reviewers,glandium

Comment 11

[Iulian Moraru]

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

Comment 12

[Rob Lemley [:rjl]]

Comment on attachment 9227598 [details]
Bug 1670147 - Set values for AccessibleHandler.dll CLSID/IIDs at configure time. r?jamie,#firefox-build-system-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Windows users with both Firefox and Thunderbird installed may have degraded performance, especially in Firefox. As noted, the issue has affected Firefox more severely. For Thunderbird, bug 1689791 will need uplift as well. This request is being made to facilitate getting this fixed in esr91.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: As noted in the comments, there's no clear way to test.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This would only impact users who have both Firefox and Thunderbird installed. The lack of test strategy does increase the risk. However the actual accessibility code does not change with this patch, only the method by which GUIDs are assigned changes. Those GUIDs do not change for Firefox. They will for Thunderbird.
• String changes made/needed: n/a

Comment 13

[Rob Lemley [:rjl]]

Jamie, anything to add to the uplift request?

Comment 14

[James Teh [:Jamie]]

(In reply to Rob Lemley [:rjl] from comment #12)

• If yes, steps to reproduce: As noted in the comments, there's no clear way to test.

It might be difficult to test the performance degradation, but there is a clear way to verify the fix at least:

1. Install Firefox.
2. Ensure Firefox is not running.
3. Install Thunderbird.
4. Start Firefox.
5. Visit about:support in Firefox.
6. In the Accessibility section, check the value for "Accessible Handler Used".
   • Expected: true
   • Actual: false

Verifying the fix will require a version of Thunderbird built with the fix from this bug and maybe the fix from bug 1689791 (although the latter mightn't be necessary since Thunderbird should use the fallback UUIDs implemented in the fix for this bug).

Comment 15

[Pascal Chevrel:pascalc]

Comment on attachment 9227598 [details]
Bug 1670147 - Set values for AccessibleHandler.dll CLSID/IIDs at configure time. r?jamie,#firefox-build-system-reviewers

This is too late and risky for our last beta, we should let it bake a cycle and can uplift it to the ESR 91 branch with the 91.1 release

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9227598 [details]
Bug 1670147 - Set values for AccessibleHandler.dll CLSID/IIDs at configure time. r?jamie,#firefox-build-system-reviewers

Approved for 91.1esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/958aef401f3c