1452361 - Don't reset cookie permission when selecting the default value

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect, P3)

Description

[sworddragon2]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

1. Make a right click at some empty area on this website and select "View Page Info".
2. Go to the register "Permissions".
3. On "Set Cookies" disable the checkbox for "Use Default".
4. Close the "Page Permissions" window and reopen it and check the state of "Set Cookies".


Actual results:

The checkbox for "Use Default" is enabled.


Expected results:

The checkbox for "Use Default" should still be disabled.


Additional information:

At default I do not allow third-party cookies and am usually using the site permissions on the related frame to allow them for embedded third-party sites I want by disabling the "Use Default" checkbox and activating the "Allow" radio button (which is the default) resulting in third-party cookies being allowed for that specific site - but this ability did broke now as third-party cookies can't be allowed anymore for specific sites.

Comment 1

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Build ID 	20180410220129
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

I was able to reproduce this issue on the latest Firefox release (59.0.2) and on the latest Nightly build (61.0a1) on Windows 10 x64, Mac 10.13.3 and Arch Linux.

16:22.07 INFO: Last good revision: 666c3c19d562b2fadece41ed82191fda225ee0e8
16:22.07 INFO: First bad revision: 99b0d3748cdae722454c26912cccdc2fdbb60e44
16:22.07 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=666c3c19d562b2fadece41ed82191fda225ee0e8&tochange=99b0d3748cdae722454c26912cccdc2fdbb60e44

It looks like bug 1379560 caused this.

@johannh, could you please take a look at this?

Comment 2

[Johann Hofmann [:johannh]]

Ah, this one is tricky because in your case "ALLOW" is technically the default (because it's checking the cookieBehavior pref) and not the third party pref...

We can probably work around that somehow...

Comment 3

[Johann Hofmann [:johannh]]

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

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1452361 - Don't reset to default permissions for cookies in page info.

This commit makes the page info window treat cookie permissions a little
differently, to reflect that the "default" value for cookies is a combination
of two prefs that doesn't strictly map onto the ALLOW/SESSION/DENY system of
permissions.

I also added some more general pageinfo permissions tests.

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

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8974389 [details]
Bug 1452361 - Don't reset to default permissions for cookies in page info.

https://reviewboard.mozilla.org/r/242734/#review249472

Thanks! And ++ for writing the tests!

::: browser/base/content/pageinfo/permissions.js:178
(Diff revision 1)
>    }
>  }
>  
>  function onRadioClick(aPartId) {
>    var radioGroup = document.getElementById(aPartId + "RadioGroup");
> -  var id = radioGroup.selectedItem.id;
> +  var id = radioGroup.selectedItem ? radioGroup.selectedItem.id : "#1";

passing comment without requesting any changes: a bit unfortunate that we use this hardcoded id system here

::: browser/base/content/test/pageinfo/browser_pageinfo_permissions.js:59
(Diff revision 1)
> +// Test some standard operations in the permission tab, falling back to a custom
> +// default permission instead of UNKNOWN.
> +add_task(async function test_default_geo_permission() {
> +  await SpecialPowers.pushPrefEnv({set: [["permissions.default.geo", SitePermissions.ALLOW]]});
> +
> +  await BrowserTestUtils.withNewTab(TEST_ORIGIN, async function(browser) {

Is it possible to de-duplicate this by putting it in a function that accepts the default value, and calling it once with UNKNOWN and once with ALLOW?

This doesn't block r+, just wondered if that would make it clearer to future readers that the same test is essentially being run twice with different defaults.

::: browser/modules/SitePermissions.jsm:615
(Diff revision 1)
>    },
>  
>    "cookie": {
>      states: [ SitePermissions.ALLOW, SitePermissions.ALLOW_COOKIES_FOR_SESSION, SitePermissions.BLOCK ],
>      getDefault() {
> -      if (Services.prefs.getIntPref("network.cookie.cookieBehavior") == 2)
> +      if (Services.prefs.getIntPref("network.cookie.cookieBehavior") == Ci.nsICookieService.BEHAVIOR_REJECT)

nice

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8974389 [details]
Bug 1452361 - Don't reset to default permissions for cookies in page info.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbe33baee22c
Don't reset to default permissions for cookies in page info. r=nhnt11

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/dbe33baee22c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem like something we need to consider for ESR60 (though I'll listen to arguments to the contrary), but a Beta uplift request might not be a bad idea.

Comment 10

[Ciprian Muresan [:cmuresan], Ecosystem QA]

I have verified that the issue is no longer reproducible on the latest Nightly build (2018-05-15) on Windows 10 x64, Mac 10.12.6 and Arch Linux x64.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8974389 [details]
Bug 1452361 - Don't reset to default permissions for cookies in page info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1379560
[User impact if declined]: Users can not add exceptions for third party cookie blocking in page info
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Medium
[Why is the change risky/not risky?]: It's a pretty fundamental change that should affect only page info or permissions UI. The patch adds comprehensive tests that should rule out major regressions.
[String changes made/needed]: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8974389 [details]
Bug 1452361 - Don't reset to default permissions for cookies in page info.

Fixes a regression with third party cookies. Thanks for adding a lot of new tests for this functionality. Approved for 61.0b6.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f23b9482597f

Comment 14

[Ciprian Muresan [:cmuresan], Ecosystem QA]

I have verified that the issue is no longer reproducible on the latest Beta build (61.0b6) on Windows 10 x64, Mac 10.13.3 and Ubuntu 16.04 x64.