Closed Bug 1289821 Opened 3 years ago Closed 5 months ago

Add logging if native messaging host manifest is missing on windows

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: aswan, Assigned: graham.mcknight2, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [native messaging]triaged)

Attachments

(1 file)

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.
Keywords: good-first-bug
Whiteboard: [native messaging] [good first bug] triaged → [native messaging]triaged
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?
Flags: needinfo?(amckay)
Flags: needinfo?(amckay) → needinfo?(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
Flags: needinfo?(aswan)
Hey Lavish! How is it going with this bug?
Product: Toolkit → WebExtensions

Add logging to _winLookup if host manifest is missing

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

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

Flags: needinfo?(aswan) → needinfo?(tomica)

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.

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

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

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

Flags: needinfo?(tomica)
Assignee: nobody → graham.mcknight2
Mentor: aswan → tomica
Keywords: checkin-needed

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e5348e100e4
Add logging to _winLookup if host manifest is missing r=zombie

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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!

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!

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

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

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!

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

Flags: needinfo?(graham.mcknight2) → needinfo?(tomica)

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.

Flags: needinfo?(tomica) → qe-verify-
You need to log in before you can comment on or make changes to this bug.