Closed Bug 781982 Opened 12 years ago Closed 5 years ago

IndexedDB does not function in private browsing mode

Categories

(Core :: Storage: IndexedDB, defect, P3)

Product:
Core
Component:
Storage: IndexedDB
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1639542
Project Flags:
Webcompat Priority ?
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- affected

People

(Reporter: bugs, Unassigned)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [DevRel:P1])

Attachments

(1 file, 2 obsolete files)

Patch 1 (v3) - Store IDB sqlite DBs in memory during private browsing
19.65 KB, patch
janv
: feedback+
Details | Diff | Splinter Review
If a user is in private browsing mode, IDB is not used. Since the game we are working on relies on IDB to cache data between games, it would be nice if users in private browsing mode were not hitting the website for resources each game. I'm not sure how Firefox handles caching in private browser mode. Do you guys overwrite the disc cache w/ zeros? Do you only use mem cache? Presumably data could still end up on the swap partition. Would you allow the cached data to be written to a tmpfs? Does windows *have* a tmpfs? I suppose it could all be done in memory. Anyway, it'd be nice to have. Over a few games on different maps we might possibly hit the 5MiB boundary, but Firefox probably has plenty of other things cached already. Presumably you have ways to deal w/ low memory situations in terms of wiping stuff.
I think this is by design. Kyle can correct me if I'm wrong, but storing IDB data in PB mode on disk is probably not acceptable since that data is associated with the domain name. For cache, we turn off the disk cache completely when you switch to PB mode, and only use the memory cache. It is possible for the data for the memory cache to be swapped to disk, but we do not aim to provide that level of safety. Kyle, do you know how hard it would be to store IDB data only in memory as opposed to disk? With sqlite it's pretty simple (you just need to open a memory DB -- everything else will work the same way) and that is how we handle things like the download manager database. But I don't even know if IDB uses sqlite for storage...
IndexedDB does use sqlite for storage. We would have to disable things like files in IndexedDB, which store actual files on disk. That would make it possible to detect if you were in PB mode, which may or may not be an issue.
Well, for now I 'spose we can just add an alert notifying that IDB failed to load, and that it might be due to private browsing, or that they need to change permissions for the site. (for most users at present, failure to create IDB is very likely private browsing mode, given the default settings of Firefox, so the fact that it fails to create is also kind of a giveaway :) ) Oh well, would be a nice to have. Kinda surprised about the files thing. Can't store those in memory too? Sure, files get big, but presumably everything is subject to the same 50MiB per prompt limit, or whatever it is. Actually, surprises me that IDB does not do an initial prompt. I thought it was supposed to be 1 prompt to allow, followed by not prompting again for 50MiB, but IDB instead stored data silently, in a clean Nightly profile. Given IDB uses separate privacy settings from other tracking, that's a little worrying actually.
Yeah, I think it should be possible to store those files in memory... Kyle, how hard would that be?
I'm not sure, but I'm not convinced that's desirable. Note that we don't prompt for IDB storage anymore ...
(In reply to comment #5) > I'm not sure, but I'm not convinced that's desirable. Well, allowing websites to detect easily whether you're in PB mode is not desirable either. Why do you think storing the files in memory is not desirable?
Because the point of this is to store large files. You could get to OOM rather quickly.
Do we know how Chrome handles IDB in their private mode?
Really? OOM? Don't you prompt at 50MiB? $ identify temp.jpeg temp.jpeg JPEG 7216x5412 7216x5412+0+0 8-bit DirectClass 8.26MB 0.000u 0:00.000 This image loaded fine in private browsing and I'm guessing took up 149MiB, presumably stored in memory. Popped right up I then opened a 2nd one, 10000x7514 - I guess 287MiB. It also loaded pretty quickly. Flipped back and forth between them, didn't *seem* to do the decoding delay, so I assume it was in memory. If that was our app, we'dve presumably hit 2 user prompts for the first 150MiB, and then 5 more prompts for the second image. At any one of those, the user could have rejected us asking for even more space, and presumably IDB would have errored. Of course, we'd kind of expect that error. Perhaps you guys could trigger that out-of-space error at the first 50MiB if things are getting low on memory :) Oh well, whatever, not that big a deal I guess, we can just do that alert asking them to disable private browsing, as noted.
There are no prompts anymore on trunk.
Wow. That sounds very abusable, I could store gigabytes of data? Man. Forget stealth tracking w/ IDB, a website could use its visitors as a massive distributed filesystem? :) Fun!
(In reply to comment #7) > Because the point of this is to store large files. You could get to OOM rather > quickly. Surely we can use fallible allocation for those files and just throw an error or something if we don't have enough memory to store them?
So. Just wondering. If you guys are gonna let websites dump unlimited data locally. Does that mean HTML5 offline cache prompt will be dropped too?
We should look into creating a memory-backed database for websites that use IndexedDB inside of private windows: http://www.sqlite.org/inmemorydb.html and http://hg.mozilla.org/mozilla-central/annotate/1c88f3f30540/toolkit/components/downloads/nsDownloadManager.cpp#l318 are good references.
Component: Private Browsing → DOM: IndexedDB
Product: Firefox → Core
Oh I see Ehsan marked my report as duplicated :-) Anyway, adding in some comments/suggestion to the discussion: I don't think that failing to provide IndexedDB in PB is an acceptable behaviour. As I user, when I use PB I expect everything to function normally, *only* it won't leave "traces". I don't expect things to fail, silently or not. In my very specific case, I am using PB to simulate running an app for the first time. My app uses indexedDB, so that's why I found it failed in PB. I don't know about the implementation specifics in Chrome, but I often use its private mode to test my app, and IndexedDB works. When I close the window, whatever data I stored goes away. This is the same (predictable) behaviour I expect on Firefox. Hope that sounds reasonable/makes sense! :-)
Yes, we have a plan on what we should do (comment 14). We just need somebody to implement it. :-)
Michael, is this something that would interest you?
(In reply to Josh Matthews [:jdm] from comment #18) > Michael, is this something that would interest you? Yes, I'll take a look at this.
Blocks: 919720
So. If putting it in memory is unacceptable, what about as like an unlinked file on the filesystem encrypted using some random key selected in that private browsing session? That way even if the browser gets closed before a shred occurred, the contents would be useless?
I recently added offline capability to my IDE at https://tedit.creationix.com/ This uses IDB to store the git repos locally. When developing in Chrome I constantly use Incognito to get around cache issues with appcache manifest. When I tried the same trick in Firefox I discovered that my app is completely broken in PB because I use IDB. I see it's been 9 months since mjh563 said he would look at it. Is there progress or did this issue slip between the cracks?
OS: Other → All
Hardware: Other → All
See Also: → 1117808
So, just an idea for the implementor. If one of the roadblocks is concern over storing gigantic amounts of data in RAM. Is there any chance you could just put the IDB in a temp file, but, one encrypted with a randomly generated per-site in-memory key using, oh, XTEA or some other simple thing? Just what are the guarantees in private browsing mode anyway? I guess a prompt on initial IDB creation wouldn't be ok since it would allow inferring private browsing. Unless the prompt was just to let the user decide on temp file vs in memory or something.
Having it be ram-backed, and not survive a reload while still in the same session, is going to suck. Can it not at least be tied to the private browsing session? We're hitting this in Thimble, where we use idb for the filesystem, and Firefox users in private browsing mode are unable to use it, whereas the same users in Chrome have no issue.
I don't believe anybody has suggested memory-backed IDB that only lives as long as a document. My understanding is that everybody wants an IDB that lasts as long as the private session.
Excellent, thanks for correcting me, then. That would be great.
Until there is a fix, can someone recommend a good way to deal with this in a web page, such that you can offer your users some UI or use an alternate db provider? Since calling `open()` on IndexedDB throws an uncatchable error, and doesn't fire the request's `onerror`, I'm not sure how you're supposed to deal with this. It makes your site look broken, when it isn't, and based on the tickets we're getting, people don't necessarily equate "I'm in private browsing..." with "there's no idb," especially since they can't actually see that there's a db even when it works. If there's something that web developers could do to communicate the error, it would be great. Thanks for helping me if I'm just missing something obvious.
Load an iframe with something like this: <script> var canUseIndexedDB = false; function notifyParent() { window.parent.reportIndexedDBStatus(canUseIndexedDB); } setTimeout(notifyParent, 0); window.indexedDB; canUseIndexedDB = true; </script> And use the value reported in reportIndexedDBStatus to inform the user about the limitations.
Thanks Josh, this is interesting.
Wait, since when did we start throwing uncatchable exceptions here? I thought the |indexedDB| property is simply null on private browsing windows?
(In reply to David Humphrey (:humph) from comment #27) > ... Since calling `open()` on IndexedDB throws an uncatchable > error, and doesn't fire the request's `onerror`, ... > var r = indexedDB.open("dummy", 1); r.onerror = function(evt) { console.error(evt.target.error); } logs a DOMError with name "InvalidStateError" for me.
Yup with PouchDB we are seeing we can detect the failure to open the database inside onerror fine. However with indexedDB becoming more popular this is beginning to become more problematic, its very hard to sensible fallback an indexeddb based application without the API and we are seeing more and more reports of people stuck on what to do to handle firefox private browsing. > IndexedDB does use sqlite for storage. We would have to disable things like files in IndexedDB, which > store actual files on disk. That would make it possible to detect if you were in PB mode, which > may or may not be an issue. Disabling particular parts of the api seems troublesome for the same compability issue, it is really hard to have a sensible fallback for authors especially since idb it is somewhat recommended that authors use libraries over indexeddb to interact with its api, which then puts the onus on the authors of those libraries to provide memory fallbacks only for firefox in private browsing.
It does seem line nemo's comment https://bugzilla.mozilla.org/show_bug.cgi?id=781982#c20 made sense to me, expose the current implementation, but in a profile/directory only accessible with a current key and ensure those profiles get cleaned up.
I'm going to take a stab at making in-memory filesystems and databases for indexeddb a reality.
Assignee: nobody → michael
@Michael - any luck with that change? We have a similar issue as reported: we need to synchronously detect whether IndexedDB is available. The sync requirement precludes executing an operation and waiting for an error. A stop-gap solution would be to remove window.indexeddb altogether if it's not supported. Conceptually that makes sense: don't expose the API if every operation is going to fail.
I don't really have the cycles to work on this right now because school etc. Sorry :'(. Unassigning myself.
Assignee: michael → nobody
Marking this as a DevAdvocacy bug. Not only are we not compatible with other browsers but we're also causing problems for multiple web libraries and web sites as the comments here show.
Keywords: DevAdvocacy
If it's not supported, window.indexedDB shouldn't be exposed, otherwise it breaks best practise feature detection Alas WebKit also breaks feature detection with WebSQL in private browsing mode https://bugs.webkit.org/show_bug.cgi?id=154633#c2 https://bugs.webkit.org/show_bug.cgi?id=137760
Whiteboard: [DevRel:P1]
I'll take a look at this. Talked to Baku and Ehsan for some initial ideas for this earlier today.
Assignee: nobody → kyle
Flags: platform-rel?
platform-rel: --- → ?
As of bug 1269361 landing, we now have mPrivateBrowsingId on origin attributes, so we can check against that to figure out what to do with IDB in private browsing mode. Now trying to figure out what approach we want to take to this. We've talked about trying to encrypt the DB to disk, which I'm researching a few solutions for. However, in the mean time, we could start by storing IDB to memory (using ":memory:" as the IDB filepath does the automatically), which will at least allow us to turn on IDB in PB while we figure out a solution for larger IDBs. Speaking of, talked to the Chrome team a couple of weeks ago. They're using LevelDB for IDB, and have strict limits on IDB size in incognito (something like 32mb per database, and 500mb overall for quota manager). So storing IDB in memory right now would at least put us on par with that, and give us more time to figure out our exact needs for encrypted on-disk storage.
Depends on: 1269361
Met with :baku and :janv a couple of days ago, here's the current strategy: - For the time being, indexeddb data that is not blobs will be stored in memory during private browsing mode. The one thing that could break this is that ArrayBuffers are serialized to IDB as fields, not blobs, so that could trip us up on memory usage, but implementing this and testing should be easier than going encrypted right off the bat. - For blob storage, we'll implement a new stream for BlobImpl, which will be encrypted and stored to disk. According to :janv, this should be simpler than implementing a new quotamanager client. We'll also deactivate explicit permanent storage to make sure nothing makes it into the permanent directory during PB. - WASM module compilation will be turned off during private browsing mode. This saves us having to figure out how storage would work for that, and it shouldn't really affect private mode browsing much anyways.
This patch is still work in progress, but captures the general idea of what I'm going for. Basically, if we try to open a database while in private browsing mode, it's opened in memory, and we keep a cache of connections to all the different database FileURLs in QuotaManager until the private browsing session ends. Right now my major issue is cleaning up databases on the last-pb-context-exited observer event. I've co-opt'd the QuotaManager ShutdownObserver, but for some reason firing the runnable to mBackgroundThread causes LazyIdleThread to hit the "Mismatched calls to observer methods!" error at http://searchfox.org/mozilla-central/source/xpcom/threads/LazyIdleThread.cpp#552. Otherwise, the idea seems to work, in that I can close and open tabs and we keep the in-memory databases alive. Baku, could you look over this and let me know if general idea here is correct? If you have any advice on what I'm doing wrong with the observer handling on LazyIdleThread too, that'd be great.
Attachment #8793198 - Flags: feedback?(amarchesini)
Comment on attachment 8793198 [details] [diff] [review] Patch 1 (v1) - WIP: Store Quota Manager/IDB sqlite DBs in memory during private browsing Review of attachment 8793198 [details] [diff] [review]: ----------------------------------------------------------------- It looks good to me, but I don't know why you have this LazyIdleThread issue without debugging it. I also want to ask janv a feedback because he owns this code. Question: do we want to be more generic and call it: "in memory" instead "in private browsing"?
Attachment #8793198 - Flags: feedback?(jvarga)
Attachment #8793198 - Flags: feedback?(amarchesini)
Attachment #8793198 - Flags: feedback+
Comment on attachment 8793198 [details] [diff] [review] Patch 1 (v1) - WIP: Store Quota Manager/IDB sqlite DBs in memory during private browsing Review of attachment 8793198 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsParent.cpp @@ +20920,5 @@ > MOZ_ASSERT(databaseFilePath == mDatabaseFilePath); > #endif > > + // TODO: If we're in private browsing mode, we need the file manager to be > + // somewhere else. Why ? nsIPrincipal now contains the private browsing info, so it should add it to origin attributes I believe, so the origin directory will be separate from "normal" origin. Anyway, these private browsing origins will need to be cleaned up at next temporary storage initialization. ::: dom/quota/ActorsParent.cpp @@ +4239,5 @@ > } > > nsCOMPtr<mozIStorageConnection> connection; > + if (aIsPrivateBrowsing) { > + rv = ss->OpenPrivateBrowsingDatabase(storageFile, getter_AddRefs(connection)); Pfff !, Do you really want to open this database in memory ? It's very important internal quota manager database file and the information stored there is needed to decide if we need to upgrade <profile>/storage or not.
I'll take a closer look later today. Based on your comment and QuotaManager.h changes, I think those changes would better fit in IndexedDatabaseManager. QuotaManager shouldn't deal with quota client specific stuff, like the database system type, in this case SQLite through mozStorage.
Comment on attachment 8793198 [details] [diff] [review] Patch 1 (v1) - WIP: Store Quota Manager/IDB sqlite DBs in memory during private browsing Review of attachment 8793198 [details] [diff] [review]: ----------------------------------------------------------------- I think the number one issue is the way how you access stuff on different threads. You should check/fix that before anything else. Otherwise it will be harder to work on other things that are needed to finish private browsing support. ::: dom/indexedDB/ActorsParent.cpp @@ +4312,5 @@ > mozIStorageConnection** aConnection) > { > + if (!aIsPrivateBrowsing) { > + return aStorageService->OpenDatabaseWithFileURL(aFileURL, aConnection); > + } What about doing it in reverse, then you don't have to negate aIsPrivateBrowsing @@ +4415,5 @@ > } > > + if (aIsPrivateBrowsing) { > + QuotaManager* quotaManager = QuotaManager::Get(); > + NS_ASSERTION(quotaManager, "This should never fail!"); no new NS_ASSERTIONs please, use MOZ_ASSERT @@ +4416,5 @@ > > + if (aIsPrivateBrowsing) { > + QuotaManager* quotaManager = QuotaManager::Get(); > + NS_ASSERTION(quotaManager, "This should never fail!"); > + rv = quotaManager->MaybeAddPBConnection(aFileOrURL); I don't understand, why you have to open the memory database file twice, once here and then again in MaybeAddPBConnection() Am I missing something ? Could you just add something like GetOrCreatePBConnection() ? So if there's one in the hash table already, you just return it. Otherwise, you need to create it and put into the hash table and return to the caller. @@ +18108,4 @@ > directoryInfo.mPersistenceType, > directoryInfo.mGroup, > directoryInfo.mOrigin, > + false, //TODO: Need to actually pass real private browsing info! The maintenance starts with scanning <profile>/storage looking for real indexedDB database files. So this can't be true I believe. @@ +20876,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > + // TODO: Even with an in-memory DB we might need to create this for now? Yes, you need to create it, but stored files/blobs should be encrypted before storing. ::: dom/quota/ActorsParent.cpp @@ +5139,5 @@ > } > } > > +void > +QuotaManager::ClearPBConnections() { So this is called on the PBackground thread, right ? But the hash table is filled on connection threads, right ? Either you need to protect it by a mutex or access it on one thread. I prefer the latter. @@ +5144,5 @@ > + // for (auto iter = mPBConnections.Iter(); !iter.Done(); iter.Next()) { > + // iter->Close(); > + // } > + mPBConnections.Clear(); > + if (mPrivateBrowsingDBConnection) { This was created on the Quota Manager IO thread, but you are on the PBackground thread here. You really need to be more careful about these things. The way to go is to add assertions to check the current thread when you are creating a new method. I think there are other places where you access the same variable on different threads, again you have to be careful about it. There's one exception when you can do that safely. For example, if you have a runnable that is executed on different threads, but it's done sequentially. But again something can be thread safe and something not. @@ +5146,5 @@ > + // } > + mPBConnections.Clear(); > + if (mPrivateBrowsingDBConnection) { > + mPrivateBrowsingDBConnection = nullptr; > + } No need to check the member, just set it to null. You can actually remove this, because I think you shouldn't create memory database for storage.sqlite @@ +5177,5 @@ > + return MaybeAddPBConnection(fileUrl); > +} > + > +nsresult > +QuotaManager::MaybeAddPBConnection(nsIFileURL* aDatabaseName) { Which thread is this called on ? I believe it's a connection thread managed by ConnectionPool in indexedDB/ActorsParent.cpp I really don't think this is suitable place, even IndexedDatabaseManager is not a good option. It should live in indexedDB/ActorsParent.cpp @@ +5193,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + rv = ss->OpenPrivateBrowsingDatabaseURL(aDatabaseName, getter_AddRefs(connection)); > + NS_ENSURE_SUCCESS(rv, rv); Somewhere you use |if (NS_WARN_IF(NS_FAILED(rv)))| which is the current style and a few lines below you have |NS_ENSURE_SUCCESS(rv, rv)| which is the old style.
Attachment #8793198 - Flags: feedback?(jvarga)
(In reply to Jan Varga [:janv] from comment #44) > ::: dom/indexedDB/ActorsParent.cpp > @@ +20920,5 @@ > > MOZ_ASSERT(databaseFilePath == mDatabaseFilePath); > > #endif > > > > + // TODO: If we're in private browsing mode, we need the file manager to be > > + // somewhere else. > > Why ? > nsIPrincipal now contains the private browsing info, so it should add it to > origin attributes I believe, so the origin directory will be separate from > "normal" origin. > Anyway, these private browsing origins will need to be cleaned up at next > temporary storage initialization. That was my convoluted way of saying "we may need to deal with encryption here". Wasn't sure if we'd need to create a completely different directory at this point or not. > > ::: dom/quota/ActorsParent.cpp > @@ +4239,5 @@ > > } > > > > nsCOMPtr<mozIStorageConnection> connection; > > + if (aIsPrivateBrowsing) { > > + rv = ss->OpenPrivateBrowsingDatabase(storageFile, getter_AddRefs(connection)); > > Pfff !, Do you really want to open this database in memory ? > It's very important internal quota manager database file and the information > stored there is needed to decide if we need to upgrade <profile>/storage or > not. I did that because wasn't sure if we stored information about private browsing domains there, that we would need to quickly/easily lose on crash or exit. Will just undo and concentrate on IDB for this. > @@ +4416,5 @@ > > > > + if (aIsPrivateBrowsing) { > > + QuotaManager* quotaManager = QuotaManager::Get(); > > + NS_ASSERTION(quotaManager, "This should never fail!"); > > + rv = quotaManager->MaybeAddPBConnection(aFileOrURL); > > I don't understand, why you have to open the memory database file twice, > once here and then again in MaybeAddPBConnection() > Am I missing something ? > > Could you just add something like GetOrCreatePBConnection() ? > So if there's one in the hash table already, you just return it. > Otherwise, you need to create it and put into the hash table and return to > the caller. Yeah, this sounds fine and much easier than what I'm doing right now, I couldn't figure out whether mozIStorageConnection objects were stateful in a way that would be a problem with the Connection object was shared between multiple owners, so I was just trying to make a connection for each one, as well as a clean one to hold on to. > > ::: dom/quota/ActorsParent.cpp > @@ +5139,5 @@ > > } > > } > > > > +void > > +QuotaManager::ClearPBConnections() { > > So this is called on the PBackground thread, right ? > But the hash table is filled on connection threads, right ? > Either you need to protect it by a mutex or access it on one thread. > I prefer the latter. Oh yeah, threw this in late last night and totally didn't watch what thread it was happening on. Now makes sense why it's crashing heh. > > @@ +5144,5 @@ > > + // for (auto iter = mPBConnections.Iter(); !iter.Done(); iter.Next()) { > > + // iter->Close(); > > + // } > > + mPBConnections.Clear(); > > + if (mPrivateBrowsingDBConnection) { > > This was created on the Quota Manager IO thread, but you are on the > PBackground thread here. > You really need to be more careful about these things. The way to go is to > add assertions to check the current thread when you are creating a new > method. > I think there are other places where you access the same variable on > different threads, again you have to be careful about it. There's one > exception when you can do that safely. For example, if you have a runnable > that is executed on different threads, but it's done sequentially. But again > something can be thread safe and something not. What's running where right now, and which object owns which threads, are pretty opaque at the moment. > @@ +5177,5 @@ > > + return MaybeAddPBConnection(fileUrl); > > +} > > + > > +nsresult > > +QuotaManager::MaybeAddPBConnection(nsIFileURL* aDatabaseName) { > > Which thread is this called on ? > I believe it's a connection thread managed by ConnectionPool in > indexedDB/ActorsParent.cpp > I really don't think this is suitable place, even IndexedDatabaseManager is > not a good option. It should live in indexedDB/ActorsParent.cpp Agreed, if I'm not doing anything with the quota manager, everything should be able to live file-local in indexedDB/ActorsParent.cpp. The march to 30k LOC in the file continues. :) Anyways, I'll get this cleaned up and moved over into IDB today, I think that should simplify things quite a bit.
Forgot to ni? :janv for questions I had in Comment 47.
Flags: needinfo?(jvarga)
I don't see any specific questions there. Anyway, sorry if my feedback sounded offensive to you, I just wanted to save you some time. From my experience, if things are accessed on wrong threads, you can waste a lot of time with debugging/fixing something which wouldn't exist if you touched things on correct threads.
Flags: needinfo?(jvarga)
Reduced scope to only store IDB data in-memory. File manager and quota manager now untouched.
Attachment #8793198 - Attachment is obsolete: true
Move IDB databases to memory when in private browsing mode. Currently does not affect blobs, file manager, or quota manager. Submitting this patch for review now while I work on blobs and the file manager.
Attachment #8794401 - Attachment is obsolete: true
Attachment #8794991 - Flags: review?(jvarga)
I'll take a look at this today.
Comment on attachment 8794991 [details] [diff] [review] Patch 1 (v3) - Store IDB sqlite DBs in memory during private browsing Review of attachment 8794991 [details] [diff] [review]: ----------------------------------------------------------------- I didn't check all small details, but overall this looks much better than the previous patch. My biggest concern is how the last-pb-context-exited notification is handled currently. Anyway, a lot of work still needs to be done like the .metadata file enhancement, clearing ^privateBrowsingId=1 origin directories, encryption/decryption of blobs. I also don't remember if we decided to limit the size of in-memory databases somehow or maybe even switch to storing encrypted databases in future. ::: dom/indexedDB/ActorsParent.cpp @@ +4313,5 @@ > + ******************************************************************************/ > + > +class InMemoryDBConnectionCache final > +{ > + NS_INLINE_DECL_REFCOUNTING(InMemoryDBConnectionCache); I don't see why this needs to be ref counted. Actually, maybe you don't need this class at all. Maybe you can use the hash table directly. @@ +4315,5 @@ > +class InMemoryDBConnectionCache final > +{ > + NS_INLINE_DECL_REFCOUNTING(InMemoryDBConnectionCache); > +public: > + static InMemoryDBConnectionCache* Get(); this is actually GetOrCreate() right ? @@ +5045,5 @@ > + nsAutoCString spec; > + aDatabaseURL->GetSpec(spec); > + > + // If we already have a connection, just return. > + nsCOMPtr<mozIStorageConnection> conn = mConnections.Get(spec); Maybe, instead of going through the service to create a new connection, you could do Clone(), but it's not a big deal. @@ +10154,5 @@ > return gFileHandleThreadPool; > } > > +void > +ClearInMemoryDBConnectionCache() { This is currently only called for last-pb-context-exited. The shutdown case can be handled in QuotaClient::ReleaseIOThreadObjects() I believe. @@ +10159,5 @@ > + AssertIsOnIOThread(); > + if (!gInMemoryDBCache) { > + return; > + } > + gInMemoryDBCache = nullptr; I think we should also start a process here to clean all origin directories that end with ^privateBrowsingId=1. We will also need to detect such origin directories at temporary storage initialization (after a normal restart or crash). So I propose to store the privateBrowsingId=1 info in the .metadata-v2 file. There's a room for that, I put there some reserved words, so we don't need to upgrade all metadata files. Actually, this cleanup should be done by quota manager in the proposed LastPBContextExited() method which will also call into quota clients. @@ -20279,4 @@ > return NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR; > } > > - if (NS_WARN_IF(mCommonParams.privateBrowsingMode())) { I think we need to keep this check for the explicit persistent storage. We don't want to allow it in private browsing. ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +1098,5 @@ > } > > + if (!strcmp(aTopic, "last-pb-context-exited")) { > + QuotaManager* quotaManager = QuotaManager::Get(); > + MOZ_ASSERT(quotaManager); Ok, I know there's no good documentation for this, but I have to say here you are not guaranteed that quotaManager is not null. Even when it's not null you are not guaranteed that you can dipsatch to the IO thread, etc. (quota manager may already be in shutdown mode). I think it would be safer to do something like QuotaManagerService::AbortOperationsForProcess() That dispatches a runnable to the background thread if quota manager is still alive and then calls AbortOperationsForProcess() for each quota client. So you could cleanup the in memory database connection cache in QuotaClient::LastPbContextExited() @@ +1102,5 @@ > + MOZ_ASSERT(quotaManager); > + RefPtr<ClearInMemoryDatabasesBackgroundThreadRunnable> clearRunnable = > + new ClearInMemoryDatabasesBackgroundThreadRunnable(); > + MOZ_ALWAYS_SUCCEEDS( > + quotaManager->OwningThread()->Dispatch(clearRunnable, NS_DISPATCH_NORMAL)); Btw, you missed "return NS_OK" here.
Attachment #8794991 - Flags: review?(jvarga) → feedback+
platform-rel: ? → ---
Anyone interested in seeing this fixed: do Containers solve the usecase you have in mind? https://testpilot.firefox.com/experiments/containers
Containers dont solve the issue for me (+my users). People build web apps that use indexeddb, users use private browsing, web app works on every browser except firefox Its a web compat issue, also breaks one of the features of private browsing in that its supposed to be undetectable?
bostonglobe.com seems to be using this to detect private browsing: https://pastebin.com/bphxwrtT
Agreeing with Dale's comment, word by word. It's a totally different feature - we cannot compare browser 'pseudoprofiles' with incognito mode. As a web developer, the simplicity of opening a new incognito window, doing my tests and closing said window, without having to erase history or create a new profile that I then have to delete, is impossible to beat.
The fact that IndexedDB does not work even for WebExtension background page process in private browsing mode is a MAJOR problem. Basically it breaks all extensions that need IndexedDB storage. And the reason for using that storage is typically that you either a) need the IndexedDB features or b) you have a lot of data to store (or both). IMHO, private browsing mode should not prevent WebExtensions from using IndexedDB fully. Also it appears that disabling History tracking will also disable IndexedDB. Maybe a configuration switch is needed so that people who want absolutely "no trace", "disable extensions" mode can have it by flipping a switch but default should IMHO allow extensions to use IndexedDB. I hope this will get fixed soon.
(In reply to J V from comment #59) > The fact that IndexedDB does not work even for WebExtension background page > process in private browsing mode is a MAJOR problem. Basically it breaks all > extensions that need IndexedDB storage. And the reason for using that > storage is typically that you either a) need the IndexedDB features or b) > you have a lot of data to store (or both). That's a separate issue that requires a different fix. Can you please file a bug ?
(In reply to Tim Nguyen :ntim from comment #60) > That's a separate issue that requires a different fix. Can you please file a > bug ? Thanks. I filed a new issue about this: https://bugzilla.mozilla.org/show_bug.cgi?id=1395932
This appears to have gotten worse recently - for a time, you could avoid the uncatchable InvalidStateError with the following approach: var r = indexedDB.open("dummy", 1); r.onerror = function(evt) { console.error(evt.target.error); // This would prevent the uncatchable InvalidStateError return true; } but currently (Firefox 55.0.3, fresh profile), the above code will cause an InvalidStateError in Firefox in Private browsing mode, and if I'm understanding this correctly, there's no way to detect or prevent this from occurring?
Disappointed today to find out that an in-memory indexedDB is not available in FF57 private browsing mode. Using current nightly. Echoing Dale Harvey's concerns above, container tabs do not address this at all, apps should have a fully working in-memory environment, destroyed when the tab is closed.
Too late for Release 57, and Beta 58 at this point.
status-firefox57: affected → wontfix
status-firefox58: affected → wontfix
Unassigning for the moment. I got as far as in-memory storage, but blob storage is going to be a decent sized project and I don't really have cycles right now.
Assignee: kyle → nobody
Status: ASSIGNED → NEW
Priority: -- → P3
If the in-memory solution is not ready yet, could you at least as a minimal solution do the same as IE/Edge where indexedDB is null during private browsing, allowing feature detection ? It's really weird to have an API available but automatically sending errors. It's a nightmare for local storage librairies authors like me, we all have issues opened in our projects because of this issue in Firefox. Some examples : - https://github.com/cyrilletuzi/angular-async-local-storage/issues/26 - https://github.com/pouchdb/pouchdb/issues/5641
+1 to Cyrille's comment and sentiment for library authors. Our usage at Salesforce suffers because of this. I requested not exposing the API in private mode in comment 35. It does leak that the user _may_ be in private browsing mode (or any other configuration where IndexedDB isn't supported).
Upping the priority, this will be on our agenda.
Assignee: nobody → jvarga
Priority: P3 → P2
Due to this, the current advice going around for Firestore users seems to be to turn off offline storage for Firefox users altogether.[1] [1] https://github.com/firebase/firebase-js-sdk/issues/801#issuecomment-388103532
(In reply to MikeZ from comment #58) > I think dave is right and like tim pointed , it may be used to detect > private browsing which is not a good thing. > > A reasonable solution (pardon me, i am not a firefox dev) would be indeed to > use the browser memory and trash the data when private browsing ends, like > others mentioned. Of course we should ensure this could not be used to > exploit user's computer (ie respect the same storage limit policy). > > Excuse but i feel like i must add something to this "bug". > > I recently tried Feedbro, a new RSS reader Webextension. For practical > reasons it seems to use IndexedDb as cache for fetched articles (i guess its > because size). Of course its not working properly in privacy mode and afaik > it may be because of that "bug". > > Of courses it makes senses to block IndexedDB for websites in privacy mode > (so websites cant track you between sessions). > But what about data from extensions? What could be a privacy problem > regarding extensions data? > > According to Mozilla documentation indexedDB are working under a > same-origin-policy. In other words "only the extension can access to the > datas", but i may be wrong. If this is enforced where is the problem? > > Basicaly, the only BAD case of making indexeddb accessible to extensions in > privacy mode would be... a bad extension allowed to > communicate those data with its creator (and then track the user). But > extensions are manually reviewed on AMO and in this case they may be refused > or forced to have an explicit privacy policy. > > Just my two cents. Same problem here, build a nice extension but not working in private mode. If there would be any reason to not allow it in an extension, why not adding an extra switch for the user "Allow in Incognito" to allow running extension in private mode (see Chrome which is disabled by default)? If the user want it, then they can. But the current state of web extension makes no sense for me, because e.g. an extension automatically run in all 3 worlds (standard, container and private), and there is no barrier like "Allow in Incognito" and the extension can already expose everything. That's why blocking access to indexedDB? Because only the extension has access to the DB. Missing barrier like "Allow in Incognito" is worse than blocking indexedDB in private mode (for extensions). Maybe i am wrong, because i have no idea about such core things... ;-)
(In reply to Thomas Werner from comment #71) > Same problem here, build a nice extension but not working in private mode. > If there would be any reason to not allow it in an extension, why not adding > an extra switch for the user "Allow in Incognito" to allow running extension > in private mode (see Chrome which is disabled by default)? This is a separate WebExtensions issue. The WebExtension background page itself is likely getting marked as in private browsing mode, but should not be. IndexedDB isn't allowed to ignore the private browsing mode flag. I believe Bug 1427986 covers this problem, although I've asked for further clarification about the state of things on that bug. If you could indicate *on that bug* whether "always use private browsing mode" under about:preferences "Privacy & Security" tab under the "History" heading is enabled in the circumstances affecting your extension, that would provide useful information. If it's via some other mechanism, please specify how private browsing mode was turned on and whether it's an extension/page script versus the background page. Thanks!
Assignee: jvarga → nobody
Priority: P2 → P3
We will not be able to address this until Q1 2019.
It would be useful to fix this to prevent the side-effect from being used to target users while in Private Browsing mode and deny them access. The Boston Globe seems to be using this technique: https://twitter.com/kdzwinel/status/1057356459644780544
See Also: → 1506680

latimes.com also does something similar, using https://eb.trbas.com/tronc/latimesarcprod/Bootstrap.js. The user sees https://imgur.com/iGyOOuN.

Stackoverflow answered a question in late 2016 about how, specifically, to fingerprint Private Browsing users for whatever reason:

https://stackoverflow.com/questions/2860879/detecting-if-a-browser-is-using-private-browsing-mode/37091058

Which spawned this gist, that is being maintained actively to this day:

https://gist.github.com/jherax/a81c8c132d09cc354a0e2cb911841ff1#file-is-private-mode-js-L32

// Firefox
if ('MozAppearance' in document.documentElement.style) {
  const db = indexedDB.open('test');
  db.onerror = on;
  db.onsuccess = off;
  return void 0;
}

While the SO question implies that it's to require private browsing mode, sites that depend on user tracking revenue in the wild are using the same logic to refuse service to Private Browsing users.

https://ghacksuserjs.github.io/TorZillaPrint/TorZillaPrint.html .. see "private window" in the first section on screen

// PB Mode
try {
  var db = indexedDB.open("IsPBMode");
  db.onerror = function() {dom.IsPBMode = "true";};
  db.onsuccess = function() {dom.IsPBMode = "false";};
}
catch(err) {
  dom.IsPBMode = "unknown";
}

unknown will happen if you have cookies blocked. The test site is for FF and TB only. But if you wanted to make it universal you would just wrap it in any Firefox feature detection such as

if (isNaN(window.mozPaintCount) === false){ console.log("Firefox")};
if (isNaN(window.mozInnerScreenX) === false){ console.log("Firefox")};
if (isNaN(window.window.scrollMaxX) === false){ console.log("Firefox")};
if (navigator.oscpu == undefined){} else { console.log("Firefox")};

Google is testing a patch to the analog to IndexedDB (the FileSystem API) to prevent targeting of users using private browsing in Chrome, FYI: https://9to5google.com/2019/02/15/google-chrome-detect-incognito/

Per bug 1506680, many newspapers are using this to trigger paywalls - NYTimes, WashingtonPost, Baltimore Sun, Boston Globe, and others.

For the folks who keep commenting here, we are aware of the importance of this bug and of the wide impact of it on many different websites. More comments of this sort aren't helpful.

We are hoping to fix this bug in the next few months, before the end of the year. Please remain patient, thanks!

Restrict Comments: true

(In reply to Mark Baysinger from comment #90)

the priority of this bug is still P3, which according to the wiki means: "This isn't a bad idea, and maybe we'll want to implement it at some point in the future, but it's not near-term roadmap material. Some core Bugzilla developer may work on it."

Actually, the wiki page you're referring to isn't applicable here. It's not very clear, but it's actually describing priorities for bugs/problems with the bugzilla product itself. (note "Some...Bugzilla developer", i.e. someone who works on Bugzilla-as-a-product.)

The P1-P5 semantics for bugs in Firefox are semi-defined in a table here: https://mozilla.github.io/bug-handling/triage-bugzilla
Basically P1 and P2 are reserved for extremely-severe bugs that must be fixed within 1-2 releases (e.g. showstopper bugs), whereas P3 is a wide category for bugs that we also want to fix but not necessarily in the immediate next release. Most bug fixes that land are for P3 bugs.

Anyhow, this is all off-topic, and this bug now has comments restricted so that it can hopefully still remain useful for implementation discussion (without having to wade through a backlog of 100+ comments) once someone starts working on it.

We actually started working on it, see bug 1562669.

Depends on: 1562669

QuotaManager storage v2 will use UUIDs for origin directory names and keep the mapping between origin and origin directory in the memory and a database. For private browsing, we would keep it in memory only.

Depends on: 1593365
Webcompat Priority: --- → ?

Adding support for IndexedDB in private browsing mode is now tracked in Bug 1639542.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: