Closed Bug 1254856 Opened 8 years ago Closed 8 years ago

Some Flash websites (forgeofempires.com, bet365.com) can't finish loading with "Accept third-party cookies: Never" checked

Categories

(Core :: Networking: Cookies, defect)

45 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
e10s ? ---
firefox45 + verified
firefox46 + verified
firefox47 + verified
firefox48 + verified
firefox-esr45 45+ verified
relnote-firefox --- 45+

People

(Reporter: alex_keel, Assigned: mrbkap)

References

Details

(4 keywords, Whiteboard: [necko-active])

Attachments

(5 files, 2 obsolete files)

Attached image loop
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160303134406

Steps to reproduce:

I opened firefox.  Username and password displayed for either:
en.forgeofempires.com
en.enlvenar.com

I press login.  The site offers the selection of worlds to play in.
On selecting my world, both sites reset to the login page

I cannot play


Actual results:

Firefox 44.0.2 was fine logging in but 45.0 has broken it.  It is getting tedious to have ever more updates.
It loops between login page and partially loading, then returning back


Expected results:

I should be able to play the game
This only happened since the last update.  The games work on ALL other browsers.
Firefox is broken
Firefox:45.0, Build ID: 20160303134406
User Agent  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0

Hi Alex,

I have tested this issue on the latest Firefox (45.0) release, latest Nightly (48.0a1 - 20160308030418) build and I could not reproduce it. I have created a new account on "en.enlvenar.com" and "en.forgeofempires.com". When I press the "Login" button, the window with selecting my worlds appear. After I choose my world the game starts and I am able to play.
I have also test this on Windows 7 x64 but I could not reproduce it.

Can you please test this on the latest Firefox release or latest Nightly build (https://nightly.mozilla.org/) and report back the results ? When doing this, please use a new clean Firefox profile, maybe even safe mode, to eliminate custom settings as a possible cause (https://goo.gl/PNe90E). 

Thanks,
Cosmin.
Flags: needinfo?(alex_keel)
I tried with FF45 on Win 7, it works. Did you try to delete the website's cookies?

In addition, could you test with a fresh profile.
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles
Attached image cookies.jpg
Flags: needinfo?(alex_keel)
I found it:  I am now forced to add an exception to accept cookies on the sites, see uploaded image.  This is new in 45.0 as it worked without the need to add the exception.
(In reply to Alex Keel from comment #5)
> I found it:  I am now forced to add an exception to accept cookies on the
> sites, see uploaded image.  This is new in 45.0 as it worked without the
> need to add the exception.

Could you provide your custom settings for "History" in about:preferences#privacy
Flags: needinfo?(alex_keel)
(without the dialog box of exceptions hidding the settings page)
Attached image custom.jpg
See my settings
Flags: needinfo?(alex_keel)
STR:

1) Set "Accept third-party cookies: Never" in about:preferences#privacy
2) Open https://en.elvenar.com/
3) Log in (at the top-right) with bugzilla:azerty123
4) Select the world called "FELYNDRAL"

Result: game fails to load completely http://i.imgur.com/hqgs0G1.jpg

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=97419358e5899b3e6ee2e21e7d89106d90b8fa70&tochange=bc44c9e55ef0e042c5a6c88bd241c08282b27f55

Blake Kaplan — Bug 1171215 - Compute third-partyness in the loadinfo instead of nsIHttpChannelInternal so that other protocols correctly respect the third-party cookie pref. r=sicking/ckerschb
Blocks: 1171215
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mrbkap)
Summary: forge of empires and elvenar loop → Some games like Forge of Empires or Elvenar can't finish loading with "Accept third-party cookies: Never" checked
Component: Untriaged → Networking: Cookies
Product: Firefox → Core
Clearing the needinfo so this gets picked up by e10s triage.
Flags: needinfo?(mrbkap)
tracking-e10s: --- → ?
Flags: needinfo?(mrbkap)
To be clear, the fix mentioned in comment 9 changed the behavior for both e10s and non-e10s. Looking through logs, the most obvious hypothesis is that I changed the behavior of the 3rd-party cookie check for plugins somehow. That being said, all of the cookies I saw being blocked were actually 3rd-party to the web site (so IMO correctly blocked). I'll debug tomorrow with an old build to see what cookies we used to block.
The loadinfo code doesn't try very hard to get a window out of the passed-in context. In this case, we have an object element (the plugin) which advertises nsIFrameLoaderOwner for the cases that it's acting as a document but doesn't have one for other cases.

I think we should do more to get a window context out of the loading context.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9d22988d7ce
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Summary: Some games like Forge of Empires or Elvenar can't finish loading with "Accept third-party cookies: Never" checked → Some Flash websites (forgeofempires.com, bet365.com) can't finish loading with "Accept third-party cookies: Never" checked
Attached patch Patch v1 (obsolete) — Splinter Review
This does fix the bug. Bill, it looks like you added the code in question. I want to make sure that this is OK for Web Extensions. I could target the fix more directly (at object or embed elements) but I don't see any reason not to do things this way, the more info we have about the load the better, AFAICT.
Attachment #8729648 - Flags: review?(mozilla)
Attachment #8729648 - Flags: feedback?(wmccloskey)
Comment on attachment 8729648 [details] [diff] [review]
Patch v1

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

Bill probably knows the code better than I do, but it seems totally reasonable to me.
Attachment #8729648 - Flags: review?(mozilla) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Sicking pointed out that we don't need to do the frameloader dance at all for the 3rd-party checks (and I'd actually forgotten that we already skip to the parent window in ComputeIsThirdPartyContext) so that lets us simplify the logic here a bit.
Attachment #8729737 - Flags: review?(mozilla)
Attachment #8729737 - Flags: review?(jonas)
Attachment #8729648 - Attachment is obsolete: true
Attachment #8729648 - Flags: feedback?(wmccloskey)
Comment on attachment 8729737 [details] [diff] [review]
Patch v2

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

::: netwerk/base/LoadInfo.cpp
@@ +86,5 @@
>        if (fl && NS_SUCCEEDED(fl->GetDocShell(getter_AddRefs(docShell))) && docShell) {
>          outerWindow = do_GetInterface(docShell);
>        }
>      } else {
> +      contextOuter.swap(outerWindow);

Nit: I think `outerWindow = contextOuter.forget()` would be slightly more clear and would do the same amount of refcounting. Up to you.
Attachment #8729737 - Flags: review?(jonas) → review+
Blake, with this second patch, don't you need to change ComputeIsThirdPartyContext to stop it from stepping up to the parent if the content type is TYPE_SUBDOCUMENT?
Flags: needinfo?(mrbkap)
Also, I'm worried that we're still computing the outer window wrong for WebExtensions in the case of <object>. Can we do a combination of the first patch and the second one?
Attached patch Combined patchSplinter Review
Here's a combined patch. I must have attached the patch before rebasing it to include the removal of that code in ComputeIsThirdPartyContext.  Sorry about that.

I played with the idea of asserting that this only happens for <object>s, but I've been so snakebitten every time I've tried asserting that sort of thing in this code that I backed off. If we don't have a loader, we use the enclosing window for everything.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aef82f8d5b8
Attachment #8729808 - Flags: review?(wmccloskey)
Attachment #8729808 - Flags: review?(jonas)
Attachment #8729737 - Attachment is obsolete: true
Attachment #8729737 - Flags: review?(mozilla)
Tracking as it might be part of the 45.0.1 dot release.
Blake, let me know what you think about this fix being part of the dot releases. Thanks
inbox.google.com is affected too.
inbox uses flash?
Whiteboard: [necko-active]
Additional Info on inbox.google.com:
- Check Accept cookies from sites
- Accept third-party cookies: From visited

The above works.
(In reply to kodtaku from comment #28)
> Additional Info on inbox.google.com:
> - Check Accept cookies from sites
> - Accept third-party cookies: From visited
> 
> The above works.

The interesting case here is "Accept third-party cookies: Never". Does it work in such a case?
Comment on attachment 8729808 [details] [diff] [review]
Combined patch

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

Thanks Blake. I think some tests would really be great here.
Attachment #8729808 - Flags: review?(jonas) → review+
(In reply to Loic from comment #29)
> (In reply to kodtaku from comment #28)
> > Additional Info on inbox.google.com:
> > - Check Accept cookies from sites
> > - Accept third-party cookies: From visited
> > 
> > The above works.
> 
> The interesting case here is "Accept third-party cookies: Never". Does it
> work in such a case?

You can change it at runtime to get the failure behavoir.  
Fail: Accept third-party cookies: Never
Success: Accept third-party cookies: From visited

Just refresh the page in-between.
Jonas, Blake, could you fill the uplift request asap? 
I would like to start the build of 45.0.1 & 45.0.1esr asap and we will need this patch. Thanks
Flags: needinfo?(jonas)
Keywords: checkin-needed
Keywords: flashplayer
Summary: Some Flash websites (forgeofempires.com, bet365.com) can't finish loading with "Accept third-party cookies: Never" checked → Some websites (forgeofempires.com, bet365.com, inbox.google.com) can't finish loading with "Accept third-party cookies: Never" checked
This needs to land on central first, right? As soon as the tree reopens I'll land it.
Flags: needinfo?(mrbkap)
Comment on attachment 8729808 [details] [diff] [review]
Combined patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Plugins will misbehave on some sites requiring login/cookies.
Fix Landed on Version: About to land on Nightly.
Risk to taking this patch (and alternatives if risky): Users can accept cookies from 3rd parties.
String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: bug 1171215
[User impact if declined]: see above
[Describe test coverage new/current, TreeHerder]: I'm working on an automated test for this now. We have tests for other instances of the 3rd-party cookie blocking behavior.
[Risks and why]: We have decent automated testing coverage for non-plugin 3rd party cookie behavior, but obviously are lacking coverage for plugins. That being said, the code changes here should be quite safe.
[String/UUID change made/needed]: n/a
Attachment #8729808 - Flags: approval-mozilla-release?
Attachment #8729808 - Flags: approval-mozilla-esr45?
Attachment #8729808 - Flags: approval-mozilla-beta?
Attachment #8729808 - Flags: approval-mozilla-aurora?
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #35)
> [Risks and why]: We have decent automated testing coverage for non-plugin
> 3rd party cookie behavior, but obviously are lacking coverage for plugins.
> That being said, the code changes here should be quite safe.

What kind of plugins would we need? Just Flash? If that is the case we have firefox-ui-tests which we might be able to use for such a test. Mind giving me some details what would be necessary?
Flags: needinfo?(mrbkap)
(In reply to Henrik Skupin (:whimboo) from comment #36)
> What kind of plugins would we need? Just Flash? If that is the case we have
> firefox-ui-tests which we might be able to use for such a test. Mind giving
> me some details what would be necessary?

I don't know enough about the details of how flash works. The key is to have a flash plugin that doesn't work if we don't send cookies with any requests that the plugin itself makes.
Flags: needinfo?(mrbkap)
https://hg.mozilla.org/mozilla-central/rev/47f638b8918d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I've reproduced the initial issue on Firefox DevEdition 47.0a2 and on Firefox 45. 

I've tested on Windows 7 64bit and on Mac OSX 10.9.5 using latest Nightly 48.0a1 (buildID: 20160315030230): in about:preferences#privacy I've set "Accept third-party cookies: Never" and the games (https://en.forgeofempires.com/ ; https://en.elvenar.com/) correctly work, i'm able to play. On the "inbox.google.com" I've seen the message "server is temporarily unavailable" and the page keeps refreshing on and on - with "Accept third-party cookies: Never"; when I set "Accept third-party cookies: Always", the "inbox.google.com" page correctly works. Any thoughts about this?
Flags: needinfo?(mrbkap)
Comment on attachment 8729808 [details] [diff] [review]
Combined patch

need for the 45.0.1 dot release.
Attachment #8729808 - Flags: approval-mozilla-release?
Attachment #8729808 - Flags: approval-mozilla-release+
Attachment #8729808 - Flags: approval-mozilla-esr45?
Attachment #8729808 - Flags: approval-mozilla-esr45+
Attachment #8729808 - Flags: approval-mozilla-beta?
Attachment #8729808 - Flags: approval-mozilla-beta+
Attachment #8729808 - Flags: approval-mozilla-aurora?
Attachment #8729808 - Flags: approval-mozilla-aurora+
Jonas, seems that the patch doesn't apply to beta or the other branches, could you rebase it for Blake? We need it asap.
Flags: needinfo?(jonas)
(thanks)
Nevermind, was a pretty straightforward rebase.

https://hg.mozilla.org/releases/mozilla-beta/rev/f487458614db
Flags: needinfo?(jonas)
(In reply to Camelia Badau, QA [:cbadau] from comment #39)
> ... On the "inbox.google.com" ...

I checked out a version of Firefox from before my fix for bug 1171215 and it took forever to load inbox.google.com (close to two minutes). I observed the same behavior with a version of Firefox with my fix for this bug. Therefore, I think that this is a separate issue and should be filed and tracked as a separate bug.
Flags: needinfo?(mrbkap)
To be clear, I was trying with hg revision 97419358e5899b3e6ee2e21e7d89106d90b8fa70
Attached patch MochitestSplinter Review
This test fails without my patch from this bug and succeeds with it. It ensures that we do the right things when we never send 3rd party cookies. I wanted to extend the test to make sure that we do the right thing in the default setting, but we don't send cross-origin cookies for XHR (even if the server allows CORS connections) so I backed off on that.
Attachment #8731004 - Flags: review?(mozilla)
We'll send cookies for cross-site XHR requests if you set xhr.withCredentials=true
Added to the release notes with "Fix some loading issues when Accept third-party cookies: was set to Never (1254856)" as wording
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #47)
> (In reply to Camelia Badau, QA [:cbadau] from comment #39)
> > ... On the "inbox.google.com" ...
> 
> I checked out a version of Firefox from before my fix for bug 1171215 and it
> took forever to load inbox.google.com (close to two minutes). I observed the
> same behavior with a version of Firefox with my fix for this bug. Therefore,
> I think that this is a separate issue and should be filed and tracked as a
> separate bug.

Thank you! 

I've tested also on Mac OSX 10.9.5 and Windows 7 64bit using Firefox 45.0.1 (buildID: 20160315153207) and Firefox ESR 45.0.1 (buildID: 20160315153150) and the games (https://en.forgeofempires.com/ ; https://en.elvenar.com/) correctly work with "Accept third-party cookies: Never" (in about:preferences#privacy).

Since the issue is verified on Firefox 48.0a1, Firefox 45.0.1 and Firefox ESR 45.0.1 build 1, I will mark this VERIFIED FIXED.  

For the issue from "inbox.google.com" page, I filled bug 1257236.
(In reply to Jonas Sicking (:sicking) from comment #50)
> We'll send cookies for cross-site XHR requests if you set
> xhr.withCredentials=true

That can be done as a followup. Right now the test uses sync xhr which throws if you set xhr.withCredentials.
Comment on attachment 8731004 [details] [diff] [review]
Mochitest

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

Ok, I got it all now, but please do add a small description of the test. It's not that obvious right away that frameLoaded() checks for the correct results.

::: dom/plugins/test/mochitest/file_checkcookie.sjs
@@ +2,5 @@
> +  try {
> +    var cookie = request.getHeader("Cookie");
> +  } catch (e) {
> +    cookie = "EMPTY_COOKIE";
> +  }

Just to be on the safe side:

// avoid confusing cache behaviors
response.setHeader("Cache-Control", "no-cache", false);

::: dom/plugins/test/mochitest/mochitest.ini
@@ +136,5 @@
>  skip-if = toolkit == "cocoa"
>  [test_zero_opacity.html]
>  [test_bug1165981.html]
>  skip-if = !(os == "win" && processor == "x86_64" && !e10s) # Bug 1253957
> +[test_bug1245545.html]

is this just a line ending issue?

::: dom/plugins/test/mochitest/test_pluginstream_3rdparty.html
@@ +11,5 @@
> +  <script type="text/javascript">
> +    setTestPluginEnabledState(SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED);
> +  </script>
> +  <link rel="stylesheet" type="text/css" 
> +        href="/tests/SimpleTest/test.css" />

Nit: could you remove trailing whitespaces and also add <meta charset=UTF-8> as you did in the other file.

@@ +15,5 @@
> +        href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +  <p id="display"></p>
> +

would you mind adding a two sentence description of the test? Whenever tests need to be updated I find it super useful to understand what the test is doing right away, because this is plugin specific cookie testing.
Attachment #8731004 - Flags: review?(mozilla) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #54)
> is this just a line ending issue?

Yeah, the existing file doesn't end with a newline (which a few programs still don't like), so my editor automatically adds one.
Setting the flag for a spot check on 46.
Flags: qe-verify+
Huge thanks and gratitude to all those that reported and fixed this issue.

I am not sure if this is what was causing my issue but this is the only thing mentioned in 45.0.1 release notes that seems like it might have been.

Going from 44.0.2 to 45 had caused Hulu not to work for me.  Everything on the site was functioning as normal except whenever clicking a show to watch.  Then it was like Hulu wasn't able to see my account so I was unable to watch anything other than free shows (in lower quality, commercials, etc.) as it would just say this show requires a free trial, a paid subscription or to log in (even though it already recognized me as logged in).  

The site appeared normal otherwise as it would show my watched shows on the main page, my queue, history, favorites, account info showing paid, etc.  Just actually trying to watch a show would only allow me to do so as an unlogged in free member.  Nothing I tried fixed it and Hulu was no help (just use Chrome).

I tried deleting cookies, cache, logging in and out, uninstalling and re-installing Flash, resetting license files, trying about:config settings, enabling/disabling DRM (as Hulu was popping up a must enable DRM banner around this time also), thinking it might be a Noscript setting issue (as 45 seems like it caused a click to allow issue with Noscript also that apparently is unrelated to this).  

Hasn't been a pleasant week or two trying to figure this out as this is above my knowledge level. I couldn't find anyone else mentioning the Hulu issue anywhere and I didn't understand how to ask for help here or file a bug report.

So thank you to all those involved in fixing this.  Grateful.
See Also: → 1257861
This bug didn't fix the Google Inbox issue which is currently tracked in Bug 1257861. Reverting the summary and keyword for clarification.
Keywords: flashplayer
Summary: Some websites (forgeofempires.com, bet365.com, inbox.google.com) can't finish loading with "Accept third-party cookies: Never" checked → Some Flash websites (forgeofempires.com, bet365.com) can't finish loading with "Accept third-party cookies: Never" checked
I've tested on Windows 7 64bit and on Mac OSX 10.9.5 using Firefox 46 Beta 6 (buildID: 20160328182534 ) and Aurora 47.0a2 (buildID: 20160323004040): in about:preferences#privacy I've set "Accept third-party cookies: Never" and the games (https://en.forgeofempires.com/ ; https://en.elvenar.com/) correctly work, i'm able to play.
You need to log in before you can comment on or make changes to this bug.