Closed Bug 1459613 Opened 5 years ago Closed 5 years ago

Show WebExtension names instead of IDs in permission doorhangers

Categories

(WebExtensions :: Frontend, defect, P1)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: johannh, Assigned: rpl)

References

Details

Attachments

(3 files)

Screen Shot 2018-05-07 at 15.23.14.png
61.38 KB, image/png
Details
Bug 1459613 - Show extension name in PermissionUI / webrtcUI doorhangers and menus items.
59 bytes, text/x-review-board-request
Details
1459613.png
46.39 KB, image/png
Details 
See screenshot. We shouldn't show these UUIDs, instead just the extension names.
Assignee: nobody → lgreco
Priority: -- → P1
Blocks: 1462345
See Also: → 1462352
Comment on attachment 8976501 [details]
Bug 1459613 - Show extension name in PermissionUI / webrtcUI doorhangers and menus items.

https://reviewboard.mozilla.org/r/244624/#review250718

::: browser/modules/PermissionUI.jsm:118
(Diff revision 1)
> +   * based on the principal URI (the URI.hostPort for any URI scheme
> +   * besides the moz-extension one which should default to the
> +   * extension name).
> +   */
> +  get principalName() {
> +    if (this.principal.URI.schemeIs("moz-extension")) {

No. No, no, no. Never check the protocol to try to figure out if a principal belongs to an extension.

    if (this.principal.addonPolicy) {
      return this.principal.addonPolicy.name;
    }
Comment on attachment 8976501 [details]
Bug 1459613 - Show extension name in PermissionUI / webrtcUI doorhangers and menus items.

https://reviewboard.mozilla.org/r/244624/#review250718

> No. No, no, no. Never check the protocol to try to figure out if a principal belongs to an extension.
> 
>     if (this.principal.addonPolicy) {
>       return this.principal.addonPolicy.name;
>     }

Thanks, that's fair, especially here that the principal was already available. (a single "No" was already enough, though ;-))

Also, in `browser/modules/webrtcUI.jsm` where only the uri is already available, I think that the following pattern could be a good one, right?

const addonPolicy = WebExtensionPolicy.getByURI(uri);
if (addonPolicy) {
  ...
}

We should really write these two patterns down in our in-tree rst docs.
Comment on attachment 8976501 [details]
Bug 1459613 - Show extension name in PermissionUI / webrtcUI doorhangers and menus items.

https://reviewboard.mozilla.org/r/244624/#review250856

I'd like to delegate this to Prathiksha :)

Thanks!
Attachment #8976501 - Flags: review?(jhofmann) → review?(prathikshaprasadsuman)
Product: Toolkit → WebExtensions
Comment on attachment 8976501 [details]
Bug 1459613 - Show extension name in PermissionUI / webrtcUI doorhangers and menus items.

Sorry, I'm gonna have to bounce this one back to Johann. I'm afraid that I don't have a lot of time at the moment to get to this. :)
Attachment #8976501 - Flags: review?(prathikshaprasadsuman) → review?(jhofmann)
Comment on attachment 8976501 [details]
Bug 1459613 - Show extension name in PermissionUI / webrtcUI doorhangers and menus items.

https://reviewboard.mozilla.org/r/244624/#review258176

Makes sense, thank you! Sorry for the delay!

::: browser/base/content/test/popupNotifications/browser_displayURI.js:46
(Diff revision 2)
> +    let extension = ExtensionTestUtils.loadExtension({
> +      manifest: {
> +        name: "Test Extension Name",
> +      },
> +      background() {
> +        const {browser} = this;

nit: please be consistent with const and let, so please use let here

::: browser/base/content/test/popupNotifications/browser_displayURI.js:56
(Diff revision 2)
> +        "extension-tab-page.html": `<!DOCTYPE html><html><body></body></html>`,
> +      },
> +    });
> +
> +    await extension.startup();
> +    const extensionURI = await extension.awaitMessage("extension-tab-url");

nit: let

::: browser/modules/webrtcUI.jsm:310
(Diff revision 2)
>    try {
>      if (!uri) {
>        uri = Services.io.newURI(href);
>      }
> -    host = uri.host;
> +
> +    const addonPolicy = WebExtensionPolicy.getByURI(uri);

nit: let
Attachment #8976501 - Flags: review?(jhofmann) → review+
Attachment #8976501 - Flags: review?(prathikshaprasadsuman)
Comment on attachment 8976501 [details]
Bug 1459613 - Show extension name in PermissionUI / webrtcUI doorhangers and menus items.

https://reviewboard.mozilla.org/r/244622/#review258568

::: browser/modules/PermissionUI.jsm:117
(Diff revisions 3 - 4)
>    get principalName() {
> -    if (this.principal.addonPolicy) {
> -      return this.principal.addonPolicy.name;
> +    let addonPolicy = WebExtensionPolicy.getByURI(this.principal.URI);
> +    if (addonPolicy) {
> +      return addonPolicy.name;
>      }

I applied this additional tweak to the patch to fix the following failure caught on try by running TV (test-verify-e10s) on the windows platforms:

Assertion failure: int32_t(mRefCnt) > 0 (dup release), at z:/build/build/src/toolkit/components/extensions/WebExtensionPolicy.cpp:427

(e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=184172724&repo=try&lineNumber=1259)

It seems to be related to accessing `this.principal.addonPolicy`, and so my guess is that the doorhanger is keeping a reference to the `this.principal` while the test is existing (and `this.principal` is keeping a reference for the `addonPolicy` instance), retrieving the `addonPolicy` using `WebExtensionPolicy.getByURI` doesn't trigger that failure (because `addonPolicy` is not going to referenced anymore after the `principalName` getter returns).
That seems like an awfully easy way to crash. Maybe file a bug about this?
(In reply to Johann Hofmann [:johannh] from comment #11)
> That seems like an awfully easy way to crash. Maybe file a bug about this?

Filed as Bug 1470965.
Depends on: 1470965
The last update contains just a revert of the workaround applied temporarily in https://reviewboard.mozilla.org/r/244624/diff/3-4/ (used to verify that the reason behind the TV failure was really triggered by accessing this.principal.addonPolicy from PermissionUI.jsm like I thought).

Bug 1470965 has been landed today on mozilla-central and the last push to try confirms that the assertion failure is not raised anymore on test-verify-e10s:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c6f901ac28b65b575d9a3f641e9fe51395898f
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/d37a23757d27
Show extension name in PermissionUI / webrtcUI doorhangers and menus items. r=johannh
https://hg.mozilla.org/mozilla-central/rev/d37a23757d27
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Hello, I've looked at all the geo-extensions I could find, can you please provide an extension to test this with?
Flags: needinfo?(lgreco)
Attached image 1459613.png — 
This issue is verified as fixed on Firefox 63.0a1 (20180701220749) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached screenshot.
Flags: needinfo?(lgreco)
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Hi,
could you try this add-on which use navigator.mediaDevices.getUserMedia() to access camera on its Firefox Android popup?
AMO link: https://addons.mozilla.org/firefox/addon/foxauth/
Source code link: https://github.com/FoxAuth/FoxAuth/blob/master/src/scripts/scanQR.js#L9

It still shows me the ID instead of extension name.
Flags: needinfo?(lgreco)
(In reply to lolipopplus from comment #19)
> Hi,
> could you try this add-on which use navigator.mediaDevices.getUserMedia() to
> access camera on its Firefox Android popup?
> AMO link: https://addons.mozilla.org/firefox/addon/foxauth/
> Source code link:
> https://github.com/FoxAuth/FoxAuth/blob/master/src/scripts/scanQR.js#L9
> 
> It still shows me the ID instead of extension name.

That is actually expected, this bug covered the fix for Firefox Desktop, Bug 1462345 is its Fennec counterpart.
Flags: needinfo?(lgreco)
You need to log in before you can comment on or make changes to this bug.