Closed Bug 1459613 Opened 7 years ago Closed 7 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: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: