Only disable IndexedDB in third-party windows when the third-party cookie preference is set

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: annevk, Assigned: Nika)

Tracking

(Blocks 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla43
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [games:p2][platform-rel-Games])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

4 years ago
Blocking IndexedDB unconditionally in third-party windows is not a sound strategy if we allow other types of storage just fine. IndexedDB should simply follow the cookie policy.

Comment 1

4 years ago
The condition here is when the network.cookie.cookieBehavior pref is set to 1.

Updated

4 years ago
See Also: → 1152899
Edward from Humble Bundle has a large, pressing use case for this that perhaps he can comment on here.

Comment 3

4 years ago
At Humble Bundle we have the Humble Widget which allows a game developer to sell their game direct from their own website by embedding an iframe into their website.   Recently we updated this Widget to support playing a demo version of the game compiled via emscripten to asm.js.  However, due to the IndexedDB restriction the data files for the game are not cached into the IndexedDB, nor accessible from the IndexedDB, thus causing a very undesirable user experience where the data files are downloaded everytime the user wants to play the demo on the developers website. (We only download the file to begin with IF the user hits the play button).

Example... http://jacklumbergame.com/ (at the bottom of the page)

Chrome seems to handle it the same way as this bug proposes..  e.g. they have a "block 3rd party cookies and site data" which disables cookies,localstorage, indexed etc..

Updated

4 years ago
Assignee: nobody → michael
Duplicate of this bug: 1184975
This patch uses the StorageAllowedForWindow logic being implemented in bug 1184789. 

Test coverage on this functionality hasn't been checked yet, but new tests for the behavior will likely have to be written.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=204836d73b40
Depends on: 1184973

Comment 7

4 years ago
Comment on attachment 8641987 [details] [diff] [review]
Update IndexedDB to use common StorageAllowedForWindow logic

Over to Kyle.
Attachment #8641987 - Flags: review?(ehsan) → review?(khuey)
Where is the implementation of StoreAllowedForWindow?  It is not trivial to find in the github tree.
Flags: needinfo?(michael)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> Where is the implementation of StoreAllowedForWindow?  It is not trivial to
> find in the github tree.

StorageAllowedForWindow is being implemented in bug 1184973
Flags: needinfo?(michael)
(In reply to Michael Layzell [:mystor] from comment #9)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8)
> > Where is the implementation of StoreAllowedForWindow?  It is not trivial to
> > find in the github tree.
> 
> StorageAllowedForWindow is being implemented in bug 1184973

I realize now that that probably wasn't very helpful ^.^.

Here's the link to the current implementation on my github: https://github.com/mystor/mozilla-central/blob/storage_pref/dom/base/nsContentUtils.cpp#L8012-L8141

The implementation is still under review in bug 1184973.
Comment on attachment 8641987 [details] [diff] [review]
Update IndexedDB to use common StorageAllowedForWindow logic

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +3917,5 @@
>    AssertIsOnMainThread();
>    MOZ_ASSERT(aPrincipal);
>  
> +  // Ensure that the IndexedDatabaseManager is initialized
> +  NS_WARN_IF(!indexedDB::IndexedDatabaseManager::GetOrCreate());

Why?

::: dom/workers/WorkerPrivate.cpp
@@ +5014,5 @@
>    } else {
>      AssertIsOnMainThread();
>  
> +    // Make sure that the IndexedDatabaseManager is set up
> +    NS_WARN_IF(!indexedDB::IndexedDatabaseManager::GetOrCreate());

Why?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> Why?

I think it was out of caution. The original code paths called that function, and the new code path didn't, and I did a quick skim of it and noticed that it did some work which could have side effects. I was worried that the work it did wasn't thread-safe, and wanted to make sure that the IDB manager was set up so that that work didn't get triggered by a worker. 

I'm not sure if it's necessary.
Updated due to changes in implementation of StorageAllowedFor*
Attachment #8641987 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cc9377303125
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Is there any chance we could land this patch on 42 (Beta, released Nov 2) to expedite the release?  I ask because we've heard this is a pretty significant FF-only problem for Unity-compiled Facebook games which run in a 3rd party iframe and use IndexedDB to cache assets.

Comment 18

4 years ago
(In reply to Luke Wagner [:luke] from comment #17)
> Is there any chance we could land this patch on 42 (Beta, released Nov 2) to
> expedite the release?  I ask because we've heard this is a pretty
> significant FF-only problem for Unity-compiled Facebook games which run in a
> 3rd party iframe and use IndexedDB to cache assets.

Michael, Kyle, what's your evaluation on the risk of this patch being uplifted to Aurora?

Also, is this the only patch that landed after the uplift?
Flags: needinfo?(michael)
Flags: needinfo?(khuey)

Comment 19

4 years ago
Err, wait.  I thought Luke was asking about 43, not 42.  42 is on beta now.  Michael, Kyle, please correct me if I'm wrong but I don't think we should uplift this because a) it has too many dependencies and b) uplifting the whole patch set seems fairly risky to me.  Please correct me if I'm wrong.

(Note that it is _valuable_ to have this in 42 if we can...)
I agree that there are probably too many moving parts in this patch for it to easily be uplifted into beta - I'd rather wait the extra 6 weeks and let it ride out the trains.
Flags: needinfo?(michael)
Why is 42 special?
Flags: needinfo?(khuey)
We already discussed that the patch is risky via email, but for the record, to answer comment 21: because that's 6 more weeks of FB and other portal-hosted games not being able to cache assets in FF.

Incidentally, apparently some FB games handle the error incorrectly and just crash in JS:
  https://apps.facebook.com/944916078884366/
so they don't even run on FF (I guess they only tested in Chrome).  I don't know how prevalent this bug is, though.
Duplicate of this bug: 1228347
Duplicate of this bug: 1228347
Whiteboard: [games:p2] → [games:p2][platform-rel-Games]
You need to log in before you can comment on or make changes to this bug.