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)
WebExtensions
Frontend
Tracking
(firefox63 verified)
VERIFIED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | verified |
People
(Reporter: johannh, Assigned: rpl)
References
Details
Attachments
(3 files)
See screenshot. We shouldn't show these UUIDs, instead just the extension names.
Updated•5 years ago
|
Assignee: nobody → lgreco
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment 2•5 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 4•5 years ago
|
||
mozreview-review-reply |
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.
Reporter | ||
Comment 5•5 years ago
|
||
mozreview-review |
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!
Reporter | ||
Updated•5 years ago
|
Attachment #8976501 -
Flags: review?(jhofmann) → review?(prathikshaprasadsuman)
Updated•5 years ago
|
Product: Toolkit → WebExtensions
Comment 6•5 years ago
|
||
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)
Reporter | ||
Comment 7•5 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•5 years ago
|
Attachment #8976501 -
Flags: review?(prathikshaprasadsuman)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•5 years ago
|
||
mozreview-review |
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).
Reporter | ||
Comment 11•5 years ago
|
||
That seems like an awfully easy way to crash. Maybe file a bug about this?
Assignee | ||
Comment 12•5 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•5 years ago
|
||
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
Comment 15•5 years ago
|
||
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
Comment 16•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d37a23757d27
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 17•5 years ago
|
||
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)
Comment 18•5 years ago
|
||
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
Comment 19•5 years ago
|
||
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)
Assignee | ||
Comment 20•5 years ago
|
||
(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.
Description
•