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)

x86
macOS
defect
Not set
normal

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)

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.
The condition here is when the network.cookie.cookieBehavior pref is set to 1.
Edward from Humble Bundle has a large, pressing use case for this that perhaps he can comment on here.
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..
Assignee: nobody → michael
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
Attachment #8635386 - Attachment is obsolete: true
Attachment #8641987 - Flags: review?(ehsan)
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
Status: NEW → RESOLVED
Closed: 9 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.
(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)
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.
Whiteboard: [games:p2] → [games:p2][platform-rel-Games]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: