1139953 - Accept cookie dialog shown in private window when e10s enabled

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Steven MacLeod [:smacleod]]

STR:
 - Enable e10s
 - Set network.cookie.lifetimePolicy=1
 - Open private browsing window
 - Load a page which sets a cookie[1]

Result: The accept cookie dialog[2] is shown
Expected result: The dialog should not be shown


I discovered this while trying to get privatebrowsing tests to run in e10s. The expected functionality is tested by browser_privatebrowsing_cookieacceptdialog.js and after making the test e10s compatible it failed due to this bug in privatebrowsing.

I'll put the e10s ready test up for review as part of Bug 1132566 soon.


[1] e.g.: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cookieacceptdialog.html
[2] cookieAcceptDialog.xul

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

The comment added in this patch tells the story here. We calculate everything, pass it through to nsICookieService from CookieServiceParent and then ignore it to check the channel (which we don't pass in). As far as I can tell, we end up doing some of these checks twice (including the third party check, though if we fail that, then we return early and don't ask the channel about it). The only bit that we need to pass is the aIsPrivate bit, so in true Gecko fashion, I created a dummy object to pass the state through. This code is very similar to nsHTMLDocument::CreateDummyChannelForCookies but is different enough than I couldn't really share any code.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8635545 [details] [diff] [review]
Patch v1

Josh, can you review this? If not, I'll ask jduell for review.

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

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 8635545 [details] [diff] [review]
Patch v1

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

This is not the first time we've resorted to this, and it probably won't be the last.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5dfb4386c80e

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5dfb4386c80e

Comment 6

[KTucker [:KTucker][Inactive 3/4/2016]]

This landing appears to be causing a smoke test blocker bug 1186920. We may need this backed out.

Comment 7

[Peter Bylenga [:PBylenga]]

Blake can we get this backed out for firefox OS until patched?

see Francisco's comment in bug 1186920:

"In contacts we use a cookie to get/store the value for ordering the contacts list. Is one of the first things that we do, it's in the critical path, and seems that's the reason why the app is failing to start."

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I checked in a patch to fix b2g (see bug 1186920).