IndexedDB does not function in private browsing mode

NEW
Assigned to

Status

()

Core
DOM: IndexedDB
5 years ago
20 days ago

People

(Reporter: nemo, Assigned: qdot)

Tracking

(Blocks: 1 bug, {dev-doc-needed, DevAdvocacy})

Trunk
dev-doc-needed, DevAdvocacy
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DevRel:P1])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.

Comment 1

5 years ago
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.
(Reporter)

Comment 3

5 years ago
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.

Comment 4

5 years ago
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 ...

Comment 6

5 years ago
(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.

Comment 8

5 years ago
Do we know how Chrome handles IDB in their private mode?
(Reporter)

Comment 9

5 years ago
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.
(Reporter)

Comment 11

5 years ago
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!

Comment 12

5 years ago
(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?
(Reporter)

Comment 13

5 years ago
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.

Updated

4 years ago
Component: Private Browsing → DOM: IndexedDB
Product: Firefox → Core

Updated

4 years ago
Duplicate of this bug: 892900
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! :-)

Comment 17

4 years ago
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?

Comment 19

4 years ago
(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
(Reporter)

Comment 20

3 years ago
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?

Comment 21

3 years ago
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?

Updated

3 years ago
OS: Other → All
Hardware: Other → All

Updated

3 years ago
See Also: → bug 1117808
(Reporter)

Comment 22

3 years ago
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.
Duplicate of this bug: 1150666
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

Comment 35

2 years ago
@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

Comment 38

2 years ago
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
Blocks: 1267582
Blocks: 1255286
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.
Created attachment 8793198 [details] [diff] [review]
Patch 1 (v1) - WIP: Store Quota Manager/IDB sqlite DBs in memory during private browsing

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 44

a year ago
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.

Comment 45

a year ago
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 46

a year ago
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)

Comment 49

a year ago
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)
Created attachment 8794401 [details] [diff] [review]
Patch 1 (v2) - Store IDB sqlite DBs in memory during private browsing

Reduced scope to only store IDB data in-memory. File manager and quota manager now untouched.
Attachment #8793198 - Attachment is obsolete: true
Created attachment 8794991 [details] [diff] [review]
Patch 1 (v3) - Store IDB sqlite DBs in memory during private browsing

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)

Comment 52

a year ago
I'll take a look at this today.

Comment 53

a year ago
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: ? → ---
Blocks: 1337082
Keywords: dev-doc-needed
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?

Comment 56

5 months ago
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.

Comment 58

2 months ago
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.

Comment 59

2 months ago
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.

Comment 60

2 months ago
(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 ?

Comment 61

2 months ago
(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

Comment 62

a month ago
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?

Comment 63

a month ago
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.
You need to log in before you can comment on or make changes to this bug.