Closed Bug 1791788 Opened 2 months ago Closed 2 months ago

Firefox 106.0 beta NativeMessaging error

Categories

(WebExtensions :: Compatibility, defect, P1)

Firefox 106
Desktop
Windows
defect
Points:
1

Tracking

(firefox-esr102 unaffected, firefox105 unaffected, firefox106+ verified, firefox107 verified)

VERIFIED FIXED
107 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox105 --- unaffected
firefox106 + verified
firefox107 --- verified

People

(Reporter: michel.gutierrez, Assigned: robwu)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [addons-jira])

Attachments

(4 files)

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.

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.

Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Points: --- → 13
Component: Audio/Video: Playback → General
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 10 → Windows
Hardware: x86_64 → Desktop

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

Component: General → Compatibility
Flags: needinfo?(rob)
Flags: needinfo?(mixedpuppy)
Product: Core → WebExtensions

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

Points: 13 → ---

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.

Flags: needinfo?(rob)
Flags: needinfo?(mixedpuppy)
Regressed by: 1772942

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.

Flags: needinfo?(gijskruitbosch+bugs)

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

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(rob)

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

Assignee: nobody → rob
Severity: -- → S3
Status: NEW → ASSIGNED
Points: --- → 1
Flags: needinfo?(rob)
Priority: -- → P1
Whiteboard: [addons-jira]

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

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.

Attachment #9295891 - Attachment description: Bug 1791788 - Add test coverage for manifest parsing → Bug 1791788 - Add test coverage for nativeMessaging manifest parsing
Pushed by rob@robwu.nl:
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 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
Attachment #9295890 - Flags: approval-mozilla-beta?
Attachment #9295891 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attached image 2022-09-23_15h28_50.png

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

Approved for 106.0b4, thanks.

Attachment #9295890 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9295891 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Attached image 2022-09-26_11h56_54.png

Many thanks all for your quick move !

You need to log in before you can comment on or make changes to this bug.