1289821 - Add logging if native messaging host manifest is missing on windows

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Andrew Swan [:aswan]]

The location of the host manifest on windows is given by a value from the registry.  If a value is set in the registry but there is no file at the given location (or if the file isn't readable), we should log a helpful error message.

Comment 1

[Lavish Aggarwal]

I would like to work on this bug.
Can you tell me what message should I log and which file I need to modify to resolve this bug?

Comment 2

[Andrew Swan [:aswan]]

The code that handles looking up a native application on windows is here:
http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/toolkit/components/extensions/NativeMessaging.jsm#87-101

For this type of developer-focused message, its fine to call Cu.reportError() with a string embedded directly in the source code.

If you're feeling ambitious, the surrounding code could be converted from Task.jsm and chained promises to use async/await at the same time: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

Comment 3

[Caitlin Neiman [:caitmuenster]]

Hey Lavish! How is it going with this bug?

Comment 4

[graham.mcknight2]

Attachment: Bug 1289821 - Add logging to _winLookup if host manifest is missing

Add logging to _winLookup if host manifest is missing

Comment 5

[graham.mcknight2]

Hi; I'm a student and I would like to take this bug. I submitted a patch, but I still have some concerns:

1. The wording of the error. It seems to me as though the exceptions for nonexistent files do not have a message field of their own, so I added my own "no such file exists"
2. Do we want to test the logging behavior? In the patch I submitted, I added an xpcshell test for this, however, none of the other logging of errors is seemingly tested, and I'm not sure that the technique used is robust.
3. A number of the tests for browser/components/extensions/test/browser/ timed out or failed due to window positioning locally (both with and without the commit in question), so I was not able to verify that this change does not break those tests further, but there do not seem to be any related to native messaging in this directory.
4. Likewise I did not verify that the current behavior on Mac/Linux was maintained, but I can set up a vm for it.

Comment 6

[Andrew Swan [:aswan]]

I'm about to be offline for several days, redirecting while I'm gone.

Comment 7

[Tomislav Jovanovic :zombie]

Hi Graham, thank you for submitting the patch, it looks good, though I left a few comments.

1. The wording of the error. It seems to me as though the exceptions for nonexistent files do not have a message field of their own, so I added my own "no such file exists"

A more specific message might work better, like "Native Manifest file pointed to from registry not found" only perhaps worded better.

2. Do we want to test the logging behavior? In the patch I submitted, I added an xpcshell test for this, however, none of the other logging of errors is seemingly tested, and I'm not sure that the technique used is robust.

Since this bug is specifically about adding a logging message, it makes sense to test that.

3. A number of the tests for browser/components/extensions/test/browser/ timed out or failed due to window positioning locally (both with and without the commit in question), so I was not able to verify that this change does not break those tests further, but there do not seem to be any related to native messaging in this directory.

Yeah, unfortunately some tests are flaky and don't pass on some dev machines :/, please ignore them as they seem totally unrelated to your changes.

4. Likewise I did not verify that the current behavior on Mac/Linux was maintained, but I can set up a vm for it.

That's not needed/expected, but if you want to go for bonus points, you can let your test run on all platforms, and assert the error message is not logged on linux/osx.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e5348e100e4
Add logging to _winLookup if host manifest is missing r=zombie

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3e5348e100e4

Comment 10

[Caitlin Neiman [:caitmuenster]]

Thanks so much for the patch, Graham! 🎉 Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you!

Comment 11

[graham.mcknight2]

Hi Caitlin! I missed this because my phone stopped showing me notifications for emails. I just created a mozillian account, and the link to vouch is here: https://mozillians.org/en-US/u/graham.mcknight2/

This is also a good opportunity for me to say: thanks for the review, Tomislav!

Comment 12

[Caitlin Neiman [:caitmuenster]]

Awesome, thank you, Graham! Your profile has been vouched.

Welcome onboard! We look forward to seeing you around! 😎

Comment 13

[Alex Cornestean]

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!

Comment 14

[graham.mcknight2]

I started, but haven't finished typing instructions to reproduce, so I will finish after work. I'm unsure whether it needs manual verification. Tomislav, can I get you to shed some light on when manual verification is needed?

Comment 15

[Tomislav Jovanovic :zombie]

This landed with a test, and it's not a significant change, so QA doesn't seem necessary. Though if you already tested, feel free to change the status to VERIFIED.