1791788 - Firefox 106.0 beta NativeMessaging error

Status: VERIFIED FIXED

(WebExtensions :: Compatibility, defect, P1)

Description

[Michel Gutierrez]

Steps to reproduce:

Install the Video DownloadHelper extension (https://addons.mozilla.org/firefox/addon/video-downloadhelper/) and its companion app (https://www.downloadhelper.net/install-coapp) on Windows 10.

Actual results:

On Firefox 105 and previous, Video DownloadHelper reports companion app installed.
On Firefox 106.0b2 (and 106.0b1 as reported by several extension users) it shows as uninstalled.

Expected results:

The native messaging companion application should report as installed whatever version of Firefox > 57.

This issue does not happen on Firefox 106 for Linux.

Note that Video DownloadHelper has 2 million active users, most of them running Windows and the companion is required for most video download operations.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Audio/Video: Playback' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[:Gijs (he/him)]

Flagging some webextension folks in case they can help move this forward.

Comment 3

[Rob Wu [:robwu]]

We haven't changed anything recently with the native messaging implementation.

On Windows, the location of the native messaging manifest is read from the registry. Did anything change there?

Mig, do you see anything useful in the browser console? There are typically debugging messages printed there if the native messaging host cannot be found, as explained at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Native_messaging#troubleshooting

Comment 4

[Rob Wu [:robwu]]

I just tried it in a Windows 10 VM, and see the following error after installing coapp and starting the extension:

File at path "C:\Program Files\net.downloadhelper.coapp\bin/net.downloadhelper.coapp-win-64.exe" does not exist, or is not executable

The error message is from https://searchfox.org/mozilla-central/rev/52da19becaa3805e7f64088e91e9dade7dec43c8/toolkit/modules/subprocess/Subprocess.jsm#154-161

and the cause of the change is the error at https://searchfox.org/mozilla-central/diff/449d91d3191f58af0031685acfe89946b64a7bab/toolkit/modules/subprocess/subprocess_win.jsm#120-122

The previous logic accepted / as path separators on Windows, the new logic does not.

Comment 5

[Rob Wu [:robwu]]

mig - until a fix is out for Firefox, you can fix this by using \\ as path separators in the native messaging manifest, instead of /.

On Firefox's side, the question is whether Subprocess.jsm should normalize the path as it did before, or whether the caller should be updated to pass platform-specific paths. In this specific case, the latter would have to be implemented at https://searchfox.org/mozilla-central/rev/52da19becaa3805e7f64088e91e9dade7dec43c8/toolkit/components/extensions/NativeMessaging.jsm#87-96

Gijs - should Subprocess.jsm normalize paths as before, or should we change the logic in NativeMessaging.jsm? I'm fine with doing the latter, but I wonder whether there are other potential regressions by callers that expected the path-normalization behavior.

Comment 6

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #5)

mig - until a fix is out for Firefox, you can fix this by using \\ as path separators in the native messaging manifest, instead of /.

On Firefox's side, the question is whether Subprocess.jsm should normalize the path as it did before, or whether the caller should be updated to pass platform-specific paths. In this specific case, the latter would have to be implemented at https://searchfox.org/mozilla-central/rev/52da19becaa3805e7f64088e91e9dade7dec43c8/toolkit/components/extensions/NativeMessaging.jsm#87-96

Gijs - should Subprocess.jsm normalize paths as before, or should we change the logic in NativeMessaging.jsm? I'm fine with doing the latter, but I wonder whether there are other potential regressions by callers that expected the path-normalization behavior.

I think nativemessaging should normalize its paths, not subprocess.jsm . All the other non-mac/linux-specific consumers are:

• browser/components/shell/ShellService.jsm - passes an nsIFile-generated path
• the browser toolbox - passes an nsIFile-generated path or an environment variable (up to the user / environment to deal with)
• toolkit/modules/Troubleshoot.jsm - only reads the environment and doesn't need to normalize anything.

So I don't think there'd be other regressions, and I think native messaging probably needs updating. We've run into at least 1 other similar issue caused by IOUtils not doing this normalization while OS.File did, but I don't have a reference to it to hand, unfortunately. At the time I think Barret and I determined that we would prefer to discourage people from hardcoding or hand-rolling path concatenation rather than pretend we can get path-normalization right across platforms which is... ambitious, in the general case (esp. for finalized absolute paths) - if using PathUtils or nsIFile and the directory service, none of this should go wrong, in principle. I do recognize this is a bit of a pain for webextension code as it has more arbitrary input to deal with, and I'm sorry for not catching this at review time...

Comment 7

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #6)

(In reply to Rob Wu [:robwu] from comment #5)

Gijs - should Subprocess.jsm normalize paths as before, or should we change the logic in NativeMessaging.jsm? I'm fine with doing the latter, but I wonder whether there are other potential regressions by callers that expected the path-normalization behavior.

I think nativemessaging should normalize its paths, not subprocess.jsm . All the other non-mac/linux-specific consumers are:

• browser/components/shell/ShellService.jsm - passes an nsIFile-generated path
• the browser toolbox - passes an nsIFile-generated path or an environment variable (up to the user / environment to deal with)
• toolkit/modules/Troubleshoot.jsm - only reads the environment and doesn't need to normalize anything.

So I don't think there'd be other regressions, and I think native messaging probably needs updating.

Ok, I'll patch NativeMessaging.jsm then.

We've run into at least 1 other similar issue caused by IOUtils not doing this normalization while OS.File did, but I don't have a reference to it to hand, unfortunately.

Luca was thinking of bug 1759162.

At the time I think Barret and I determined that we would prefer to discourage people from hardcoding or hand-rolling path concatenation rather than pretend we can get path-normalization right across platforms which is... ambitious, in the general case (esp. for finalized absolute paths) - if using PathUtils or nsIFile and the directory service, none of this should go wrong, in principle. I do recognize this is a bit of a pain for webextension code as it has more arbitrary input to deal with, and I'm sorry for not catching this at review time...

Np. I'll work on a patch.

Comment 8

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #7)

We've run into at least 1 other similar issue caused by IOUtils not doing this normalization while OS.File did, but I don't have a reference to it to hand, unfortunately.

Luca was thinking of bug 1759162.

Heh, surprisingly, I have not seen that one before so I guess "at least 2"? :-(

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1791788 - Normalize native messaging path on Windows

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1791788 - Add test coverage for nativeMessaging manifest parsing

This patch refactors the existing nativeMessaging tests, by introducing
a shared helper to test bad nativeMessaging hosts. It also introduces a
hook to the test helper to allow the test to easily manipulate the
generated native messaging manifest to intentionally trigger the error
conditions. These are used to add test coverage for scenarios that were
previously not covered.

test_forward_slashes_in_path_works is the regression test for the linked
bug.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8dd984f0613
Normalize native messaging path on Windows r=rpl
https://hg.mozilla.org/integration/autoland/rev/05153bbfaa1d
Add test coverage for nativeMessaging manifest parsing r=rpl

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 9295890 [details]
Bug 1791788 - Normalize native messaging path on Windows

Beta/Release Uplift Approval Request

• User impact if declined: Windows-only: Extensions relying on native messaging hosts won't work if the path from their manifest file uses forward slashes instead of backslashes as path separators. This is a regression that has not reached release yet.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Already covered by unit tests, but to manually verify:

1. On Windows, install https://www.downloadhelper.net/install-coapp
2. Install the add-on at https://addons.mozilla.org/firefox/addon/video-downloadhelper/
3. Click on the extension button to open the extension panel.
4. Click on the Gear icon to open the settings page.

Expected:

• "Companion App installed"

Before the bug fix:

• "Companion App not installed"

Note: the extension here can work around the problem by patching their installer. If that happens, then you won't be able to use this extension as a test case any more. In either case the extension would function as expected, and the new unit tests already ensure that the specific scenario has test coverage.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Well-understood issue. Simple fix targets specific issue without side effects.
• String changes made/needed:
• Is Android affected?: No

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a8dd984f0613
https://hg.mozilla.org/mozilla-central/rev/05153bbfaa1d

Comment 14

[Alex Cornestean]

Verified fixed on the latest Nightly (107.0a1/Build ID 20220923093059).

Performing the steps provided in Comment 12, shows the companion app as installed when accessing the extension’s settings page, confirming the fix.

See the attached screenshot for more details.

Comment 15

[Alex Cornestean]

Attachment: 2022-09-23_15h28_50.png

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9295890 [details]
Bug 1791788 - Normalize native messaging path on Windows

Approved for 106.0b4, thanks.

Comment 17

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/cd05f18beeb7
https://hg.mozilla.org/releases/mozilla-beta/rev/14fafb9256f6

Comment 18

[Alex Cornestean]

Verified fixed on the latest Beta (106.0b4/20220925185751).

Performing the steps provided in Comment 12, shows the companion app as installed when accessing the extension’s settings page, confirming the fix.
See the screenshot for more details.

Comment 19

[Alex Cornestean]

Attachment: 2022-09-26_11h56_54.png

Comment 20

[Michel Gutierrez]

Many thanks all for your quick move !