1299967 - Page Permissions are no longer sorted alphabetically

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P5)

Description

[Ton [:Tonnes]]

With the exception of Activate Plugins always listed on top (for some reason - I was told this would probably be intentional), Page permissions are no longer sorted alphabetically starting in FF 50 aurora, making it harder to find / easier to overlook a proper permission. This also occurs for current nightly (51), and both for en-US and other locales.

It would be nice to see page permissions sorted alphabetically again.

Note that https://support.mozilla.org/en-US/kb/page-info-window-view-technical-details-about-page also lists these permissions sorted as such, and locales may do their best to do similar for localized articles. When left as is, unsorting them there may need to follow, or worst case: the order of the items may be unpredictable. (?)

Comment 1

[Andrew Overholt [:overholt]]

Decision was that we're ok shipping with this but would happily take a patch.

Comment 2

[Ton [:Tonnes]]

If coding was my cup of tea, I’d be glad to help out. It would probably be easiest if the one responsible for the regression fixes it.

Comment 3

[Julia Friesel[:jfr]]

hey, i'd like to work on this, can you assign me please?

Comment 4

[Ton [:Tonnes]]

Thank you Julia.

Testing some builds, it appears the issue started on 2016-06-24. If I see it right, https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2016-06-23&enddate=2016-06-25 may indicate bug 1280485 to be the cause. An intentional and/or unavoidable change?

Comment 5

[Julia Friesel[:jfr]]

Attachment: Bug 1299967 - Sort permission list in SitePermissions alphabetically.

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

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8805773 [details]
Bug 1299967 - Sort permission list in SitePermissions alphabetically.

https://reviewboard.mozilla.org/r/89444/#review88686

Looks good, please fix the nits.

::: browser/modules/SitePermissions.jsm:97
(Diff revision 1)
>     */
>    isSupportedURI: function (aURI) {
>      return aURI.schemeIs("http") || aURI.schemeIs("https");
>    },
>  
> -  /* Returns an array of all permission IDs.
> +  /* Returns an array of all permission IDs sorted aphabetically by label for display in the UI.

typo: alphabetically

::: browser/modules/SitePermissions.jsm:100
(Diff revision 1)
>    },
>  
> -  /* Returns an array of all permission IDs.
> +  /* Returns an array of all permission IDs sorted aphabetically by label for display in the UI.
>     */
>    listPermissions: function () {
> -    return kPermissionIDs;
> +    return kPermissionIDs.sort((a,b) => {

Nit: please add a space after the comma

::: browser/modules/SitePermissions.jsm:101
(Diff revision 1)
>  
> -  /* Returns an array of all permission IDs.
> +  /* Returns an array of all permission IDs sorted aphabetically by label for display in the UI.
>     */
>    listPermissions: function () {
> -    return kPermissionIDs;
> +    return kPermissionIDs.sort((a,b) => {
> +        return this.getPermissionLabel(a).localeCompare(this.getPermissionLabel(b));

Nit: I think these are 4 spaces, can you confirm? We prefer 2 spaces indentation.

Comment 7

[Julia Friesel[:jfr]]

(In reply to Ton from comment #4)
> Thank you Julia.
> 
> Testing some builds, it appears the issue started on 2016-06-24. If I see it
> right,
> https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2016-06-
> 23&enddate=2016-06-25 may indicate bug 1280485 to be the cause. An
> intentional and/or unavoidable change?

Thanks, for the hint Ton, I was able to discuss it with the people responsible in person. About to fix it.

Comment 8

[Julia Friesel[:jfr]]

Comment on attachment 8805773 [details]
Bug 1299967 - Sort permission list in SitePermissions alphabetically.

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

Comment 9

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48f88f5d27fb

Comment 10

[Julia Friesel[:jfr]]

Comment on attachment 8805773 [details]
Bug 1299967 - Sort permission list in SitePermissions alphabetically.

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

Comment 11

[Julia Friesel[:jfr]]

Comment on attachment 8805773 [details]
Bug 1299967 - Sort permission list in SitePermissions alphabetically.

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

Comment 12

[Julia Friesel[:jfr]]

Comment on attachment 8805773 [details]
Bug 1299967 - Sort permission list in SitePermissions alphabetically.

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

Comment 13

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d15f5e3939a

Comment 14

[:Gijs (he/him)]

The patch as-is will break sorting if the locale changes.

Note also that the patch as-is depends on the locale being correct by the time SitePermissions.jsm is loaded. This might not be the case if the module loads early, in Linux distro builds that use language packs (see bug 1005640 for more context about some of the issues surrounding this). Which basically means that even if this works today, it becomes a boobytrap because it could break in the future if the jsm is loaded earlier for some reason.

And maybe all of that is OK because it's not substantially worse than the pre-patch behaviour - but it would be a good idea for this bug to be explicit about the decisions that we're making. Johann, can you clarify?

Comment 15

[Johann Hofmann [:johannh]]

(In reply to :Gijs Kruitbosch from comment #14)
> The patch as-is will break sorting if the locale changes.

The _intention_ of this patch is to always sort the permissions based on the locale names. Or do you mean the sorting will be incorrect once the user changes the locale?

> Note also that the patch as-is depends on the locale being correct by the
> time SitePermissions.jsm is loaded. This might not be the case if the module
> loads early, in Linux distro builds that use language packs (see bug 1005640
> for more context about some of the issues surrounding this). Which basically
> means that even if this works today, it becomes a boobytrap because it could
> break in the future if the jsm is loaded earlier for some reason.

Yes, these are good points and I think that the sorting should be done in either listPermissions or on this line in the pageinfo code: http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/browser/base/content/pageinfo/permissions.js#14.

I strongly prefer the latter because it will prevent other people from wasting resources on fetching locale strings and sorting when calling listPermissions without the intention to sort. I already discussed this with Dao and we ended up with the current solution, so I'll loop him in.

Julia, sorry for the holdup. We should be able to resolve this quickly. :)

Comment 16

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] - partially unresponsive until 11/14 from comment #15)
> (In reply to :Gijs Kruitbosch from comment #14)
> > The patch as-is will break sorting if the locale changes.
> 
> The _intention_ of this patch is to always sort the permissions based on the
> locale names. Or do you mean the sorting will be incorrect once the user
> changes the locale?

Yes.

> > Note also that the patch as-is depends on the locale being correct by the
> > time SitePermissions.jsm is loaded. This might not be the case if the module
> > loads early, in Linux distro builds that use language packs (see bug 1005640
> > for more context about some of the issues surrounding this). Which basically
> > means that even if this works today, it becomes a boobytrap because it could
> > break in the future if the jsm is loaded earlier for some reason.
> 
> Yes, these are good points and I think that the sorting should be done in
> either listPermissions

This looks like it also runs when the module is loaded?

> or on this line in the pageinfo code:
> http://searchfox.org/mozilla-central/rev/
> 445097654c1cc1098ee0171a29c439afe363a588/browser/base/content/pageinfo/
> permissions.js#14.

Do we reuse this code in the permissions / site identity panel? The name suggests this is page info dialog code.

Comment 17

[Johann Hofmann [:johannh]]

> Do we reuse this code in the permissions / site identity panel? The name suggests this is page info dialog code.

That code is indeed page info dialog code. Some time ago we decided that permissions don't need to be sorted in the control center since it's very unlikely that more than 2 or 3 will ever be shown to the user. Given the circumstances you described and lack of better ideas so far I think it's sensible to just sort the array in the page info code.

Julia, do you have time to get back to this and sort the array in pageinfo/permissions.js[0] as we initially did?

[0] http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/browser/base/content/pageinfo/permissions.js#14

Thanks!

Comment 18

[Julia Friesel[:jfr]]

Hi there, I'm sorry but I won't have time any time soon... Could someone else do this one?

Comment 19

[Johann Hofmann [:johannh]]

No worries, we'll sort it out. Thanks for letting me know :)

Comment 20

[Pavan Karthik [:matrixisreal]]

Hi, I would like to work on this would you assign it to me.

Comment 21

[Johann Hofmann [:johannh]]

Hi there, sorry for the delay! Please let me know if you need any help :)

Comment 22

[Pavan Karthik [:matrixisreal]]

Hi,
I have few doubts
1. In this line http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/browser/base/content/pageinfo/permissions.js#14 should I sort them after we push the "plugins" ID or before that.
2. Can you suggest, how to debug the huge mozilla code(examining variables etc).
Thanks :D

Comment 23

[Pavan Karthik [:matrixisreal]]

Attachment: permissionIDs sorted alphabetically by Label.

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8817480 [details] [diff] [review]
permissionIDs sorted alphabetically by Label.

Review of attachment 8817480 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the review delay. I'm on vacation right now. Maybe Gijs can take this over?

::: browser/base/content/pageinfo/permissions.js
@@ +10,5 @@
>  var gPermURI;
>  var gPermPrincipal;
>  var gUsageRequest;
>  
> +//Array of permissionIDs sorted alphabetically by Label.

nit: put a space before "Array", also I think "Label" should be lowercase

@@ +11,5 @@
>  var gPermPrincipal;
>  var gUsageRequest;
>  
> +//Array of permissionIDs sorted alphabetically by Label.
> +var gPermissions = SitePermissions.listPermissions().sort((a, b) => {

There's a problem here, in that we get, sort and push to the original permission list. This changes what SitePermissions.listPermissions returns to any other caller.

Independent of what we do here, I think it's a bad idea to have SitePermissions.listPermissions return a reference to the original list. (See http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/browser/modules/SitePermissions.jsm#100 and http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/browser/modules/SitePermissions.jsm#269 )

We should probably modify it to return `Object.keys(gPermissionObject);` instead of `kPermissionIDs` directly.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8817480 [details] [diff] [review]
permissionIDs sorted alphabetically by Label.

Review of attachment 8817480 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Johann's existing review comments. :-)

(In reply to Pavan Karthik from comment #22)
> 2. Can you suggest, how to debug the huge mozilla code(examining variables
> etc).
> Thanks :D

You can use the browser toolbox ( https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox ), or use Cu.reportError("some string") to get stuff to show up in the browser console. Let me know if you need more help!

Comment 26

[Pavan Karthik [:matrixisreal]]

   listPermissions: function() {
-    return kPermissionIDs;
+    return Object.keys(gPermissionObject);
   },
 

-var gPermissions = SitePermissions.listPermissions();
+// Array of permissionIDs sorted alphabetically by label.
+var gPermissions = SitePermissions.listPermissions().sort((a, b) => {
+  let firstLabel = SitePermissions.getPermissionLabel(a);
+  let secondLabel = SitePermissions.getPermissionLabel(b);
+  return firstLabel.localeCompare(secondLabel);
+});


Hi, I have made the above changes.
But after the changes,

- the permission labels are sorted just for the first time since the browser is opened.
                      
- then every other time I get the following error:
"TypeError: gPermissionObject[aPermissionID] is undefined "     
inside SitePermissions.jsm:169:9 due to the function SitePermissions.getPermissionLabel()

I couldnt figure out the reason,I could use your help.
Thanks :)

Comment 27

[:Gijs (he/him)]

(In reply to Pavan Karthik from comment #26)
> But after the changes,
> 
> - the permission labels are sorted just for the first time since the browser
> is opened.
>                       
> - then every other time I get the following error:
> "TypeError: gPermissionObject[aPermissionID] is undefined "     
> inside SitePermissions.jsm:169:9 due to the function
> SitePermissions.getPermissionLabel()
> 
> I couldnt figure out the reason,I could use your help.
> Thanks :)

I couldn't reproduce this problem locally. Can you attach a new patch with the changes you're making? What specifically are you doing that triggers the error - are you opening the page info dialog? Something else?

Comment 28

[Pavan Karthik [:matrixisreal]]

Attachment: v1.patch

Comment 29

[Pavan Karthik [:matrixisreal]]

Hi,

(In reply to :Gijs Kruitbosch from comment #27)

> I couldn't reproduce this problem locally. Can you attach a new patch with
> the changes you're making?

   v1.patch has the changes i have made.

> What specifically are you doing that triggers the
> error - are you opening the page info dialog? Something else?
  
   I have set a breaking point at the function onLoadPermission(uri, principal) in permission.js:32

Comment 30

[Pavan Karthik [:matrixisreal]]

And then I am opening the page info Dialog, Yes.

Comment 31

[:Gijs (he/him)]

(In reply to Pavan Karthik from comment #29)
> Hi,
> 
> (In reply to :Gijs Kruitbosch from comment #27)
> 
> > I couldn't reproduce this problem locally. Can you attach a new patch with
> > the changes you're making?
> 
>    v1.patch has the changes i have made.
> 
> > What specifically are you doing that triggers the
> > error - are you opening the page info dialog? Something else?
>   
>    I have set a breaking point at the function onLoadPermission(uri,
> principal) in permission.js:32

I still can't reproduce. I suspect that for some reason (maybe an add-on in your Firefox profile?) we end up looking for a permission that is not present in the object. Can you add:

Cu.reportError(aPermissionID);

in the getPermissionLabel function in SitePermissions.jsm. That will log each permission ID we're looking up a label for to the browser console. Can you use that to figure out which permission is breaking things here, ie for which we're failing to find a label? Maybe you need to run

 ./mach build faster 

to update some localization files or something?

Comment 32

[:Gijs (he/him)]

Needinfo for comment #31.

Comment 33

[Pavan Karthik [:matrixisreal]]

(In reply to :Gijs Kruitbosch from comment #31)

Hi, after running 
./mach build faster , its working fine.
Btw, what does faster do and what do you think have got wrong(I havent fiddled with pageinfos before).
Thanks

Comment 34

[Pavan Karthik [:matrixisreal]]

Attachment: v1.1.patch

Is this good to checked in now?

Comment 35

[:Gijs (he/him)]

Comment on attachment 8819669 [details] [diff] [review]
v1.1.patch

Review of attachment 8819669 [details] [diff] [review]:
-----------------------------------------------------------------

Yup, looks good to me!

Comment 36

[Pavan Karthik [:matrixisreal]]

Thanks Gijs :D
Any Good Bug to work on next?

Comment 37

[:Gijs (he/him)]

(In reply to Pavan Karthik from comment #36)
> Thanks Gijs :D
> Any Good Bug to work on next?

No problem - how about bug 1235547?

Comment 38

[Pavan Karthik [:matrixisreal]]

(In reply to :Gijs Kruitbosch from comment #37)

> No problem - how about bug 1235547?

Looks good,I'll start working on it asap,
How about you suggest me one or two other bugs too, i have got a lot of free time now.:P
Thanks :)

Comment 39

[Johann Hofmann [:johannh]]

Thanks for getting this done! You could also try bug 1269350. :)

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0103a22afe98
Sort page permissions list alphabetically. r=Gijs

Comment 41

[Pavan Karthik [:matrixisreal]]

I'll change the status to Fixed!? :D

Comment 42

[:Gijs (he/him)]

(In reply to Pavan Karthik from comment #41)
> I'll change the status to Fixed!? :D

No, it'll get marked as fixed automatically when this successfully merges to mozilla-central (after tests pass, and so on).

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0103a22afe98

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this patch when you get a chance. You can do so by clicking the Details link next to your attachment, changing the approval-mozilla-aurora (and beta) dropdown to "?", and answering the questions that appear in the comment field.

Comment 45

[Pavan Karthik [:matrixisreal]]

Comment on attachment 8819669 [details] [diff] [review]
v1.1.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1299967
[User impact if declined]:
Permissions become more difficult to manually search for.
[Is this code covered by automated tests?]:
No,I guess
[Has the fix been verified in Nightly?]:
Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
No
[List of other uplifts needed for the feature/fix]:
Nothing
[Is the change risky?]:
No
[Why is the change risky/not risky?]:
Permissions which are being displayed in an instance are just sorted.
[String changes made/needed]:
Nothing

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8819669 [details] [diff] [review]
v1.1.patch

Let's uplift this to aurora for 52. I'd like to leave it out of beta 51 though since we are getting late in the beta cycle.

Comment 47

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

This patch fails to apply for me on aurora. Can you check what is up with that, please?

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6f6cf418098

Comment 49

[Pavan Karthik [:matrixisreal]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #47)
> This patch fails to apply for me on aurora. Can you check what is up with
> that, please?

Hi Aryx,
Do I still need to check with this? i see that the patch is landed.
If yes what do I need to do exactly?

Comment 50

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

Nothing to do for you, Pavan. Ryan applied this patch to aurora.

Comment 51

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 51.0a1 (2016-09-01) on Windows 7 , 64 Bit!

This bug's fix is verified with latest Developer Edition(Aurora) and latest Nightly!


Build   ID  20170118004007
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0

Build   ID  20170118030214
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
[bugday-20170118]