841212 - Cookie exception list becomes empty when the new "First party only" exception type is used for any domain

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Ian Nartowicz]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20130107124423

Steps to reproduce:

1. Make sure you have some cookie exceptions set, visible in the exceptions dialog.
2. Set the new exception type "Allow first party only" for bbc.co.uk (or any domain) from the about: permissions page.
3. Open the cookie exceptions dialog from the options privacy pane.


Actual results:

The exceptions list in the dialog is completely empty.
An exception is thrown in the console, concerning GetStringFromName().


Expected results:

The exceptions list should still show all the existing cookie exceptions.  Ideally the new exception type should also be shown, although there is an existing bug to support this new feature in the exceptions dialog.

The bug is caused because _getCapabilityString in permissions.js does not recognise the new exception type, does not derive a strings name for it, and the call to _bundle.getString() fails.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Ian, thank you for the diagnosis, and I am sorry for missing this case. I will fix it.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

imelven, what bug prefs do you have to notice this bug?

Comment 3

[Ian Melven :imelven]

(In reply to Monica Chew from comment #2)
> imelven, what bug prefs do you have to notice this bug?

I happened to see bug 770691 comment 39 in my bugmail so I checked this bug out :)

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Argh, that's what happens when bugs get too long. Ian N, please feel free to assign bugs caused by my breakages to me, so they don't get lost. That's probably true of everyone.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Sorry for the lack of progress on this, the last couple weeks have not been looking so good and this week is not looking so good either. I am hopeful to do this next week.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Add ALLOW_FIRST_PARTY_ONLY to permission.js

This should have been part of bug 770705, but was missed. Steps to test:
- Change any cookie permission to ALLOW_FIRST_PARTY_ONLY on about:permissions
- Open preferences and switch to privacy tab
- Change history settings to "Use custom settings for history"
- The status list is now populated, where it wasn't before.

This patch isn't quite correct, in that now there are buttons for "block", "allow", and "allow for session", but not "allow first party only" in the exceptions panel. However, the "use custom settings for history" is used less than 1% of the time, and I bet that exceptions lists are used even more infrequently, so it's not worth adding UI for that.

Comment 7

[Ian Nartowicz]

(In reply to Monica Chew from comment #6)
> 
> This patch isn't quite correct, in that now there are buttons for "block",
> "allow", and "allow for session", but not "allow first party only" in the
> exceptions panel. However, the "use custom settings for history" is used
> less than 1% of the time, and I bet that exceptions lists are used even more
> infrequently, so it's not worth adding UI for that.

Bug 699479 asks for some UI related to this, although not for this exact thing.  I thought there was also a bug for the same button on the cookies ask dialog, but I can't find it.

Comment 8

[:Margaret Leibovic]

Comment on attachment 720868 [details] [diff] [review]
Add ALLOW_FIRST_PARTY_ONLY to permission.js

This seems like a good band-aid fix to me. Maybe one day we can consolidate all this permissions UI!

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Yes, we definitely need a unified plan for the permissions UI! Thanks, Margaret.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64b60056b8df

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64b60056b8df

Comment 12

[Francesco Lodolo [:flod]]

Is there a specific reason for choosing a completely different/case style in this new string?

preferences.properties
canAccessFirstParty = Allow first party only

aboutPermissions.dtd
permission.allowFirstPartyOnly = Allow First Party Only

Comment 13

[:Margaret Leibovic]

It looks like that's just following the pattern in each of those files. Perhaps it's because the preferences.properties string is used as a description, while the aboutPermissions.dtd sting is an option in a dropdown menu.

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 720868 [details] [diff] [review]
Add ALLOW_FIRST_PARTY_ONLY to permission.js

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 770705
User impact if declined: broken cookie exception list since FF 18 if ALLOW_FIRST_PARTY_ONLY is set for any domain
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): This should be pretty low risk. It adds a string that was previously throwing an exception as undefined.
String or UUID changes made by this patch: none

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

If this is adding a string then we are breaking string freeze on Beta and that might require this to land on Aurora only. Looping in Axel to confirm if I'm reading this patch right and state if we're best to wait here.

Comment 16

[Axel Hecht [:Pike]]

Yes, the patch does break string freeze. If we can, we should just let this ride the trains.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Axel and Lukas,

Thanks for the input. I think it's ok not to uplift to Beta, but we should uplift to Aurora if that doesn't break anything, so we can fix the regression in 21 instead of 22.

This regression affects only people who use the cookie exception list (<< 1%, based on similar data from http://monica-at-mozilla.blogspot.com/2013/02/writing-for-98.html) and use ALLOW_FIRST_PARTY_ONLY, which has been available since FF 18 but is completely unpublicized.

Thanks,
Monica

Comment 18

[Alex Keybl [:akeybl]]

(In reply to Monica Chew from comment #17)
> This regression affects only people who use the cookie exception list (<<
> 1%, based on similar data from
> http://monica-at-mozilla.blogspot.com/2013/02/writing-for-98.html) and use
> ALLOW_FIRST_PARTY_ONLY, which has been available since FF 18 but is
> completely unpublicized.

If a required step to reproduce is to use an unpublicized feature, let's not break l10n string freeze on Aurora for this.