Closed
Bug 1147821
Opened 10 years ago
Closed 9 years ago
Only disable IndexedDB in third-party windows when the third-party cookie preference is set
Categories
(Core :: Storage: IndexedDB, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: annevk, Assigned: nika)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat, Whiteboard: [games:p2][platform-rel-Games])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
The condition here is when the network.cookie.cookieBehavior pref is set to 1.
Updated•10 years ago
|
Comment 2•10 years ago
|
||
Edward from Humble Bundle has a large, pressing use case for this that perhaps he can comment on here.
Comment 3•10 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•9 years ago
|
Assignee: nobody → michael
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
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
Attachment #8635386 -
Attachment is obsolete: true
Attachment #8641987 -
Flags: review?(ehsan)
Comment 7•9 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)
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
Flags: needinfo?(michael)
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.
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?
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.
Attachment #8641987 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 13•9 years ago
|
||
Updated due to changes in implementation of StorageAllowedFor*
Attachment #8641987 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.
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?
Flags: needinfo?(michael)
Flags: needinfo?(khuey)
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...)
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.
Flags: needinfo?(michael)
Why is 42 special?
Flags: needinfo?(khuey)
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.
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/
Keywords: dev-doc-needed,
site-compat
Comment 26•9 years ago
|
||
This has been documented here:
https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB#Security
Keywords: dev-doc-needed → dev-doc-complete
Whiteboard: [games:p2]
Updated•8 years ago
|
Whiteboard: [games:p2] → [games:p2][platform-rel-Games]
You need to log in
before you can comment on or make changes to this bug.
Description
•