Don't reset cookie permission when selecting the default value

VERIFIED FIXED in Firefox 61

Status

()

defect
P3
normal
VERIFIED FIXED
Last year
Last year

People

(Reporter: sworddragon2, Assigned: johannh)

Tracking

({regression})

59 Branch
Firefox 62
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 verified, firefox62 verified)

Details

Attachments

(1 attachment)

Reporter

Description

Last year
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.
Reporter

Updated

Last year
Version: 52 Branch → 59 Branch
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?
Blocks: 1379560
Status: UNCONFIRMED → NEW
Component: Untriaged → Permission Manager
Ever confirmed: true
Flags: needinfo?(jhofmann)
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Assignee

Comment 2

Last year
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...
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Component: Permission Manager → Page Info Window
Flags: needinfo?(jhofmann)
Priority: -- → P3
Product: Core → Firefox
Assignee

Updated

Last year
Summary: Page permissions aren't stored anymore → Don't reset cookie permission when selecting the default value
Comment hidden (mozreview-request)

Comment 5

Last year
mozreview-review
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
Attachment #8974389 - Flags: review?(nhnt11) → review+
Comment hidden (mozreview-request)

Comment 7

Last year
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbe33baee22c
Don't reset to default permissions for cookies in page info. r=nhnt11

Comment 8

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbe33baee22c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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.
Flags: qe-verify+
Flags: needinfo?(jhofmann)
Flags: in-testsuite+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Assignee

Comment 11

Last year
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
Flags: needinfo?(jhofmann)
Attachment #8974389 - Flags: approval-mozilla-beta?
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.
Attachment #8974389 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.