Closed Bug 1354229 Opened 7 years ago Closed 7 years ago

Services.cookies.usePrivateMode state and OriginAttributes.privateBrowsingId are not necessarily in sync

Categories

(Core :: Networking: Cookies, defect)

Product:
Core
Component:
Networking: Cookies
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: robwu, Assigned: robwu)

References

Details

(Whiteboard: [necko-next])

Attachments

(3 files)

Bug 1354229 - use private cookie DB in nsICookieManager depending on OriginAttributes
59 bytes, text/x-review-board-request
Details
Bug 1354229 - Re-enable cookie tests in private windows
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1354229 - Disable private parts of test_ext_cookies.html on Android
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
Bug 777620 introduced nsCookieService::UsePrivateMode to switch between the default cookie DB to the private cookie DB.
Bug 1267910 added OriginAttributes to nsICookieManager2/nsCookieService: cookies were not stored in a hashmap by domain only, but also by OriginAttributes (domain+OriginAttributes).

One of the OriginAttributes is the privateBrowsing flag, which defaults to 0 (default browsing mode).


This is causing issues in the following methods:

- Services.cookies.countCookiesFromHost (nsCookieService::CountCookiesFromHost)
  This method only takes a domain name and creates a key for hashmap-lookup with a default-initialized OriginAttributes. Consequently, privateBrowsing is alway 0 and the method will always return 0 even if UsePrivateMode is used.

  This also applies to other users of InitializeOriginAttributes, such as Services.cookies.add, etc.

- Services.cookies.getCookiesFromHost (nsCookieService::getCookiesFromHost)
  Services.cookies.getCookiesWithOriginAttributes (nsCookieService::GetCookiesWithOriginAttributes)
  Services.cookies.removeCookiesWithOriginAttributes (nsCookieService::RemoveCookiesWithOriginAttributes)
  These methods take an OriginAttributes type as parameter, but do not check whether the privateBrowsingId in the attributes match with the UsePrivateMode state.

- It is possible to create a cookie in the private browsing store with privateBrowsing flag set to 0 (this makes no sense, see e.g. bug 1318948).



To fix this, I propose the following solution:
1) In cases where no explicit OriginAttributes parameter is accepted by cookie methods, set the privateBrowsing flag of the initialized OriginAttributes to the state as set by UserPrivateMode

2) In cases where an explicit OriginAttributes is passed in, there are multiple options:
   - Overwrite privateBrowsing of the OriginAttributes based on the UsePrivateMode state.
   - Throw an error if the privateBrowsing flag of OriginAttributes does not match with the PrivateBrowsingState.
Tim, can you take a look at this bug? Are you in a good position to fix this?
Flags: needinfo?(tihuang)
I forgot to put a test case in the initial report.
And by explicitly spelling out the 

Those are the steps to reproduce the bug:

1. Enable Chrome debugging (e.g. via the checkbox at about:debugging)
2. Open a private browsing window and visit example.com
3. Open the console in the tab, and run document.cookie = 'foo=bar; max-age=3600';
4. Open the global JS console (Ctrl/Cmd-Shift-J), and run the following snippet:

Services.cookies.usePrivateMode(true, () => {
    console.log(Services.cookies.countCookiesFromHost('example.com'));
    // Expected: 1. Actual: 0.

    console.log(Services.cookies.getCookiesFromHost('example.com',{}).getNext().QueryInterface(Ci.nsICookie2).name);
    // Expected: "foo". Actual: error (because the enumerator has no results)
});

(tested in Firefox 54).
I think nsCookieService::UsePrivateMode and mDBState should be removed completely, and all cookie manager methods should be updated to take a required origin attributes parameter.
(In reply to Jesper Kristensen from comment #3)
> I think nsCookieService::UsePrivateMode and mDBState should be removed
> completely, and all cookie manager methods should be updated to take a
> required origin attributes parameter.

I agree with that we no longer need nsCookieService::UsePrivateMode anymore. On the one hand, there is no one using this API except WebExteinsion, this takes addons into account. And I think WebExtension can move to use 'privateBrowsingId' instead. On the other hand, this API seems redundant that originAttributes can do all the jobs. Therefore, I think we should remove nsCookieService::UsePrivateMode and update Services.cookies.countCookiesFromHost to use originAttributes.

ni baku to see his opinions.
Assignee: nobody → tihuang
Flags: needinfo?(tihuang) → needinfo?(amarchesini)
Whiteboard: [necko-next]
I agree with you. No addons are using this method and we can easily change webExtension code.
Flags: needinfo?(amarchesini)
Tim, if you don't mind I'll take over the bug to make sure that the bug gets fixed.

I've submitted a patch that uses the OriginAttributes to find the right DBState removes the usePrivateMode API.

I've also looked into removing mDBState (as suggested by Jesper in comment 3), but decided against removing it because the refactoring is complicated (involving adding an explicit DBState pointer to most methods in nsCookieService). I'm not confident that doing so would not cause the use of stale pointers because some of the methods can trigger external observers which in their turn could invalidate the DBState pointers.

:jduell
I added you as a reviewer because you approved the original patch in bug 777620 that is superseded by my patch.

:mixedpuppy
I need a r+ from a WebExtension peer. I've chosen you since Kris appears to be busy, and you seems to know some background based on your comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1318948#c6
Assignee: tihuang → rob
Comment on attachment 8876506 [details]
Bug 1354229 - Re-enable cookie tests in private windows

https://reviewboard.mozilla.org/r/147840/#review152602

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:125
(Diff revision 2)
>      browser.test.assertEq(1, stores.length, "expected number of stores returned");
>      browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
>      browser.test.assertEq(1, stores[0].tabIds.length, "one tabId returned for store");
>      browser.test.assertEq("number", typeof stores[0].tabIds[0], "tabId is a number");
>  
> -    // This part causes intermittent failures see Bug 1309637.
> +    {

yuck, do braces pass eslint like this?  if so, fine.
Attachment #8876506 - Flags: review?(mixedpuppy) → review+
Attachment #8876702 - Flags: review?(mixedpuppy)
Comment on attachment 8876505 [details]
Bug 1354229 - use private cookie DB in nsICookieManager depending on OriginAttributes

jdm wrote the private browsing cookie stuff, so is probably a better reviewer for this.  (feel free to punt back to me as needed)
Attachment #8876505 - Flags: review?(jduell.mcbugs) → review?(josh)
Comment on attachment 8876702 [details]
Bug 1354229 - Disable private parts of test_ext_cookies.html on Android

https://reviewboard.mozilla.org/r/148038/#review152626

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:126
(Diff revision 1)
>      browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
>      browser.test.assertEq(1, stores[0].tabIds.length, "one tabId returned for store");
>      browser.test.assertEq("number", typeof stores[0].tabIds[0], "tabId is a number");
>  
> -    {
> +    // Opening private windows/tabs is not supported on Android - bug 1372178
> +    if (browser.windows) {

AppConstants.platform !== "android"

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:218
(Diff revision 1)
>      browser.test.assertEq("", cookie.name, "default name set");
>      browser.test.assertEq("", cookie.value, "default value set");
>      browser.test.assertEq(true, cookie.session, "no expiry date created session cookie");
>  
> -    {
> +    // Opening private windows/tabs is not supported on Android - bug 1372178
> +    if (browser.windows) {

dito
Attachment #8876702 - Flags: review?(mixedpuppy) → review-
Why two patches to change a couple lines from if (false) to if (android)?  Can you consolidate those?
Flags: needinfo?(rob)
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> Why two patches to change a couple lines from if (false) to if (android)? 
> Can you consolidate those?

I separated the second and third patch because their purposes are different.
The second patch adds tests to verify the functionality from the first patch (and part of adding the tests is re-enabling tests that were previously disabled).

Then I ran the tests on the try server, and observed failures on Android, because browser.windows is not defined.
If browser.windows is ever added (in the future?), then the test should run. Therefore I used feature detection instead of OS detection.
If someone uses git blame to figure out why the test is disabled, then they don't have to look at the big diff that added the tests, because that would be misleading. Hence the separate third patch.
Flags: needinfo?(rob)
It's not that big of a patch to do two, and would have explained the braces I commented about in the first.
Comment on attachment 8876702 [details]
Bug 1354229 - Disable private parts of test_ext_cookies.html on Android

https://reviewboard.mozilla.org/r/148038/#review152660
Attachment #8876702 - Flags: review- → review+
Comment on attachment 8876505 [details]
Bug 1354229 - use private cookie DB in nsICookieManager depending on OriginAttributes

https://reviewboard.mozilla.org/r/147838/#review155278

Thank you for doing this. It's good that these APIs actually behave correctly now.
Attachment #8876505 - Flags: review?(josh) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0d238229ffc4 -d 129dd7f06a24: rebasing 402634:0d238229ffc4 "Bug 1354229 - use private cookie DB in nsICookieManager depending on OriginAttributes r=jdm"
merging netwerk/cookie/nsCookieService.cpp
merging toolkit/components/extensions/ext-cookies.js
warning: conflicts while merging netwerk/cookie/nsCookieService.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Attachment #8876505 - Flags: review?(jduell.mcbugs)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/114095814191
use private cookie DB in nsICookieManager depending on OriginAttributes r=jdm
https://hg.mozilla.org/integration/autoland/rev/5707a502bb84
Re-enable cookie tests in private windows r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/a1b87937f170
Disable private parts of test_ext_cookies.html on Android r=mixedpuppy
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: