1117808 - Service Worker Cache should do the right thing in private browsing mode.

Status: RESOLVED DUPLICATE of bug 1714354

(Core :: Storage: Cache API, defect, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

+++ This bug was initially created as a clone of Bug #1112134 +++

Splitting the discussion about private browsing mode off from bug 1112134.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12)
> (In reply to Ben Kelly (PTO, back Mon Jan 5) [:bkelly] from comment #11)
> > they can't be forensically recovered from the disk.  If its between crashing
> > the whole browser from an OOM vs writing a temp file that might be
> > recoverable, do we really want to favor the OOM?  (Keep in mind OOM'ing may
> > lose data in other non-PB tabs.)
> 
> We should never favor an OOM, obviously.  :-)  Let's take a step back.
> 
> How about this:
> 
> 1. Ensure that if you access the cache API from within a private session, we
> will not return anything from the "normal" cache.  That is, caches should
> give you the illusion that there is nothing stored from the origin initially.

Agreed.  This should be required before pref'ing on.

> 2. Pretend that an origin that accesses the API from a private session has 0
> amount of storage left from the quota manager's perspective, and thus fail
> any request that tries to put anything into the cache.
> 
> This will obviously make it possible to detect whether you are in PB mode,
> but I guess that ship has sailed with IDB (and of course with other side
> channel attacks that make it possible to detect this today.)  But at least
> it keeps the same API surface exposed to content, so if (when) sites do
> stupid stuff such as assuming Firefox N+ has caches implemented, they won't
> break when loaded in PB mode.

Well, there is an open bug to make IDB pretend to function in bug 781982.  Apparently people like to us PB mode to avoid caching during development and its annoying when IDB stops working as well.  Chrome apparently keeps IDB working in this mode.

I think a short term solution would be to act like we have 0 bytes of storage.  I would probably do this synthetically on the child side instead of trying to spin up the entire quota system in the parent.

Long term, I think we should investigate having quota manager create a memory backed storage area.  I believe it has all the hooks necessary to do this:

  - We use custom file URIs to launch sqlite with QM.  This could be modified to create a memory-backed sqlite.
  - QM could provide a custom nsIFile implementation that points to an in-memory structure.
  - QM would similarly return memory-only file streams for those operations.

QM could then be configured to set a limit on the memory used for these structures.

Or maybe we could make memory-backed file systems on all platforms?  I know you can do it on linux, but not sure about windows/mac.

Obviously this would be a longer term approach.

What do you guys think of these short term and long term solutions?

Comment 2

[(no longer active)]

I like the short term approach!

What are the use cases for the long term approach though?  Is it just the cache API?  If yes, I doubt that it would be worth doing so much work to support it...

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> I like the short term approach!
> 
> What are the use cases for the long term approach though?  Is it just the
> cache API?  If yes, I doubt that it would be worth doing so much work to
> support it...

The long term approach would solve the PB issue for all QuotaManager based APIs now and in the future.  This includes IDB.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Let's debate the long-term solutions elsewhere. I think the short-term solution sounds good.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, I guess this bug was created as the "elsewhere" to debate the long-term solution :)

I don't know if a custom nsIFile implementation would really help. Things like the SQLite backend for IDB doesn't operate on nsIFile.

If we want storage to work in PB mode I think we need a real filesystem in order to keep the various storage APIs working.

A few potential solutions

* Have the OS create a memory back filesystem. I'm not sure if that would work on all of our tier-1
  platforms?

* Create normal files, but then spin up a separate watcher process which deletes those files if Gecko
  crashes. We already have the crash reporter doing something similar. Possibly we could reuse that
  process? All we need is to delete a single directory.

* Have something similar to PB but specifically for developers. We could either simply have a checkbox
  for "enable storage in PB mode" flag that developers could change. Or we could use the cookie-jar
  support that gecko already has specifically when entering a "developer mode".

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Oh, I guess this bug was created as the "elsewhere" to debate the long-term
> solution :)
> 
> I don't know if a custom nsIFile implementation would really help. Things
> like the SQLite backend for IDB doesn't operate on nsIFile.

Right, but we already have a custom QM URI handler for opening an SQLite database.  The QuotaManager could further customize this handler to use a memory-backed SQLite instance in this case.  (SQLite already supports being memory-backed, etc.)

If you are doing file IO with QuotaManager, you must use the nsIFile provided to you by QM when you open the directory.  You also have to use the QM-specific file streams.  These are already requirements today.

It seems these existing hooks could just be extended to handle the memory-only case.

> If we want storage to work in PB mode I think we need a real filesystem in
> order to keep the various storage APIs working.

That would be ideal.  I'm just not sure how possible it is.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Ben Kelly [:bkelly] from comment #6)
> If you are doing file IO with QuotaManager, you must use the nsIFile
> provided to you by QM when you open the directory.  You also have to use the
> QM-specific file streams.  These are already requirements today.

I thought we were getting rid of those requirements in the new QM API.

Jan? Bent?

I'm very worried about trying to keep all this data in memory. That will make us OOM really fast on mobile. Both Fennec and B2G.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> I thought we were getting rid of those requirements in the new QM API.

Ah, well, I have not seen the new API yet.  Maybe that indeed is coming.

> I'm very worried about trying to keep all this data in memory. That will
> make us OOM really fast on mobile. Both Fennec and B2G.

Well, I was also proposing that QM would then place a limit on it.  Even a small limit would allow some use of the APIs for development purposes and to make it harder to detect PB mode.

I'm fine not taking on all this work.  If we're going to ever fix bug 781982, though, it would be nice to do it at the QM layer so Cache and other code could share the benefit.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I definitely think this needs to be solved at the QM level. We can't require all storage APIs to have a separate backend implementation for PB mode.

Comment 10

[Jan Varga [:janv]]

(In reply to Jonas Sicking (:sicking) from comment #9)
> Yeah, I definitely think this needs to be solved at the QM level. We can't
> require all storage APIs to have a separate backend implementation for PB
> mode.

I support this approach too, ideally QM would provide a virtual file system API to its clients (just like SQLite does these days). That would open the door for other cool things like compressing data on the fly, encryption, etc.

Comment 11

[(no longer active)]

(In reply to Jan Varga [:janv] from comment #10)
> (In reply to Jonas Sicking (:sicking) from comment #9)
> > Yeah, I definitely think this needs to be solved at the QM level. We can't
> > require all storage APIs to have a separate backend implementation for PB
> > mode.
> 
> I support this approach too, ideally QM would provide a virtual file system
> API to its clients (just like SQLite does these days). That would open the
> door for other cool things like compressing data on the fly, encryption, etc.

This sounds like a fine idea to me.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I would really rather we not do something so fancy as a virtual filesystem. It seems much easier to just have the QuotaManager hand out shmem/DELETE_ON_CLOSE FDs that can be wrapped into file streams or passed to SQLite or other storage backends, doesn't it?

Comment 13

[(no longer active)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #12)
> I would really rather we not do something so fancy as a virtual filesystem.
> It seems much easier to just have the QuotaManager hand out
> shmem/DELETE_ON_CLOSE FDs that can be wrapped into file streams or passed to
> SQLite or other storage backends, doesn't it?

We usually try not to write anything to the disk in private browsing mode in the first place, so it would be nice if we can get away without an fd backed by the disk even if it is DELETE_ON_CLOSE, but if not perhaps we can survive too...

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to [Away: 1/29-2/20] from comment #13)
> We usually try not to write anything to the disk in private browsing mode

Yeah, definitely. But I doubt we can get away with that on mobile, and on desktop we'd have to have some smarts to handle large dbs...

Comment 15

[(no longer active)]

Yeah, I doubt that there is an easy and ideal solution available here.  :-)

Comment 16

[Leo Balter]

I'm interested in the follow up for this bug. I wonder if there is anyone we could reach out for championing the proposed fix.

Thanks!

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Duping forward to bug 1714354 which reflects current plans.