Closed Opened 5 years ago Closed 4 years ago

# Accept cookie dialog shown in private window when e10s enabled

Not set

## Tracking

### ()

RESOLVED FIXED
Firefox 42
Tracking Status
e10s m8+ ---
firefox42 --- fixed

## Attachments

### (1 file)

 4.65 KB, patch jdm : review+ Details | Diff | Splinter Review
STR:
- Enable e10s
- 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.

[2] cookieAcceptDialog.xul
Assignee: nobody → mrbkap
Attached patch Patch v1Splinter Review
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 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
Attachment #8635545 - Flags: review?(josh)
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.
Attachment #8635545 - Flags: review?(josh) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/5dfb4386c80e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5dfb4386c80e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1186920
This landing appears to be causing a smoke test blocker bug 1186920. We may need this backed out.
Whiteboard: [backout-asap]
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."
Flags: needinfo?(mrbkap)
I checked in a patch to fix b2g (see bug 1186920).
Flags: needinfo?(mrbkap)
Depends on: 1187430
You need to log in before you can comment on or make changes to this bug.