1350277 - http://*/* and https://*/* permissions display identical descriptions

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Vasilica Tamas, QA [:vtamas]]

Attachment: 2017-03-24_1136.png

[Affected versions]:
Firefox 55.0a1 (2017-03-23)
Firefox 54.0a2 (2017-03-23)
Firefox 53.0b6 (20170323121323)


[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit

[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Create extensions.webextPermissionPrompts and set it to true.
3.Create xpinstall.signatures.dev-root and set it to true.
4.Restart the browser.
5.Click on “+ Add to Firefox” button to install the following webextension: https://addons-dev.allizom.org/en-US/firefox/addon/9gag-mini-permissions/

[Expected Results]:
http://*/* and https://*/* permissions displays together only one description.


[Actual Results]:
- The same description appears twice, one for each permission.
- See attached screenshot.

[Additional Notes]:
- This issue also reproduce for domains specified in "content_scripts" field.
- Here is a screenshot about how Chrome behaves using the same extension: https://www.screencast.com/t/geDlQFFVRj

Comment 1

[:shell escalante]

dev doc is to use * instead of permissions for both http or https

Comment 2

[Andrew Swan [:aswan]]

The fix here is pretty straightforward: use Sets instead of arrays to keep track of hosts and domains inside ExtensionsUI._buildStrings().  ie here: http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/browser/modules/ExtensionsUI.jsm#242

Comment 4

[Andrew Swan [:aswan]]

This also applies to hosts that appear in the "content_scripts" section of the manifest, as reported in bug 1363263

Comment 8

[Andy McKay]

Attachment: Bug 1350277 remove duplicates when showing host permissons

Review commit: https://reviewboard.mozilla.org/r/159904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159904/

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8888879 [details]
Bug 1350277 remove duplicates when showing host permissons

https://reviewboard.mozilla.org/r/159904/#review169054

Nice, thanks, but can you please apply the same logic to wildcard/domain permissions?

Comment 11

[Andy McKay]

Comment on attachment 8888879 [details]
Bug 1350277 remove duplicates when showing host permissons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159904/diff/1-2/

Comment 12

[Andy McKay]

Thanks, updated per feedback to do the wildcard check. The xpi change just removes the signing as thats unneccesary these days and changes the manifest to have a matching http and https single host and wildcard domain.

Comment 16

[Andy McKay]

Comment on attachment 8888879 [details]
Bug 1350277 remove duplicates when showing host permissons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159904/diff/2-3/

Comment 17

[Andy McKay]

Comment on attachment 8888879 [details]
Bug 1350277 remove duplicates when showing host permissons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159904/diff/3-4/

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8888879 [details]
Bug 1350277 remove duplicates when showing host permissons

https://reviewboard.mozilla.org/r/159904/#review184034

::: commit-message-e99e3:1
(Diff revision 4)
> +bug 1350277 ignore protocol when showing host permissons r?aswan

Capitalize Bug and the summary.  Although we already ignore the protocol, this is about weeding out duplciates -- update the summary to describe that more clearly?

Comment 19

[Andy McKay]

Comment on attachment 8888879 [details]
Bug 1350277 remove duplicates when showing host permissons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159904/diff/4-5/

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 21

[Andy McKay]

Done.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b74ec6bb1d8
remove duplicates when showing host permissons r=aswan

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser/base/content/test/webextensions/browser_permissions_addons_search.js:

https://hg.mozilla.org/integration/autoland/rev/9ac8940da5c3bdc417a057c8e424c6655a1c9e88

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2b74ec6bb1d84093e3367e8903aa6d94817cfa00&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=130759170&repo=autoland

12:19:39     INFO -  78 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | Permissions list has 0 entries -
12:19:39     INFO -  79 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | Permissions header is hidden -
12:19:39     INFO -  80 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | Installation was cancelled -
12:19:39     INFO -  81 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | Extension is not installed -
12:19:39     INFO -  82 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | about:addons is displaying search results -
12:19:39     INFO -  83 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | Found browser_webext_permissions.xpi in search results -
12:19:39     INFO -  84 INFO Console message: [JavaScript Error: "Unparseable host permission moz-extension://ae3bb518-f006-cd4f-9386-f015a3a47afd/*" {file: "resource://gre/modules/Extension.jsm" line: 842}]
12:19:39     INFO -  formatPermissionStrings@resource://gre/modules/Extension.jsm:842:9
12:19:39     INFO -  _buildStrings@resource:///modules/ExtensionsUI.jsm:263:12
12:19:39     INFO -  observe@resource:///modules/ExtensionsUI.jsm:169:21
12:19:39     INFO -  installRemote/this.mInstall.promptHandler/<@chrome://mozapps/content/extensions/extensions.xml:679:15
12:19:39     INFO -  installRemote/this.mInstall.promptHandler@chrome://mozapps/content/extensions/extensions.xml:659:51
12:19:39     INFO -  checkPrompt/<@resource://gre/modules/addons/XPIInstall.jsm:1700:17
12:19:39     INFO -  async*checkPrompt@resource://gre/modules/addons/XPIInstall.jsm:1691:6
12:19:39     INFO -  install@resource://gre/modules/addons/XPIInstall.jsm:1444:7
12:19:39     INFO -  install@resource://gre/modules/addons/XPIInstall.jsm:2186:7
12:19:39     INFO -  downloadCompleted/<@resource://gre/modules/addons/XPIInstall.jsm:2502:9
12:19:39     INFO -  makeSafe/<@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:104:17
12:19:39     INFO -  getRepositoryAddon@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:85:5
12:19:39     INFO -  getAddon/<@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:744:9
12:19:39     INFO -  promise callback*getAddon@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:742:12
12:19:39     INFO -  getVisibleAddonForID@resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:791:5
12:19:39     INFO -  downloadCompleted@resource://gre/modules/addons/XPIInstall.jsm:2479:5
12:19:39     INFO -  onStopRequest/<@resource://gre/modules/addons/XPIInstall.jsm:2425:13
12:19:39     INFO -  promise callback*onStopRequest@resource://gre/modules/addons/XPIInstall.jsm:2423:9
12:19:39     INFO -  Buffered messages logged at 12:19:39
12:19:39     INFO -  85 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | addon-webext-permissions notification shown -
12:19:39     INFO -  86 INFO TEST-PASS | browser/base/content/test/webextensions/browser_permissions_addons_search.js | notification panel open -
12:19:39     INFO -  Buffered messages finished
12:19:39    ERROR -  87 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/webextensions/browser_permissions_addons_search.js | Notification icon is correct "chrome://browser/skin/warning.svg" ~= /^jar:file:\/\/.*\/icon\.png$/ -

Comment 24

[Andy McKay]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c13a6dfc46a1236f0e579c7ef2f7df2c4daea1

Comment 25

[Andy McKay]

Comment on attachment 8888879 [details]
Bug 1350277 remove duplicates when showing host permissons

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159904/diff/5-6/

Comment 26

[Andy McKay]

Turns out I do have to sign this add-on, the failures were checking if the extension was signed or not. I've signed the extension and run a try build. Looks good apart from those unrelated oranges.

Comment 27

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s cd4094992fe5 -d 9219c2f933e2: rebasing 419919:cd4094992fe5 "Bug 1350277 remove duplicates when showing host permissons r=aswan" (tip)
merging browser/base/content/test/webextensions/browser_webext_permissions.xpi
merging toolkit/components/extensions/Extension.jsm
warning: /repos/mozreview-gecko/browser/base/content/test/webextensions/browser_webext_permissions.xpi looks like a binary file.
warning: conflicts while merging browser/base/content/test/webextensions/browser_webext_permissions.xpi! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41effc9b8c27
Remove duplicates when showing host permissons. r=aswan

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(In reply to Mozilla Autoland from comment #27)
> We're sorry, Autoland could not rebase your commits for you automatically.
> Please manually rebase your commits and try again.
> 
> hg error in cmd: hg rebase -s cd4094992fe5 -d 9219c2f933e2: rebasing
> 419919:cd4094992fe5 "Bug 1350277 remove duplicates when showing host
> permissons r=aswan" (tip)
> merging
> browser/base/content/test/webextensions/browser_webext_permissions.xpi
> merging toolkit/components/extensions/Extension.jsm
> warning:
> /repos/mozreview-gecko/browser/base/content/test/webextensions/
> browser_webext_permissions.xpi looks like a binary file.
> warning: conflicts while merging
> browser/base/content/test/webextensions/browser_webext_permissions.xpi!
> (edit, then use 'hg resolve --mark')
> unresolved conflicts (see hg resolve, then hg rebase --continue)

I manually imported the commit and was able to apply and push it without issue. Was Autoland trying to rebase a binary file or something here?

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/41effc9b8c27

Comment 31

[Gregory Szorc [:gps]]

Filed bug 1400377 to track the autoland issue.

Comment 32

[Vasilica Tamas, QA [:vtamas]]

Attachment: 2017-09-19_1518.png

I confirm that this issue is fixed on Firefox 57.0a1 (20170918220054) under Windows 10 64-bit and Mac OS X 10.12.3. There are no longer duplicated permissions displayed in webextension installation pop-up. See screenshot.

Comment 34

[Will Bamberg [:wbamberg]]

I'm not sure what developer documentation is needed here, can you elaborate please?

Comment 35

[Andrew Swan [:aswan]]

(In reply to Will Bamberg [:wbamberg] from comment #34)
> I'm not sure what developer documentation is needed here, can you elaborate
> please?

I think there was a suggestion a while back that before this bug was fixed, extension authors could work around this bug by using *://foo.com/ instead of listing both http://foo.com/ and https://foo.com/
I do agree that it doesn't seem like there's much to add now that this is fixed...

Comment 36

[Will Bamberg [:wbamberg]]

Thanks aswan.

For reference: dev-doc-needed is for bugs that deliver a new feature (or fix) that will need documenting. As such we don't look at the bug until it has been marked fixed. So adding dev-doc-needed to ask for documentation of a workaround that won't be needed after that bug is fixed will not have the desired effect. In a situation like this it's better to raise a documentation bug: https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer%20Documentation.