Anne (:annevk) Reporter
	Description • 9 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.

	(no longer active)
	Comment 1 • 9 years ago

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

	Luke Wagner [:luke]
	Comment 2 • 9 years ago

Edward from Humble Bundle has a large, pressing use case for this that perhaps he can comment on here.

	Edward Rudd
	Comment 3 • 9 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..

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 5 • 9 years ago

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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 6 • 9 years ago

Updated version of patch. Part 4 of new storage logic. Full tree here: https://github.com/mystor/mozilla-central/tree/storage_pref

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

	(no longer active)
	Comment 7 • 9 years ago

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

Over to Kyle.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 8 • 9 years ago

Where is the implementation of StoreAllowedForWindow?  It is not trivial to find in the github tree.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 9 • 9 years ago

(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

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 10 • 9 years ago

(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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 11 • 9 years ago

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?

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 12 • 9 years ago

(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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 13 • 9 years ago

Updated due to changes in implementation of StorageAllowedFor*

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4806930c168

	Pulsebot
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc9377303125

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/cc9377303125

	Luke Wagner [:luke]
	Comment 17 • 9 years ago

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.

	(no longer active)
	Comment 18 • 9 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?

	(no longer active)
	Comment 19 • 9 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...)

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 20 • 9 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 21 • 9 years ago

Why is 42 special?

	Luke Wagner [:luke]
	Comment 22 • 9 years ago

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.

	Kohei Yoshino [:kohei]
	Comment 23 • 9 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/web-storage-indexeddb-cache-api-now-obey-third-party-cookies-preference/

	Chris Mills [:cmills]
	Comment 26 • 8 years ago

This has been documented here:

https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB#Security