Page Permissions are no longer sorted alphabetically

VERIFIED FIXED in Firefox 52

Status

()

defect
P5
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Tonnes, Assigned: matrixisreal, Mentored)

Tracking

({good-first-bug, regression})

50 Branch
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50 wontfix, firefox51 wontfix, firefox52 verified, firefox53 verified)

Details

(Whiteboard: [fxprivacy] [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

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. (?)
Keywords: regression
Whiteboard: [fxprivacy] [triage]
Priority: -- → P5
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Decision was that we're ok shipping with this but would happily take a patch.
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.
hey, i'd like to work on this, can you assign me please?
Assignee: nobody → julia.friesel
Mentor: jhofmann
Keywords: good-first-bug
Whiteboard: [fxprivacy] → [fxprivacy] [good first bug]
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 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.
Attachment #8805773 - Flags: review?(jhofmann) → review+
(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.
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?
Flags: needinfo?(jhofmann)
(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. :)
Flags: needinfo?(jhofmann) → needinfo?(dao+bmo)
(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.
> 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!
Flags: needinfo?(julia.friesel)
Hi there, I'm sorry but I won't have time any time soon... Could someone else do this one?
No worries, we'll sort it out. Thanks for letting me know :)
Assignee: julia.friesel → nobody
Flags: needinfo?(julia.friesel)
Hi, I would like to work on this would you assign it to me.
Hi there, sorry for the delay! Please let me know if you need any help :)
Assignee: nobody → pavankarthikboddeda
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
Attachment #8817480 - Flags: review?(jhofmann)
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.
Attachment #8817480 - Flags: review?(jhofmann) → review?(gijskruitbosch+bugs)
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!
Attachment #8817480 - Flags: review?(gijskruitbosch+bugs)
Mentor: gijskruitbosch+bugs
   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 :)
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(pavankarthikboddeda)
Posted patch v1.patch (obsolete) — Splinter Review
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
And then I am opening the page info Dialog, Yes.
Attachment #8817480 - Attachment is obsolete: true
(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?
Flags: needinfo?(pavankarthikboddeda)
Flags: needinfo?(dao+bmo)
Needinfo for comment #31.
Flags: needinfo?(pavankarthikboddeda)
(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
Posted patch v1.1.patchSplinter Review
Is this good to checked in now?
Attachment #8819389 - Attachment is obsolete: true
Attachment #8819669 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8819669 [details] [diff] [review]
v1.1.patch

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

Yup, looks good to me!
Attachment #8819669 - Flags: review?(gijskruitbosch+bugs) → review+
Flags: needinfo?(pavankarthikboddeda)
Keywords: checkin-needed
Attachment #8805773 - Attachment is obsolete: true
Thanks Gijs :D
Any Good Bug to work on next?
(In reply to Pavan Karthik from comment #36)
> Thanks Gijs :D
> Any Good Bug to work on next?

No problem - how about bug 1235547?
(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 :)
Thanks for getting this done! You could also try bug 1269350. :)
Status: NEW → ASSIGNED
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0103a22afe98
Sort page permissions list alphabetically. r=Gijs
Keywords: checkin-needed
I'll change the status to Fixed!? :D
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
(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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/0103a22afe98
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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.
Flags: needinfo?(pavankarthikboddeda)
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
Attachment #8819669 - Flags: approval-mozilla-beta?
Attachment #8819669 - Flags: approval-mozilla-aurora?
Flags: needinfo?(pavankarthikboddeda)
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.
Attachment #8819669 - Flags: approval-mozilla-beta?
Attachment #8819669 - Flags: approval-mozilla-beta-
Attachment #8819669 - Flags: approval-mozilla-aurora?
Attachment #8819669 - Flags: approval-mozilla-aurora+
This patch fails to apply for me on aurora. Can you check what is up with that, please?
Flags: needinfo?(pavankarthikboddeda)
Flags: needinfo?(pavankarthikboddeda)
(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?
Nothing to do for you, Pavan. Ryan applied this patch to aurora.
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.