1290481 - Implement mitigations for opaque response storage in the DOM cache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[(no longer active)]

See <https://github.com/whatwg/storage/issues/31>.

Comment 1

[(no longer active)]

We should fix this in the next release of Firefox (49).

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Ehsan, David, can you help find an owner to work on this? We are heading into 49 beta 5 so there is not a lot of time to fix/uplift this for 49.

Comment 3

[Andrew Overholt [:overholt]]

Jan's going to do this.

Comment 4

[Jan Varga [:janv]]

Actually maybe someone from Taipei wants take it, let me ask them ...

Comment 5

[Jan Varga [:janv]]

I re-read some emails about this and also the issue on github, but it's still not clear to me what we actually want to do to fix this bug. Do you guys have any concrete ideas ?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

We need to pad and randomize the effective quota usage of opaque responses.  So I think QuotaManager needs some API to say "AddOpaqueEntity()" and "RemoveOpaqueEntity()".  For each opaque "entity" QM would artificially increase quota usage for the origin by some random amount.  Ideally I think we should do this without actually consuming the user's disk space.

Comment 7

[(no longer active)]

I think Ben described it better than I could!

Comment 8

[Jan Varga [:janv]]

Ben, I don't know much about DOM cache internals, I just quickly went through the code.
StartStreamCopy() seems to invoke stream copying for responses, so we can check here if a response is opaque and pass this info to BodyStartWriteStream(). That would then call FileOutputStream::Create() with a new argument e.g. aAddOpaqueEntityOnClose
FileQuotaStream<FileStreamBase>::Close() would call the proposed AddOpaqueEntity()
That also means that Close() may fail because we reached storage limits (group or global limit) when we tried to artificially inflate usage for given origin.
Does this sound good to you ?
I need also figure out when to call RemoveOpaqueEntity() of course.

The estimate() implementation which is in the works calculates usage by iterating over memory objects which will contain the padding (not by getting real file sizes on disk). So everything should work as expected I guess.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jan Varga [:janv] from comment #8)
> Ben, I don't know much about DOM cache internals, I just quickly went
> through the code.
> StartStreamCopy() seems to invoke stream copying for responses, so we can
> check here if a response is opaque and pass this info to
> BodyStartWriteStream(). That would then call FileOutputStream::Create() with
> a new argument e.g. aAddOpaqueEntityOnClose
> FileQuotaStream<FileStreamBase>::Close() would call the proposed
> AddOpaqueEntity()
> That also means that Close() may fail because we reached storage limits
> (group or global limit) when we tried to artificially inflate usage for
> given origin.
> Does this sound good to you ?

So, I was thinking dom/cache would just call AddOpaqueEntity() before it started writing.  So the limit is already baked in before it streams to disk.  If it hits an error during writing it has to call RemoveOpaqueEntity() again.

> I need also figure out when to call RemoveOpaqueEntity() of course.

We can just call RemoveOpaqueEntity() when the record is removed from sqlite.  That way any orphaned body files that are cleaned up on browser restart don't have to it called.

> The estimate() implementation which is in the works calculates usage by
> iterating over memory objects which will contain the padding (not by getting
> real file sizes on disk). So everything should work as expected I guess.

Are you going to persist the random pad amount?  Or recalculate the random pad every browser restart?  Just curious.  I think either way could work.

Comment 10

[Jan Varga [:janv]]

Hm, yeah maybe we can do it before writing, I'll see ...

Regarding persistence, even when we just recalculate the random pad we need to know which origins actually contain padding and how many of them.
One of the partial projects for the next-gen local storage is to persist all the quota info we have in memory, since it's very likely that local storage is used much more often than IndexedDB and DOM cache. So scanning hundreds of directories after every restart would be bad.
For this bug we don't need full caching only a simplified version of it fortunately.

I'm not sure we'll be able to fix this for 49, but I'll do my best.
Anyway, is this so big security problem given the fact we haven't shipped estimate() implementation yet and responses are stored compressed ?

Comment 11

[Anne (:annevk)]

I don't really have much to add. I asked Google to chime in with their solution.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jan Varga [:janv] from comment #10)
> Hm, yeah maybe we can do it before writing, I'll see ...
> 
> Regarding persistence, even when we just recalculate the random pad we need
> to know which origins actually contain padding and how many of them.
> One of the partial projects for the next-gen local storage is to persist all
> the quota info we have in memory, since it's very likely that local storage
> is used much more often than IndexedDB and DOM cache. So scanning hundreds
> of directories after every restart would be bad.
> For this bug we don't need full caching only a simplified version of it
> fortunately.

As long as QuotaManager can track how many opaque things it needs to pad for, I'm fine with the pad changing on every restart.

> I'm not sure we'll be able to fix this for 49, but I'll do my best.
> Anyway, is this so big security problem given the fact we haven't shipped
> estimate() implementation yet and responses are stored compressed ?

The issue is that content script can infer the size of opaque responses through the quota mechanism itself.  For example, add items until you hit quota and writing fails.  Then back off a little and try adding an opaque response.  If it fits, then you have a bound on its size.  Rinse and repeat to determine its exact size.

So its not directly related to estimate().

Comment 13

[Tom Van Goethem]

I'm not that familiar with the codebase, but I'd just like to point out that for this defence to work properly, the random padding should be associated with a Response, and the only way a new random padding value can be generated, is to fetch the resource again (which takes a considerable amount of time, and is noticeable at the server side).
By generating enough random padding values, it's possible to reliably infer the initial value. If an attacker can generate a new random padding value every time an opaque response is stored (approximately every ms), he can gather enough samples in a few seconds to render the added randomness obsolete.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you think it is realistic to fix this for 49? We are heading into beta 8 build today (momentarily) and beta 9 later in the week. I thnk we should aim for 50 here at the soonest. But we can still consider uplift if you land a patch.

Comment 15

[Jan Varga [:janv]]

I was on vacation last week, sorry.
Well, I don't think this is realistic for 49, uplifting seems like a good option.

Comment 16

[Andrew Overholt [:overholt]]

This isn't going to happen in 50.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 52 as affected as well.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Talked with Andrew, we won't ship this in 51 and likely not 52, but it still is on the table for a future project.

Comment 19

[Andrew Overholt [:overholt]]

[Tracking Requested - why for this release]:

This isn't going to happen for 52 or 53 (well, we could *maybe* uplift to 53).

Comment 20

[Marcia Knous [:marcia]]

Tracking 54+ to keep this on the radar.

Comment 21

[Anne (:annevk)]

Shawn, I think we should keep tracking this somehow. It became a little hard to find after you removed that link.

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Anne (:annevk) from comment #21)
> Shawn, I think we should keep tracking this somehow. It became a little hard
> to find after you removed that link.

Originally I thought this would even happen without exposing storage api, so I removed it from storage api meta bug.

Comment 24

[Gerry Chang [:gchang]]

Mark 52 fix-optional as there are no actions for the moment.

Comment 25

[Marcia Knous [:marcia]]

Tracking 55+ per Comment 21.

Comment 26

[Jan Varga [:janv]]

Tom, are you going to take this bug ?

Comment 27

[Tom Tung [:tt, :ttung]]

Sure, I'll take it after finishing Bug 1348050 if you don't mind that.

Comment 28

[Andrew Overholt [:overholt]]

Anne, do you think we need to fix this before we ship the Storage API (bug 1254428)?

Comment 29

[Anne (:annevk)]

If anything this should have blocked shipping navigator.storage.estimate(), which shipped in January per https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate. persist() and persisted() don't change anything here.

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Anne (:annevk) from comment #29)
> If anything this should have blocked shipping navigator.storage.estimate(),
> which shipped in January per
> https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate.
> persist() and persisted() don't change anything here.

estimate() is disabled for release now. We planed to flip the preference together with persist(), persisted() and UI. So looks we should fix this before enabling estimate() by default on release.

Comment 31

[Andrew Overholt [:overholt]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #30)
> (In reply to Anne (:annevk) from comment #29)
> > If anything this should have blocked shipping navigator.storage.estimate(),
> > which shipped in January per
> > https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate.
> > persist() and persisted() don't change anything here.
> 
> estimate() is disabled for release now. We planed to flip the preference
> together with persist(), persisted() and UI. So looks we should fix this
> before enabling estimate() by default on release.

Are we tracking shipping that in bug 1254428?

Comment 32

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Andrew Overholt [:overholt] from comment #31)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #30)
> > (In reply to Anne (:annevk) from comment #29)
> > > If anything this should have blocked shipping navigator.storage.estimate(),
> > > which shipped in January per
> > > https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate.
> > > persist() and persisted() don't change anything here.
> > 
> > estimate() is disabled for release now. We planed to flip the preference
> > together with persist(), persisted() and UI. So looks we should fix this
> > before enabling estimate() by default on release.
> 
> Are we tracking shipping that in bug 1254428?

Not exactly. There are some nice to have enhancements blocking bug 1254428 per my best understanding. 
In the storage project, we use the whiteboard [storage-v1] to indicate shipping blockers.

Comment 33

[Tom Tung [:tt, :ttung]]

Hi Ben and Jan:

I put things that I've investigated so far below. Please feel free to correct me if I'm wrong. Thanks!

Things I need to do:
- Generate the random padding size while receiving an opaque response from the Network[1]. The reason is that we don't want to make attackers generate enough random padding value easily base on comment 13.

- Create two function QM::AddOpqueEntity(uint size), QM::RemoveQpaqueEntity(uint size).
  - AddOpqueEntity() is called by Cache while we are starting to write[2].
  - RemoveQpaqueEntity() is called by Cache while the cache is failed to write[3] or when the record for that response is removed[4].

- Padding size for an opaque response goes with internalResponse and is stored with cacheResponse
- Total padding size for an origin is tracked by QuotaManager and it will be recorded it in the metadata file.

- For QuotaManager, it needs to read padding sizes for each origin from metadata file and add it to its originInfo when we start the Firefox and start to calculate the usage[5]. Besides, it also needs to remove the size when the origin is evicted or cleared.

Things I need to figure out:
- Which kind of random method should I use? Seed & Mean?
- Async problems (e.g. cache writes/deletes successfully, but fails to write in metadata on QM or somethings like this).

[1] http://searchfox.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#377
[2] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#782
[3] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#935
[4] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#1016
[5] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#5047

Comment 34

[Jan Varga [:janv]]

I don't think the metadata file is the right place, only as a last resort. There's storage.sqlite in the profile directory, currently only used for checking storage version. Have you investigated it as a place for storing paddings ?

Comment 35

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #34)
> I don't think the metadata file is the right place, only as a last resort.
> There's storage.sqlite in the profile directory, currently only used for
> checking storage version. Have you investigated it as a place for storing
> paddings ?

Actually, that is one things I want to discuss with you tomorrow. :) 
Since it is in the profile directory, I wonder we need to store at least two more columns (origins, padding size) for this bug. The reason is that we might need a simple way to remove storage for origins (don't need to query padding sizes for specific origins). Therefore, I proposed to store it in metadata or somewhere else under origin directory.

Comment 36

[Jan Varga [:janv]]

There can be none, 1 or a thousand of padding sizes for an origin, right ? So this info is rather dynamic and the metadata file is a plain binary file.
Changing metadata format is hard and it usually causes a major version bump.
Adding a new SQL table to storage.sqlite would only require minor version bump (which is a backward compatible change).
The database will be rather small, so quering/deleting padding sizes for an origin shouldn't be slow.
But yes, we will have to be careful about correct synchronization, crash recovery, etc.

Comment 37

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #36)
> There can be none, 1 or a thousand of padding sizes for an origin, right ?
> So this info is rather dynamic and the metadata file is a plain binary file.
> Changing metadata format is hard and it usually causes a major version bump.
> Adding a new SQL table to storage.sqlite would only require minor version
> bump (which is a backward compatible change).
> The database will be rather small, so quering/deleting padding sizes for an
> origin shouldn't be slow.
> But yes, we will have to be careful about correct synchronization, crash
> recovery, etc.

Oh, I see. Thanks for the feedback!!

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #33)
> - Padding size for an opaque response goes with internalResponse and is
> stored with cacheResponse

Do you mean CacheStorage needs to put something in its sqlite for the padding?  That doesn't seem necessary if we just are adjusting pad values for the overall quota origin.  Seems like we just need to tell the QM when we add/remove opaque things.

Comment 39

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #38)
Thanks for the feedback!

> Do you mean CacheStorage needs to put something in its sqlite for the
> padding?  That doesn't seem necessary if we just are adjusting pad values

Yes, that's what I meant.

> for the overall quota origin.  Seems like we just need to tell the QM when
> we add/remove opaque things.

Got it!

There is one more thing need to be clarified: when should I random the pad value?

Generating a new pad value for an origin when Cache API writes an opaque response into disk seems not a good idea based on comment 13. 

Generating a new pad value for an origin when someone fetches another cross-origin request with no-cors mode seems better. I'm thinking to generate a new pad value for an origin when fetching. And, make InternalResponse keep it. Then, apply the value and update it into storage.sqlite once Cache API writes the response successfully.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I guess I didn't understand comment 13 before.  That does change things a bit in my mind.

That suggests we do need to store the pad amount in CacheStorage.

Sorry for my confusion.

So yea, when we create an opaque response the bad amount would get added to InternalResponse.  That value would then get persisted in and out of CacheStorage so it won't change over time.

CacheStorage can then include the pad amount in its QuotaClient::GetUsageForOrigin() call.  I think this means you don't need to store the pad amount in QM storage.sqlite.  Right?

Can we then just add an argument to FileOutputStream::Create() that specifies the pad amount?

I guess what I don't understand about QM is if it keeps an in-memory running total of quota used or if it recalculates from disk all the time.  Clearly GetUsageForOrigin() can include pad for the "recalculate from disk" approach, but if it keeps a running total in memory then we need to get the pad included there.

Jan, does any of that make sense?

Comment 41

[Jan Varga [:janv]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #40)
> CacheStorage can then include the pad amount in its
> QuotaClient::GetUsageForOrigin() call.  I think this means you don't need to
> store the pad amount in QM storage.sqlite.  Right?

We had a meeting today and I mentioned to Tom that there's another option how to store the pad amount.
For each quota tracked file we would create another file with the pad amount stored in it.
InitOrigin()/GetUsageForOrigin() would then detect it and calculate file size/usage correctly.

QuotaManager::GetQuotaObject() needs to be modified too to correctly calculate file size (to include the pad amount).

> 
> Can we then just add an argument to FileOutputStream::Create() that
> specifies the pad amount?

Maybe, but we can also add something special like QuotaManager::CreatePaddingFor(nsIFile* aFile)
I'm not sure about this, I'll let Tom investigate what would be the best way to do it.

> 
> I guess what I don't understand about QM is if it keeps an in-memory running
> total of quota used or if it recalculates from disk all the time.  Clearly
> GetUsageForOrigin() can include pad for the "recalculate from disk"
> approach, but if it keeps a running total in memory then we need to get the
> pad included there.

For default and temporary storage, we cache usage in memory objects. These objects
are created by calling QuotaClient::InitOrigin(). GetUsageForOrigin() needs to be
updated too, to correctly report origin usage in page info dialog and new storage preferences.

Comment 42

[Tom Tung [:tt, :ttung]]

Just to add some notes: 

(In reply to Jan Varga [:janv] from comment #41)
> (In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment
> #40)
> > CacheStorage can then include the pad amount in its
> > QuotaClient::GetUsageForOrigin() call.  I think this means you don't need to
> > store the pad amount in QM storage.sqlite.  Right?
> 
> We had a meeting today and I mentioned to Tom that there's another option
> how to store the pad amount.
> For each quota tracked file we would create another file with the pad amount
> stored in it.
> InitOrigin()/GetUsageForOrigin() would then detect it and calculate file
> size/usage correctly.
> 
> QuotaManager::GetQuotaObject() needs to be modified too to correctly
> calculate file size (to include the pad amount).

It's better to let QM know the current usage including padding in-memory (OriginInfo or QuotaObject), so that it don't need to ask for getting opaque responses' padding size each time when QM wants to check the usage (e.g. MaybeUpdateSize checks the current usage to ensure the usage don't exceed the quota, LockedCollectOriginsForEviction check the usage for inactive origin).

As Jan mentioned in that meeting, for storing padding sizes in disk, I will try to find out the appropriate way to store it in QM (storing in storage.sqlite or storing in another new file). Right now, I may have one more option which is just storing it CacheStorage and QM gets the overall padding sizes when it's initiating.

Besides, I also need to ensure we get the same usage from estimate() and preferences UI.
 
> > Can we then just add an argument to FileOutputStream::Create() that
> > specifies the pad amount?
> 
> Maybe, but we can also add something special like
> QuotaManager::CreatePaddingFor(nsIFile* aFile)
> I'm not sure about this, I'll let Tom investigate what would be the best way
> to do it.

Sure, I'll figure it out.

Comment 43

[Tom Tung [:tt, :ttung]]

Also there are patches to simulate the attack (getting the anti-CSRF token) in Bug 1350072. I can reproduce it by running them.

Comment 44

[Tom Van Goethem]

For completeness: the size of the response headers (more precisely Content-Length) can also be leveraged to exploit BREACH-like attacks (explained in a bit more detail in the second paragraph here: https://tom.vg/2016/08/request-and-conquer/#compression-based-attacks).

Comment 45

[Tom Tung [:tt, :ttung]]

Attachment: wip-p1.patch

Comment 46

[Tom Tung [:tt, :ttung]]

Attachment: wip-p2.patch

Comment 47

[Tom Tung [:tt, :ttung]]

Attachment: wip-p3.patch

Comment 48

[Tom Tung [:tt, :ttung]]

Attachment: wip-p4.patch

Comment 49

[Tom Tung [:tt, :ttung]]

Attachment: wip-p5.patch

Comment 50

[Tom Tung [:tt, :ttung]]

Current Progress:
P1: Get opaque response's padding length when we get it from the network.
P2: Store it into Disk as Cache SQLite 
P3: Modify MaybeUpdateSize to make it be able to calculate padding as usage rather than just usage.
P4: Make OriginInfo, GroupInfo, ... etc. to be able to calculate padding, too.
P5: Make Cache API be able to notify QuotaManager response's padding.

ToDo:
1. Consider storing a copy(overall padding size for an origin) for QuotaManger.
2. Investigate random method, mean and seed (may refer to [1], but needs to consider how to get size of opaque response in FetchDriver.)
3. Get overall padding for origins when initializing QuotaManager.
4. Testcases

[1] https://github.com/whatwg/storage/issues/31

Comment 51

[Tom Tung [:tt, :ttung]]

Hi Ben,

I'm working on updating padding length for an opaque response and utilize FileOutputStream to update it. 

I found that we'll create two different directories and files if we use Cache:Add to add the same request twice. And, I was thinking to utilize the mechanism for adding the same request via Cache:Add/Cache:Put twice to update padding length. 

However, I found that we only update the usage to QuotaManager when Cache API is about to write something into the disk [1]. We don't update the usage to QuotaManager when we delete files and directories (file->Remove() at [2-4]). 

e.g. 
caches.open("test").then(c => c.add(url)); // first
// create file xxxx/cache/morgue/206/{92412019-df4e-be4b-9e6f-56645876f0ce}.tmp <- update padding to QM through MaybeUpdateSize()[1]
// .tmp -> .final

caches.open("test").then(c => c.add(url)); // second
// create file xxxx/cache/morgue/217/{5825fda4-50dd-1f48-94a3-f455d03bcfd9}.tmp <- update padding to QM through MaybeUpdateSize() [1]
// delete file xxxx/cache/morgue/206/{92412019-df4e-be4b-9e6f-56645876f0ce}.tmp <- update padding to QM ??[2]
// .tmp -> .final

Did we do that for any reasons? If so, could you tell me why we did that?
If not, I'll file a bug for it and try to resolve it first. Thanks!

[1] http://searchfox.org/mozilla-central/source/dom/quota/FileStreams.cpp#81
[2] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#66
[3] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#250
[4] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#324
[5] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/dom/cache/FileUtils.cpp#465

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

I thought QuotaManager wrapped nsIFile and automatically adjusted the size when Remove() is called.  Jan, do we have to do something else to explicitly tell QM about removals?

Comment 53

[Jan Varga [:janv]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #52)
> I thought QuotaManager wrapped nsIFile and automatically adjusted the size
> when Remove() is called.  Jan, do we have to do something else to explicitly
> tell QM about removals?

No, there's nothing like a wrapping around nsIFile.
Here's what needs to be done:

rv = file->GetFileSize(&fileSize);
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

rv = file->Remove(false);
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

quotaManager->DecreaseUsageForOrigin(type, group, origin, fileSize);

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Ok.  I find it confusing that if you use a writable stream, its wrapped, but if you use the nsIFile you get from QM its not wrapped.

So Tom, can you please fix that as part of this as well?  Thanks.

Comment 55

[Jan Varga [:janv]]

Well, you could use a stream (with io flags PR_TRUNCATE) to truncate a file, then usage updating would be done automatically.
That might be even nicer way how to do it.

Comment 56

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #54)
> So Tom, can you please fix that as part of this as well?  Thanks.

Sure!

Comment 57

[Tom Tung [:tt, :ttung]]

Attachment: patch for updating usage for dom cache

Comment 58

[Tom Tung [:tt, :ttung]]

Comment on attachment 8870372 [details] [diff] [review]
patch for updating usage for dom cache

Move this patch to bug 1367309

Comment 59

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This is on going, but more likely to happen since 56. Considering this has been here for quite a while and it's late in 55, mark fix-optional for 55.

Comment 60

[Tom Tung [:tt, :ttung]]

Attachment: P1: generate a random number eachtime when we fetch a opaque response.

Todo: generate real random number.

Comment 61

[Tom Tung [:tt, :ttung]]

Attachment: P2: Store padding length to the disk (Cache.sqlite).

Comment 62

[Tom Tung [:tt, :ttung]]

Attachment: P3: Update MaybeUpdateSize to make it be able to deal with padding.

Comment 63

[Tom Tung [:tt, :ttung]]

Attachment: P4: Calculate padding while counting usage

Comment 64

[Tom Tung [:tt, :ttung]]

Attachment: P5: Update padding length to the QuotaManager while storing an opauqe response.

Comment 65

[Tom Tung [:tt, :ttung]]

Next: 

- Calculate overall padding from cache.sqlite when initializing QuotaManager
  - Note: Be careful about only one connection is allowed to cache.sqlite at the same time (make sure which thread access that SQLite) based on meeting(5/24) with Jan.

- Random method...

Comment 66

[Tom Tung [:tt, :ttung]]

Based previous service worker meeting, I should find some other way to store paddings rather than using cache.sqlite file. 
I had used cache.sqlite to store paddings for each response, and it takes too much overhead/time when initializing each origin to get the usage. This approach needs to spend additional time to initial cache.sqlite for getting overall paddings for each origin.

So, I list what should be taking care below:
1. For each opaque/opaque-redirect response is stored by Cache.add()/Cache.addAll()/Cache.put(), its padding length needs to be stored and updated to the QuotaManger.
2. When the QuotaManger is initializing or the user checks overall usage from preference UI, we need to get padding length for origin from the file.

Comment 67

[Tom Tung [:tt, :ttung]]

Hi Ben and Jan,

I'm finding a way out to store padding for the opaque response. 

I'm used to considering to store it in the cache.sqlite file. But, it takes too much time to initialize cache.sqlite file when QM gets usage from quota clients.

Right now, I try to make each opaque response create a pad file to record its padding length. The pad file is named ".pad" and stores with the body file (e.g. /morgue/{end-of-id}/{id}.final, /morgue/{end-of-id}/.pad). Though this method takes extra time on creating/accessing a file to store/read padding length, it saves time to initialize cache.sqlite file.

Before starting to finalize the patches and send them to review, I'd like to ask for your suggestions for this approach. 
I put my current implementation and questions below inline. Could you help me to check if there are problems with it I need to fix or something I need to take care or I miss anything? Thanks!

- Implementation 
For DOM Cache
1. While creating opaque and opaque redirect responses [1, 2], generate padding length.
[1] http://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#65
[2] http://searchfox.org/mozilla-central/source/dom/fetch/InternalResponse.cpp#68

2. Before writing the body to OS filesystem [3], create a file to store padding length of the response if it's an opaque or opaque redirect response (padding length > 0). And, update the padding length to QM. 
[3] http://searchfox.org/mozilla-central/source/dom/cache/FileUtils.cpp#143

3. When Cache gets the response from OS filesystem(e.g. cache.match[4], cache.matchAll[5]), read the padding length from the pad file (file to store padding length) as well to get its padding length. 
[4] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#511
[5] http://searchfox.org/mozilla-central/source/dom/cache/Manager.cpp#577

4. When removing the body file (BodyDeleteFiles()[6]), remove the file for recording padding length as well and update decreasing padding length.
So that, if a failure happens while writing the body to file, the padding length will be updated to the original length by BodyDeleteFiles().
[6] http://searchfox.org/mozilla-central/source/dom/cache/FileUtils.cpp#254-284

5. When it's the first time to call EnsureOriginInitialized(), the QuotaManager gets usage from the quota clients. I'll calculate all the pad files while traversing the directory in GetBodyUsage()[7].
[7] http://searchfox.org/mozilla-central/source/dom/cache/QuotaClient.cpp#38-61

Note:
- Pad file is created on target thread, but it can be read from target thread (when cache API is called) and background thread (when checking usage).

For QM
1. Add "mPaddingLength" attributes for OringinInfo and GroupInfo and "mTemporaryStoragePaddingLength" variable to track overall padding lengths for an origin, a group and all.

2. When returning overall origin usage (GetUsageOp[9], GetOriginUsageOp[10]), count the padding length as usage.
[9] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#6857
[10] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#7010

- Question
For DOM Cache
1. Is there a method to estimate how many usages will take to store an opaque response before we actually store it into filesystem? The reporter suggested it's better to make the mean relate to the size of the response.

2. I'm thinking to only store pad file when the response owns padding. It may expose the cached response type is opaque/opaque-redirect or not, but I think it's fine to do that. Do I need to create this file for all the cached body like a metadata file?

For QM
1. CacheQuotaClient::GetUsageForOrigin() live in the backgroundThread. Since QM provides an API to calculate overall usage by calling this function at runtime, I'm a little afraid the result for current usage might be incorrect. Or it's okay to do that since we just need to have a snapshot of overall usage? If so, I think it's fine for getting padding in the background thread as well.

2. Do we need to distinguish padding length and usage in QM? I think it's better to do that for checking purpose. I store them separately for now, but I cannot find any other strange reason to do this and I'm not sure it's a good idea or not.

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #67)
> - Question
> For DOM Cache
> 1. Is there a method to estimate how many usages will take to store an
> opaque response before we actually store it into filesystem? The reporter
> suggested it's better to make the mean relate to the size of the response.

I don't think so.  We don't know the full size on disk until you have written the temp file.  You can create the pad metadata before moving that to its final location, though.

> 2. I'm thinking to only store pad file when the response owns padding. It
> may expose the cached response type is opaque/opaque-redirect or not, but I
> think it's fine to do that. Do I need to create this file for all the cached
> body like a metadata file?

Please only create these for opaque responses.  (Is opaque-redirect a concern here?  I didn't expect body data there.)

One reason not to store this for every body file is that these files will each take up one disk block.  This is typically 4KB which is a fair amount of overhead for such a small amount of data.

Comment 69

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #68)
Thanks for answering!

> I don't think so.  We don't know the full size on disk until you have
> written the temp file.  You can create the pad metadata before moving that
> to its final location, though.

Oh, I see. That might be a good solution, but there are some issues need to be figured out. Such as: checking if we can write the padding into virtual space (it acquire QM mutex lock twice) since it updates padding after writing the temp file. (Currently, we check the quota when writing the temp file.)

Anyway, I'll try to find a way to do that. If I cannot find a good solution, I will file a follow-up bug for it.

> Please only create these for opaque responses.  (Is opaque-redirect a
> concern here?  I didn't expect body data there.)

Sorry, I didn't think too much about this. I thought the different between opaque-redirect and opaque response is redirected or not. I didn't notice opaque-redirect response don't have a body data.

> One reason not to store this for every body file is that these files will
> each take up one disk block.  This is typically 4KB which is a fair amount
> of overhead for such a small amount of data.

Got it!

Comment 70

[Tom Tung [:tt, :ttung]]

In today's meeting, 

- Jan suggested that I can think about to save padding length in the database(cache.sqlite), but recording overall padding length for an origin in a file cache directory (xxxOrigin/cache/.padding) to avoid taking too much time when initializing QM. And, it takes about the same time to get the file size and read this file content.

- Besides, the file should only take 8 bytes to store overall padding length. We don't count this file as part of usage. We only count its content(padding) as part of usage.

- For consistent problem (having different overall padding length on sqlite and .padding file), I can reference from indexedDB. It makes sure that it takes care all the unsync problems for a file and database. Basically, I need to update the padding length to QM and file just before executing Commit() to cache.sqlite database.

- I don't need to treat usage and padding as two different things in QM.

Comment 71

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #70)
Jan, please correct me if I wrote anything wrong or I forgot to put anything. That's why I don' cancel the ni flag.

Comment 72

[Jan Varga [:janv]]

Yeah, you understood it correctly. We'll need to talk about the consistency problem a bit more, but for now you don't have to worry about it too much.

Comment 73

[Tom Tung [:tt, :ttung]]

Hi Ben,

If I create a metadata file to track current overall padding length for an origin in DOM Cache at cache directory(e.g. "<profile>/storage/default/http+++origin/cache/.paddings"). Also, I record each padding length on cache.sqlite file at the same time.

So, the advantage of this method is it only takes eight more bytes and one more column on the cache.sqlite database to track the padding for DOM Cache. 

When calculating usage from each quota client, DOM Cache only needs to check the .padding file to get the overall usage of this origin. It takes about the same time to get the file size. 

Besides, compared to create a .padding for each cache.put() an opaque response into the disk, it won't create too many files in the cache directory. (How many cached opaque responses do a normal user have in average? If they are not so many, maybe we could have a .padding file with each cached opaque response like I said in comment 67)

Moreover, it won't need extra read when cache.match()/cache.key() to get a cached opaque response. We get its padding length from cache.sqlite when we get the header.

The disadvantage is that I need to implement this mechanism carefully to avoid inconsistency problem between the .padding file and cache.sqlite file.

Does it sound good to you?

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

Sound good to me.  Thanks!

Comment 75

[Tom Tung [:tt, :ttung]]

Below are the current thoughts after discussing with Jan

Each origin should have a .padding file to ensure something like someone wouldn't remove padding file manually.
-> it means we will need to increase 8 bytes for every cache directory

The biggest problem for now is that how to deal with the version compatibility. -> solution: major upgrade?

[Execute cache operation]
Current step A. BodyStartWriteStream(): Write the body to the temp body file
(can be any thread)

Current step B. OnAsyncCopyComplete(): After finishing writing body file
(target IO thread)
1. Get the size of body file and calculate the padding size for the opaque response
2. Update padding size to QM by creating quotaObject (path: the body file's path)

Current step B.1. Change temp body file to the final body file (BodyFinalizeWrite())
(target IO thread)

3. Copy the .padding file to .padding-tmp file
4. Read the overall padding size from the .padding-tmp file(or calculate it from cache.sqlite file?)
5. Update the overall padding size to the .padding-tmp

Current step B.2. Update the cache.sqlite (commit)
(target IO thread)

6. Rename the .padding-tmp file to the .padding file

*Fail -> remove .padding-tmp

[Upgrade]
1. Search opaque responses from each cache.sqlite and generate padding size for each of them
2. Calculate overall padding size for an origin and write it to the cache directory padding file
(Still do this even the padding size for the origin is zero)
3. Update the QuotaManager's schema (Major upgrade -> Need to consider more)
http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#129

[InitOrigin for each quota client]
1. Find .padding files from the cache directory
- If we can find it, get the padding size from the .padding files
- If we cannot find it or we cannot read it, we need to access the cache.sqlite to get all the recording padding size from the database and write it to the .padding files.
(Need to recalculate the overall padding size if the tmp file is found, becasue we cannot distingulish if we crash before commiting the sqlite or not)

Comment 76

[Tom Tung [:tt, :ttung]]

Hi Ben,

I planned to add padding for all the existing opaque responses while upgrading the cache.sqlite's version. However, I'm not sure if we need to do this or not. I mean the opaque responses which have been stored in the database before upgrading to the newest version.

So, I wanted to add padding for them to avoid exposing the existing opaque response size. However, I'm not pretty if this would happen. Besides, I'm a little worried about what to do if the generating padding makes the usage exceed the limit when upgrading the database.

Could you give me some suggestions for this? Thanks!

Comment 77

[Ben Kelly [:bkelly, not reviewing]]

I don't think we need to go back and add it to existing opaques responses.  We can just add it to new opaque responses.  As long as the code is smart enough to understand there might be an old opaque response without any padding set.

Comment 78

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #77)
I see. Thanks!

Comment 79

[Tom Tung [:tt, :ttung]]

Attachment: P1: Make InternalResponse and CacheResponse be able to keep padding size.

Comment 80

[Tom Tung [:tt, :ttung]]

Attachment: P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite.

Comment 81

[Tom Tung [:tt, :ttung]]

Attachment: P3: Record padding size into cache.sqlite

Comment 82

[Tom Tung [:tt, :ttung]]

Attachment: P4: Update padding size to the QuotaManager

Comment 83

[Tom Tung [:tt, :ttung]]

Attachment: P5: Implement a function to generate padding size

Comment 84

[Tom Tung [:tt, :ttung]]

Attachment: P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory.

Comment 85

[Tom Tung [:tt, :ttung]]

Attachment: P7: Implement few utilis function to access direcotry padding file

Comment 86

[Tom Tung [:tt, :ttung]]

Attachment: P8: Implement a function to get overall padding size for an origin from cache.sqlite

Comment 87

[Tom Tung [:tt, :ttung]]

Attachment: P9: Update padding size to the direcotry padding file

Comment 88

[Tom Tung [:tt, :ttung]]

Attachment: P10: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage

Comment 89

[Tom Tung [:tt, :ttung]]

Attachment: P11: Get the padding size while QM gets usage from DOM Cache

Comment 90

[Tom Tung [:tt, :ttung]]

Attachment: P12: Get padding from DB when there is no directroy padding file

Comment 91

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=969b8d4de3b07788e7162fe69a658f717c8f15dc&selectedJob=115567389

Comment 92

[Tom Ritter [:tjr]]

Uhg, I'm so bad at statistics. I think the mechanism in the patch allows leaking the size after a number of observed measurements.

The current mechanism:
a) Picks a random number R between 0 and 102400
b) Adds the real size to R
c) Rounds R up to a multiple of 20483 (It's not clear to me why a prime number was picked though...)

If you make enough measurements, you will observe that the smallest and the largest bucket occur less frequently than the others. Based on the frequency of the smallest and largest bucket, and the knowledge that it's a uniform distribution of random numbers, it seems like you can calculate the real size?

I know we are restricting the 'pick a random padding' call only to resource download to prevent this from being too easy, but I'm still concerned. 



I wonder if we can eliminate the vulnerability entirely...?  What if on browser startup we choose a cryptographic random number and use it in conjunction with an HMAC scheme. Calculate the padding to be used as a function of HMAC(key, size-of-resource) where the key is calculated as hash(crypto-random_number || resource uri).

(We need to include the resource uri in the key. This prevents an attacker from building a lookup table of "This real size maps to this padding value.")

This way, re-requesting a resource will always return the same (padded) size for as long as the browser is running - without actually having to remember the decision if the item is removed from storage. (We don't attempt to defend against attacks occurring across browser restarts.) 

I'm not certain this is a safe approach, nor am I certain the current approach is wrong - would love to get feedback.

Comment 93

[Tom Ritter [:tjr]]

I thought about this more.

1) The HMAC with the resource URI as part of the key might work if there was one and only one representation of a URI, but there are infinite (querystrings.) This puts us back to the statistical analysis. Attempting to sanitize the URI a) wouldn't work much of the time and b) in some web applications would leak the exact size after only two queries.

2) Using the origin instead of the resource URI would break in a small number of queries if the attacker can create a lookup table of sizes on that origin. For things like twitter this is easily possible with attacker controlled data.

So the HMAC idea is out I think.

The attack I see with the current design - assuming it works - is based around the idea that the occurrence of the buckets is non-uniform, because the first and last buckets occur less frequently. If we used a weighted distribution - instead of uniform - to make those buckets appear as frequently as the others, that should go away.

But are we hiding anything by _having_ buckets? I don't think we are. The real size is always somewhere in the middle bucket.

So I think the 'add a random number' part of 'add a random number and then round up' should be removed. That eliminates the bucket-occurance attack and the false security in having multiple buckets a size can be in.

Comment 94

[Tom Tung [:tt, :ttung]]

(In reply to Tom Ritter [:tjr] from comment #92)
Thanks for giving feedback!

> The current mechanism:
> a) Picks a random number R between 0 and 102400
> b) Adds the real size to R
> c) Rounds R up to a multiple of 20483 (It's not clear to me why a prime
> number was picked though...)

Sorry, I don't have much sense with security things...

So, I have no idea how to pick up an appropriate number. I just arbitrary pick up (102400, 20483) for now to test locally.
I reference from the solution provided in [1]. 102400 is rMax and 20483 is D in [1] (For values of rMax and D, tomvangoethem suggests 100kb and 20kb respectively). 

Thus, these two magic numbers should be much more meaningful and should be changed after gaining more feedback. 

[1] https://github.com/whatwg/storage/issues/31

> If you make enough measurements, you will observe that the smallest and the
> largest bucket occur less frequently than the others. Based on the frequency
> of the smallest and largest bucket, and the knowledge that it's a uniform
> distribution of random numbers, it seems like you can calculate the real
> size?

Just discuss with Tim Huang, we find out it's probably to get the real size by collecting tons of data and knowing it's a uniform distribution.

(In reply to Tom Ritter [:tjr] from comment #93)
> So I think the 'add a random number' part of 'add a random number and then
> round up' should be removed. That eliminates the bucket-occurance attack and
> the false security in having multiple buckets a size can be in.

If we do this, I guess we may need to prevent the roundUp number, says D, is related to the real size, says f. 
For example, it might be easy to perform the attack (utilize we will have smaller size after zip the file) in Bug 1350072 when D = f + 1 (It makes the result size become even more obviously between guessing the correct secret key and not).

Comment 95

[Tom Tung [:tt, :ttung]]

Attachment: P1: Make InternalResponse and CacheResponse be able to keep padding size.

Update patch to make response copy its padding size while clone().

Comment 96

[Tom Tung [:tt, :ttung]]

Attachment: P5: Implement a function to generate padding size.

Update patch for making part of code be initialied once.

Comment 97

[Tom Tung [:tt, :ttung]]

Attachment: P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory.

Update patch.

Comment 98

[Tom Tung [:tt, :ttung]]

Attachment: P7: Create a mutex lock on CacheQuotaClient for protected accessing directory padding file.

Insert a patch for adding a mutex lock to protect the directory lock from accessing from different threads.

Comment 99

[Tom Tung [:tt, :ttung]]

Attachment: P8: Implement few utilis function to access direcotry padding file.

Comment 100

[Tom Tung [:tt, :ttung]]

Attachment: P9: Implement a function to get overall padding size for an origin from cache.sqlite.

Comment 101

[Tom Tung [:tt, :ttung]]

Attachment: P10: Update padding size to the direcotry padding file.

Comment 102

[Tom Tung [:tt, :ttung]]

Attachment: P11: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage.

Comment 103

[Tom Tung [:tt, :ttung]]

Attachment: P12: Get the padding size while QM gets usage from DOM Cache

Comment 104

[Tom Tung [:tt, :ttung]]

Attachment: P13: Get padding from DB when there is no directroy padding file.

Comment 105

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bfd1e04cd6e5270ffd2181850e5e03642cd4b1a

Comment 106

[Tom Tung [:tt, :ttung]]

Since comment 92 and comment 93 (the way to create padding size; P5), I would like to only send P1 ~ P4 to review first.

Comment 107

[Tom Tung [:tt, :ttung]]

Comment on attachment 8888698 [details] [diff] [review]
P1: Make InternalResponse and CacheResponse be able to keep padding size.

Hi Ben,

Sorry for sending review request when you are in PTO... :( 
But, I think you and Jan are the most appropriate reviewers to review this.

In this bug, I only pad the opaque response by adding virtual padding size on it. So, basically, I update more size to QM while writing body files & SQLite entries and decrease more when removing body files & SQLite entries. 

Besides, to prevent from spending too much time on initializing directories, I add a directory padding file on each DOM Cache directories to avoid QM needs to open "cache.sqlite" when initializing. However, because of QM gets quota client's usage on Quota thread and DOM Cache do most of the things on its IO threads, I add a mutex lock to avoid read/write the same directory files at the same time.

Unfortunately, since I add a table on cache.sqlite to store padding size and make QM add directory file for every existing DOM Cache directory, I need to upgrade both SQLite files. It means we won't support backward compatibility after applying my patch.

This patch is mainly to add padding size attribute on InteranlResponse and CacheResponse to track response's padding.

Comment 108

[Tom Tung [:tt, :ttung]]

Comment on attachment 8887435 [details] [diff] [review]
P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite.

This patch is mainly to upgrade cache.sqlite to add zero padding size for existing opaque responses

Comment 109

[Tom Tung [:tt, :ttung]]

Comment on attachment 8887436 [details] [diff] [review]
P3: Record padding size into cache.sqlite

This patch is mainly to store padding size into cahce.sqlite

Comment 110

[Tom Tung [:tt, :ttung]]

Comment on attachment 8887437 [details] [diff] [review]
P4: Update padding size to the QuotaManager

This patch is mainly to update padding size to QM. I increase the padding size to QM between finishing writing the temporary body file and finalizing the body file. For the timing to decrease the padding size, I choose to do that after DOM Cache deletes the orphaned body file.

Hi Jan,

Could you help me to review this part as well? Thanks!

Comment 111

[Tom Van Goethem]

The thing we're trying to solve here is to make it *sufficiently* hard for an attacker to get the size of a response. More precisely, it should not be possible for an adversary to get a better estimate than with other known (wont-fix?) techniques (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1211669 or just network-based timing attacks). These other techniques can easily differentiate between resources that have ~5kB size difference, so we should aim to prevent better attacks.

As Tom Ritter mentioned: the HMAC approach looks interesting, but I don't think it's feasible in real-world applications because of bogus URL parameters that can be added. Origin-based also won't work, because it would only take a single resource with a fixed size to figure out the padding that's being used.

If I understand correctly, Tom Ritter also suggested not adding a random number and just rounding up. I would advise against that: when there is content on the resource that is reflected from the request (or otherwise under the control of the attacker), the attacker can add padding to the response until it reaches the tipping point of a bucket size. I.e. the attacker would see the response size grow from n * bucketSize to (n+1) * bucketSize. With this, he can compute the exact response size (although this can be application-specific, I think the consequences can be much more severe).

Regarding to having a non-uniform distribution so all buckets would occur equally as often: I think that this is subject to the same issues as I mentioned above, i.e. for a certain amount of reflected content the smallest observed size (including padding) will be (n+1) * bucketSize. This will require slightly more requests from the attacker, but I think it's still feasible. If all buckets have the same probability of being drawn, then the probability of getting the lowest one is 1/numBuckets.
The approach I originally suggested does not suffer as badly from this attack: the closer the response gets to tipping point, the less likely it is that the smallest buckets gets chosen.

Since I am definitely not a statistician, I can't say for certain which approach is the best. But I think it's good to make sure that there's no easy way to determine the exact size of a resource. To avoid any arduous statistical analysis, I've tried to assess approaches by means of simulation, i.e. try to think of the best way to attack the defence and quickly implement that to see how well it works. This is the code I wrote for the suggested defence: https://gist.github.com/tomvangoethem/eaf9312401ab6098282eb7631bff8547

Comment 112

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8888698 [details] [diff] [review]
P1: Make InternalResponse and CacheResponse be able to keep padding size.

Review of attachment 8888698 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/cache/TypeUtils.cpp
@@ +203,5 @@
> +
> +  if (aIn.Type() == ResponseType::Opaque) {
> +    aOut.paddingSize() = aIn.GetPaddingSize();
> +  } else {
> +    aOut.paddingSize() = InternalResponse::UNKNOWN_PADDING_SIZE;

Can we rewrite this as:

  MOZ_DIAGNOSTIC_ASSERT(aIn.Type != ResponseType::Opaque ||
                        aIn.GetPaddingSize() != InternalResponse::UNKNOWN_PADDING_SIZE);
  aOut.paddingSize() = aIn.GetPaddingSize();

@@ +296,5 @@
>      case ResponseType::Default:
>        break;
>      case ResponseType::Opaque:
>        ir = ir->OpaqueResponse();
> +      ir->SetPaddingSize(aIn.paddingSize());

Lets just copy the padding size unconditionally and assert its only non-UNKNOWN if its opaque.  Similar to the proposed snippet above.

::: dom/fetch/InternalResponse.h
@@ +229,5 @@
> +  int64_t
> +  GetPaddingSize()
> +  {
> +    MOZ_ASSERT(Type() == ResponseType::Opaque);
> +    MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE || mPaddingSize >= 0);

Can you make these MOZ_DIAGNOSTIC_ASSERT() please?

@@ +240,5 @@
> +    // Only pad the opaque response.
> +    MOZ_ASSERT(Type() == ResponseType::Opaque);
> +    // An opaque response's padding size cannot be reset once set.
> +    MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE);
> +    MOZ_ASSERT(aPaddingSize >= 0);

MOZ_DIAGNOSTIC_ASSERT() for these as well.  They are all cheap.

@@ +243,5 @@
> +    MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE);
> +    MOZ_ASSERT(aPaddingSize >= 0);
> +
> +    mPaddingSize = aPaddingSize;
> +  }

Also, can you move these implementations into the .cpp file?  I know we have some methods in the header already, but I am encouraging folks to put things in the cpp unless there is measured performance reason to inline in the header.  It makes it easier to find the code and compiles are faster.

Comment 113

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8887435 [details] [diff] [review]
P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite.

Review of attachment 8887435 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/DBSchema.cpp
@@ +148,5 @@
> +const char* const kTableOpaqueResponsePaddings =
> +  "CREATE TABLE opaque_response_paddings ("
> +    "padding_size INTEGER NOT NULL, "
> +    "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE"
> +  ")";

Is there a reason this is a separate table?

I think we can just add nullable padding_size to the entry table.  When NULL this column will take 1 byte per row.

In contrast, storing as a separate table requires storing the entry_id, row length, row ID, foreign key, etc.  If we were sure we would always have many many non-opaque compared to only a few opaque this might give some disk storage gains, but it will not be too large.

I think for now we should just put padding_size inline in the entry table.

Comment 114

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8887436 [details] [diff] [review]
P3: Record padding size into cache.sqlite

Review of attachment 8887436 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping this for now since I requested a DBSchema change in the previous patch.

Comment 115

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8887437 [details] [diff] [review]
P4: Update padding size to the QuotaManager

Review of attachment 8887437 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/FileUtils.cpp
@@ +268,5 @@
> +  int64_t fileSize = 0;
> +  rv = bodyFile->GetFileSize(&fileSize);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  if (*aPaddingSizeOut == /* Unset padding size */ -1) {

Can we use the InternalResponse constant for this instead of the bare -1?

@@ +598,5 @@
>  
>    rv = aFile->Remove( /* recursive */ false);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +  // Size of Marker file is 0

I'm not sure what this comment means...  I guess that if the file being removed is empty it might have a zero size?

::: dom/cache/Manager.cpp
@@ +1032,1 @@
>    int64_t mDeletedPaddingSize;

Maybe add comments for these two explaining a bit.  The mDeletedPaddingSize tracks any pad amount associated with overwritten entries.  The mUpdatedPaddingSize tracks how much pad amount has been added for new entries so that it can be removed if an error occurs.

Right?

Comment 116

[Tom Ritter [:tjr]]

(In reply to Tom Van Goethem from comment #111)
> If I understand correctly, Tom Ritter also suggested not adding a random
> number and just rounding up. I would advise against that: when there is
> content on the resource that is reflected from the request (or otherwise
> under the control of the attacker), the attacker can add padding to the
> response until it reaches the tipping point of a bucket size. I.e. the
> attacker would see the response size grow from n * bucketSize to (n+1) *
> bucketSize. With this, he can compute the exact response size (although this
> can be application-specific, I think the consequences can be much more
> severe).

Okay, having the 'add a random value' makes it harder to perform that attack. You still learn the size as soon as you observe a new bucketsize, but the odds you'll observe that new bucketsize are reduced because of the additional random value. Yes, that makes sense.

> Regarding to having a non-uniform distribution so all buckets would occur
> equally as often: I think that this is subject to the same issues as I
> mentioned above, i.e. for a certain amount of reflected content the smallest
> observed size (including padding) will be (n+1) * bucketSize. This will
> require slightly more requests from the attacker, but I think it's still
> feasible. If all buckets have the same probability of being drawn, then the
> probability of getting the lowest one is 1/numBuckets.
> The approach I originally suggested does not suffer as badly from this
> attack: the closer the response gets to tipping point, the less likely it is
> that the smallest buckets gets chosen.

Hm. I agree, if the attacker controls the content.

If the attacker can control the size of the response, the size is leaked as soon as the attacker can say with confidence "If I add X bytes I will sometimes get the smallest bucketsize A, if I add Y bytes I will never get bucketsize A". 

If the attacker cannot control the size of the response, the size is leaked as soon as the attacker can say with confidence "The smallest bucketsize S occurs with <this> frequency. That means the size is offset into bucket S+3 this amount."

(I think.)

Changing from a uniform distribution to what I'll call the 'equalized' distribution making all bucket occurrences the same would make it _easier_ for the attacker who can control the response length to leak the response size! (I think that's what you said Tom.)

BUT.

Changing from a uniform distribution to a normal distribution seems like it would make the attacker-controlled case take more queries, because it is harder for the attacker to hit the low bucket. It would make the attacker-uncontrolled case take the same amount though - the probability is known, it doesn't matter what it is.


Maybe the correct approach here is to open a new bug to perform a hard analysis on the padding technique, and tweak it based on the outcome of that.

Comment 117

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [PTO, back July 24][:bkelly] from comment #112)

Thanks for the review!

> Comment on attachment 8888698 [details] [diff] [review]
> P1: Make InternalResponse and CacheResponse be able to keep padding size.
> 
> Review of attachment 8888698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: dom/cache/TypeUtils.cpp
> @@ +203,5 @@
> > +
> > +  if (aIn.Type() == ResponseType::Opaque) {
> > +    aOut.paddingSize() = aIn.GetPaddingSize();
> > +  } else {
> > +    aOut.paddingSize() = InternalResponse::UNKNOWN_PADDING_SIZE;
> 
> Can we rewrite this as:
> 
>   MOZ_DIAGNOSTIC_ASSERT(aIn.Type != ResponseType::Opaque ||
>                         aIn.GetPaddingSize() !=
> InternalResponse::UNKNOWN_PADDING_SIZE);
>   aOut.paddingSize() = aIn.GetPaddingSize();

I think you mean this?
MOZ_DIAGNOSTIC_ASSERT(aIn.Type() == ResponseType::Opaque ||
                      aIn.GetPaddingSize() ==
                      InternalResponse::UNKNOWN_PADDING_SIZE);
Note: For InternalResponse -> CachedResponse, there is two conidtions for opaque reponse. The first is that the internalResponse is just fetch from network, and we have not generated padding size for it yet. The second is that the internalResponse had been cached before so it have a "persisted" padding size.

> @@ +296,5 @@
> >      case ResponseType::Default:
> >        break;
> >      case ResponseType::Opaque:
> >        ir = ir->OpaqueResponse();
> > +      ir->SetPaddingSize(aIn.paddingSize());
> 
> Lets just copy the padding size unconditionally and assert its only
> non-UNKNOWN if its opaque.  Similar to the proposed snippet above.

Sure.

So, I'll write an assertion like below.
Note: For CacheResponse -> InteranlResponse, opaque response won't have the unknown padding size. We should have generated a padding size for it.
MOZ_DIAGNOSTIC_ASSERT((aIn.type() == ResponseType::Opaque) !=
                      (aIn.GetPaddingSize() ==
                       InternalResponse::UNKNOWN_PADDING_SIZE));

> ::: dom/fetch/InternalResponse.h
> @@ +229,5 @@
> > +  int64_t
> > +  GetPaddingSize()
> > +  {
> > +    MOZ_ASSERT(Type() == ResponseType::Opaque);
> > +    MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE || mPaddingSize >= 0);
> 
> Can you make these MOZ_DIAGNOSTIC_ASSERT() please?

Sure.

> @@ +240,5 @@
> > +    // Only pad the opaque response.
> > +    MOZ_ASSERT(Type() == ResponseType::Opaque);
> > +    // An opaque response's padding size cannot be reset once set.
> > +    MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE);
> > +    MOZ_ASSERT(aPaddingSize >= 0);
> 
> MOZ_DIAGNOSTIC_ASSERT() for these as well.  They are all cheap.

Sure

> @@ +243,5 @@
> > +    MOZ_ASSERT(mPaddingSize == UNKNOWN_PADDING_SIZE);
> > +    MOZ_ASSERT(aPaddingSize >= 0);
> > +
> > +    mPaddingSize = aPaddingSize;
> > +  }
> 
> Also, can you move these implementations into the .cpp file?  I know we have
> some methods in the header already, but I am encouraging folks to put things
> in the cpp unless there is measured performance reason to inline in the
> header.  It makes it easier to find the code and compiles are faster.

Sure, I see.

Comment 118

[Tom Tung [:tt, :ttung]]

Attachment: P1: Make InternalResponse and CacheResponse be able to keep padding size. r=bkelly.

Comment 119

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [PTO, back July 24][:bkelly] from comment #113)
> ::: dom/cache/DBSchema.cpp
> @@ +148,5 @@
> > +const char* const kTableOpaqueResponsePaddings =
> > +  "CREATE TABLE opaque_response_paddings ("
> > +    "padding_size INTEGER NOT NULL, "
> > +    "entry_id INTEGER NOT NULL REFERENCES entries(id) ON DELETE CASCADE"
> > +  ")";
> 
> Is there a reason this is a separate table?

I thought it better to store it in a seperate table since we only need to store padding size for opaque response. It takes less space if there is not so many opaque resposnes.
 
> I think we can just add nullable padding_size to the entry table.  When NULL
> this column will take 1 byte per row.
> 
> In contrast, storing as a separate table requires storing the entry_id, row
> length, row ID, foreign key, etc.  If we were sure we would always have many
> many non-opaque compared to only a few opaque this might give some disk
> storage gains, but it will not be too large.
> 
> I think for now we should just put padding_size inline in the entry table.

I see. I'll change the wey to store padding size.

Comment 120

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [PTO, back July 24][:bkelly] from comment #115)
> ::: dom/cache/FileUtils.cpp
> @@ +268,5 @@
> > +  int64_t fileSize = 0;
> > +  rv = bodyFile->GetFileSize(&fileSize);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +
> > +  if (*aPaddingSizeOut == /* Unset padding size */ -1) {
> 
> Can we use the InternalResponse constant for this instead of the bare -1?

Sure

> @@ +598,5 @@
> >  
> >    rv = aFile->Remove( /* recursive */ false);
> >    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> > +  // Size of Marker file is 0
> 
> I'm not sure what this comment means...  I guess that if the file being
> removed is empty it might have a zero size?

Oops, that is self-note for reminding me there is a case that we delete a zero size file (marker file). 
I'll remove that.

> ::: dom/cache/Manager.cpp
> @@ +1032,1 @@
> >    int64_t mDeletedPaddingSize;
> 
> Maybe add comments for these two explaining a bit.  The mDeletedPaddingSize
> tracks any pad amount associated with overwritten entries.  The
> mUpdatedPaddingSize tracks how much pad amount has been added for new
> entries so that it can be removed if an error occurs.
> 
> Right?

Yes, you are right. I'll add comments for them to make its readable

Comment 121

[Tom Tung [:tt, :ttung]]

ni myself for comment #111 and comment #116

Comment 122

[Tom Tung [:tt, :ttung]]

Attachment: P1: Make InternalResponse and CacheResponse be able to keep padding size. r=bkelly.

Move the assertions to getter/setter function.

Comment 123

[Tom Tung [:tt, :ttung]]

Attachment: P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite.

Addressed the comment (add a column instead of a new table).

Comment 124

[Tom Tung [:tt, :ttung]]

Attachment: P3: Record padding size into cache.sqlite

Resend the review request since I have addressed the comment for P2

Comment 125

[Tom Tung [:tt, :ttung]]

Attachment: P4: Update padding size to the QuotaManager

Address comment, so resend the review request.

Comment 126

[Tom Tung [:tt, :ttung]]

(In reply to Tom Ritter [:tjr] from comment #116)
> Maybe the correct approach here is to open a new bug to perform a hard
> analysis on the padding technique, and tweak it based on the outcome of that.

I file the bug 1383656 for following up. I think maybe we could implement a basic way to generate the padding in this bug and tweak the value/distribution in that bug later?

Comment 127

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8889316 [details] [diff] [review]
P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite.

Review of attachment 8889316 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 128

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8889319 [details] [diff] [review]
P3: Record padding size into cache.sqlite

Review of attachment 8889319 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/DBSchema.cpp
@@ -630,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
>    rv = state->Execute();
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> -

nit: Can you leave this blank line?

@@ +1451,5 @@
> +    // It's possible to have null padding size for non-opaque response
> +    rv = state->GetIsNull(3, &isNull);
> +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +    if (!isNull) {

Should we set the value to UNKNOWN_SIZE if its NULL?  Or to zero?  I think we should probably set it to something in case the caller forgets to initialize it.

@@ +2052,5 @@
> +    int64_t paddingSize = 0;
> +    rv = state->GetInt64(6, &paddingSize);
> +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +    aSavedResponseOut->mValue.paddingSize() = paddingSize;

Can we add a MOZ_DIAGNOSTIC_ASSERT() here that paddingSize is >= 0?

::: dom/cache/Manager.cpp
@@ +786,5 @@
>        }
>      }
>  
> +    // XXXtt: Write .padding file to the cache directory
> +    // UpdateDirectoryFile(mDBDir);

Is this handled in a later patch?

Comment 129

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #128)
> ::: dom/cache/DBSchema.cpp
> @@ -630,5 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> >    rv = state->Execute();
> >    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > -
> 
> nit: Can you leave this blank line?

Sure

> @@ +1451,5 @@
> > +    // It's possible to have null padding size for non-opaque response
> > +    rv = state->GetIsNull(3, &isNull);
> > +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +
> > +    if (!isNull) {
> 
> Should we set the value to UNKNOWN_SIZE if its NULL?  Or to zero?  I think
> we should probably set it to something in case the caller forgets to
> initialize it.

Sorry, I'm not sure what you mean. I guess you were talking about what should I do in ReadResponse()?

In ReadResponse(), if the column "response_padding_size" is null, it's set to UNKNOWN_SIZE. There is an assertion to ensure column is null only when the reading respnose is not type opaque.

In DeleteEntries(), I don't think I need to set any value. I calculate the overall padding size to be removed and pass them out in order to decrease it from QM (P4).

> @@ +2052,5 @@
> > +    int64_t paddingSize = 0;
> > +    rv = state->GetInt64(6, &paddingSize);
> > +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +
> > +    aSavedResponseOut->mValue.paddingSize() = paddingSize;
> 
> Can we add a MOZ_DIAGNOSTIC_ASSERT() here that paddingSize is >= 0?

Sure.

> ::: dom/cache/Manager.cpp
> @@ +786,5 @@
> >        }
> >      }
> >  
> > +    // XXXtt: Write .padding file to the cache directory
> > +    // UpdateDirectoryFile(mDBDir);
> 
> Is this handled in a later patch?

Yes, I implement it in P10

Comment 130

[Tom Tung [:tt, :ttung]]

Attachment: P1: Make InternalResponse and CacheResponse be able to keep padding size. r=bkelly.

Update P1 since fail on try (I should put assertion for setting padding size after set the response type)

Comment 131

[Tom Tung [:tt, :ttung]]

Attachment: P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite. r=bkelly

Comment 132

[Tom Tung [:tt, :ttung]]

Attachment: P3: Record padding size into cache.sqlite. r=bkelly.

Address comment except the quesion I put in comment 129

Comment 133

[Tom Tung [:tt, :ttung]]

Attachment: P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory.

Update patches for padding file

Comment 134

[Tom Tung [:tt, :ttung]]

Attachment: P7: Create a mutex lock on CacheQuotaClient for protected accessing directory padding file.

Comment 135

[Tom Tung [:tt, :ttung]]

Attachment: P8: Implement few utils functions to access direcotry padding file.

Comment 136

[Tom Tung [:tt, :ttung]]

Attachment: P9: Implement a function to get overall padding size for an origin from cache.sqlite.

Comment 137

[Tom Tung [:tt, :ttung]]

Attachment: P10: Update padding size to the direcotry padding file.

Comment 138

[Tom Tung [:tt, :ttung]]

Attachment: P11: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage.

Comment 139

[Tom Tung [:tt, :ttung]]

Attachment: P12: Get the padding size while QM gets usage from DOM Cache

Comment 140

[Tom Tung [:tt, :ttung]]

Attachment: P13: Get padding from DB when there is no directroy padding file.

Comment 141

[Tom Tung [:tt, :ttung]]

Attachment: P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory.

Fix for try failure (mozilla::DebugOnly<bool> should use with MOZ_ASSERT)

Comment 142

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #129)
> In ReadResponse(), if the column "response_padding_size" is null, it's set
> to UNKNOWN_SIZE. There is an assertion to ensure column is null only when
> the reading respnose is not type opaque.
> 
> In DeleteEntries(), I don't think I need to set any value. I calculate the
> overall padding size to be removed and pass them out in order to decrease it
> from QM (P4).

Can you at least set it to zero at the top of the method here?  So that way if nothing with padding gets deleted zero is always returned.  Otherwise we are depending on the caller initializing to zero first.

Comment 143

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #142)
> Can you at least set it to zero at the top of the method here? So that way
> if nothing with padding gets deleted zero is always returned. Otherwise we
> are depending on the caller initializing to zero first.

Thanks! I see. 

I need to take care case [1] when the deleting entries are too big as well.

[1] http://searchfox.org/mozilla-central/source/dom/cache/DBSchema.cpp#1359-1360

Comment 144

[Tom Tung [:tt, :ttung]]

Attachment: P3: Record padding size into cache.sqlite. r=bkelly.

Update patch

Comment 145

[Tom Tung [:tt, :ttung]]

Attachment: P7: Create a mutex lock on CacheQuotaClient for protected accessing directory padding file.

Comment 146

[Tom Tung [:tt, :ttung]]

Comment on attachment 8889857 [details] [diff] [review]
P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory.

Hi Ben, Jan,

The following patches are the implementation of the directory padding file. The file is stores in each DOMCache base directory (profile/storage/origin/cache/.padding). There are two types of padding file: temporary_file and file (.padding-tmp .padding) to make sure the content inside the file is correct (If the temporary file exists, it means the content inside the file is wrong). Since the current cache operations live in cache IO threads (per manager/origin) and CacheQuotaClient calculates the overall usage in Quota IO thread, I implement a mutex lock to protect the padding file, and it is held by CacheQuotaClient. The reason is CacheQuotaClient is initialized in [1], and the QM holds it. This ensures the mutex lock must live earlier and longer than the every executing cache runnable. 

[1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#3617

P6: For the existing cache directroy, I implement a major upgrade for QM from v2 to v3 to create a padding file with the 0 padding size inside.

P7: Create the mutex lock in CacheQuotaClient, so that we can protect the padding file from multi-thread access

P8: Implement utility functions in FileUtils.cpp to read/write/delete/check the padding file.

P9: Implement a function to get overall padding size, so that we can restore the padding file.

P10: The main logic for accessing padding file on cache operations (cache.put/cache.delete/...).

P11: Add thread assertion on CacheQuotaClient to ensure its functions run as expected

P12: Get the padding size in initOrigin() and getUsageForOrigin(). Besides, I rewrite the implementation inside, since it's easier to restore padding file in initOrigin() than getUsageForOrigin(). The reason is that is also guaranteed to be executed before every cache operations. 

P13: Access the database in initOrigin(), so that we can restore the file.

Comment 147

[Tom Tung [:tt, :ttung]]

Attachment: P8: Implement few utility functions to access direcotry padding file.

Comment 148

[Tom Tung [:tt, :ttung]]

Attachment: P10: Update padding size to the direcotry padding file.

Comment 149

[Tom Tung [:tt, :ttung]]

Attachment: P11: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage.

Comment 150

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8890222 [details] [diff] [review]
P10: Update padding size to the direcotry padding file.

Review of attachment 8890222 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/QuotaClient.h
@@ +18,5 @@
>  already_AddRefed<quota::Client>
>  CreateQuotaClient();
>  
> +Mutex&
> +GetDirPaddingFileMutexLock();

I think its really dangerous to expose a mutex like this.  Its too easy for it to start getting used in a lock that triggers a deadlock.  Its also really hard to audit mutex usage when the locks are spread across many files.

Can you instead change this to a few atomic operations like:

1. RemovePaddingFiles();
2. template<typename Callable> UpdatePadding(Callable aCommitHook)
3. DecreasePadding()

The template callable thing is to let you perform the database Commit() call from within the locked UpdatePadding() method.  So you would do something like:

  UpdateCallable([state]() { return state->Commit() });

And in UpdateCallable it does the:

  nsresult rv = aCommitHook();
  if (NS_WARN_IF(NS_FAILED(rv)) {
    // delete temp file
  }

This will keep all the locking in one QuotaClient.

Comment 151

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8889857 [details] [diff] [review]
P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory.

Review of attachment 8889857 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me, but this is mostly QM which I don't know too much about.

Comment 152

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8890218 [details] [diff] [review]
P8: Implement few utility functions to access direcotry padding file.

Review of attachment 8890218 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

In general try to avoid "exists" checks.  Its better to just try the read/delete to avoid additional fstat system calls.

::: dom/cache/FileUtils.cpp
@@ +745,5 @@
> +nsresult
> +LockedMaybeUpdateDirectoryPaddingFile(nsIFile* aBaseDir,
> +                                      mozIStorageConnection* aConn,
> +                                      const int64_t aIncreaceSize,
> +                                      const int64_t aDecreaceSize,

nit: s/Increace/Increase/g and s/Decreace/Decrease/g

@@ +765,5 @@
> +    return rv;
> +  }
> +
> +  int64_t currentPaddingSize = 0;
> +  if (!DirectoryPaddingFileExists(aBaseDir, DirPaddingFile::FILE) ||

I don't think we need this Exists check, do we?  It seems like we could just try to read and let it fail if its not there.

@@ +787,5 @@
> +  }
> +
> +  MOZ_DIAGNOSTIC_ASSERT(currentPaddingSize >= 0);
> +
> +  rv = LockedDirectoryPaddingWrite(aBaseDir, DirPaddingFile::TMP_FILE,

I think the name of this method is confusing since this only writes to the TMP file.  You still need to call finalize after this.  Is there a reason we don't do that here?  At a minimum we should probably make the name of the method more obviously about writing Tmp and document how its intended to be used.

@@ +872,5 @@
> +  }
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> +
> +  rv = file->Remove( /* recursive */ false);
> +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

I think we should ignore errors where the file is already removed.  That way we don't need to have an Exists() check before calling this.

::: dom/cache/FileUtils.h
@@ +84,5 @@
>  DecreaseUsageForQuotaInfo(const QuotaInfo& aQuotaInfo,
>                            const int64_t& aUpdatingSize);
> +
> +bool
> +DirectoryPaddingFileExists(nsIFile* aBaseDir, DirPaddingFile aPaddingFileType);

Maybe add a block comment in here describing the padding file structure and API usage.

Comment 153

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8889836 [details] [diff] [review]
P9: Implement a function to get overall padding size for an origin from cache.sqlite.

Review of attachment 8889836 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/FileUtils.cpp
@@ +773,5 @@
>      rv = LockedDirectoryPaddingDeleteFile(aBaseDir, DirPaddingFile::FILE);
>      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +    rv = db::FindOverallPaddingSize(aConn, &currentPaddingSize);
> +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

We don't need to add the aIncreaseSize or aDecreaseSize here because its already encompassed within the database?  A comment might be good to describe that.

Comment 154

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8890223 [details] [diff] [review]
P11: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage.

Review of attachment 8890223 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/QuotaClient.cpp
@@ +294,5 @@
>  
>  Mutex&
>  GetDirPaddingFileMutexLock()
>  {
> +  // Should only be accessed in Cache IO theads and Quota IO thread.

We can at least MOZ_ASSERT(!NS_IsMainThread()).

Comment 155

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8889839 [details] [diff] [review]
P12: Get the padding size while QM gets usage from DOM Cache

Review of attachment 8889839 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping review on this since I r-'d a previous patch that affects this I think.

Comment 156

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8889840 [details] [diff] [review]
P13: Get padding from DB when there is no directroy padding file.

Review of attachment 8889840 [details] [diff] [review]:
-----------------------------------------------------------------

I have a question here and dropping flag since I have an earlier r-.

::: dom/cache/DBAction.cpp
@@ +260,5 @@
>    nsresult rv = RunSyncWithDBOnTarget(aQuotaInfo, aDBDir, aConn);
>    aResolver->Resolve(rv);
>  }
>  
> +InitDBAction::InitDBAction(const nsACString& aGroup, const nsACString& aOrigin)

Why do we need a new Action class here?  Are you just using it to get the Connection?

Comment 157

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #152)
Thanks for the review!

> In general try to avoid "exists" checks.  Its better to just try the
> read/delete to avoid additional fstat system calls.

Learn a lesson! I will remove unnecessary exists checks.

> ::: dom/cache/FileUtils.cpp
> @@ +745,5 @@
> > +nsresult
> > +LockedMaybeUpdateDirectoryPaddingFile(nsIFile* aBaseDir,
> > +                                      mozIStorageConnection* aConn,
> > +                                      const int64_t aIncreaceSize,
> > +                                      const int64_t aDecreaceSize,
> 
> nit: s/Increace/Increase/g and s/Decreace/Decrease/g

Noted

> @@ +765,5 @@
> > +    return rv;
> > +  }
> > +
> > +  int64_t currentPaddingSize = 0;
> > +  if (!DirectoryPaddingFileExists(aBaseDir, DirPaddingFile::FILE) ||
> 
> I don't think we need this Exists check, do we?  It seems like we could just
> try to read and let it fail if its not there.

Yes, you are right. I'll remove this exists check.

> @@ +787,5 @@
> > +  }
> > +
> > +  MOZ_DIAGNOSTIC_ASSERT(currentPaddingSize >= 0);
> > +
> > +  rv = LockedDirectoryPaddingWrite(aBaseDir, DirPaddingFile::TMP_FILE,
> 
> I think the name of this method is confusing since this only writes to the
> TMP file.  You still need to call finalize after this.  Is there a reason we
> don't do that here?  At a minimum we should probably make the name of the
> method more obviously about writing Tmp and document how its intended to be
> used.

So, I need to finalize the tmp file after the db transcation is commit.

I try to make this function be able to deal with the following two case at the same time. Maybe I need to seperate them to two functions?

Case 1: When exceuting cache actions (cache.put/cache.delete/...), we want to have a way to know the opaque response is added into db or not (roll back).

Timeline: CreateDBTranscation & ModifyDBEntries -(A)> WriteTmpPaddingFile -(B)> CommitTranscation -(C)> FinalizePaddingFile 

If failures happen arround the arrow A, the content of db and padding file is not modified. Thus, it's ok.
If failures happen arround the arrow B, the content of db will be rolled back which should have the same padding size as padding file since we only write to temporary file.
If failures happen arround the arrow C, the content of db is updated but padding file is not. Thus, we could utilize checking whether the temporary file is existed or not to decide if we need to restore the padding file.

Case 2: Resotre
In this case we don't need to consider db case, so we can write the padding size into the padding file directly.

> @@ +872,5 @@
> > +  }
> > +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> > +
> > +  rv = file->Remove( /* recursive */ false);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
> I think we should ignore errors where the file is already removed.  That way
> we don't need to have an Exists() check before calling this.

That's a good idea. I'll do that.

> ::: dom/cache/FileUtils.h
> @@ +84,5 @@
> >  DecreaseUsageForQuotaInfo(const QuotaInfo& aQuotaInfo,
> >                            const int64_t& aUpdatingSize);
> > +
> > +bool
> > +DirectoryPaddingFileExists(nsIFile* aBaseDir, DirPaddingFile aPaddingFileType);
> 
> Maybe add a block comment in here describing the padding file structure and
> API usage.

Sure.

Comment 158

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #153)
> Comment on attachment 8889836 [details] [diff] [review]
> P9: Implement a function to get overall padding size for an origin from
> cache.sqlite.
> 
> Review of attachment 8889836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/FileUtils.cpp
> @@ +773,5 @@
> >      rv = LockedDirectoryPaddingDeleteFile(aBaseDir, DirPaddingFile::FILE);
> >      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> >  
> > +    rv = db::FindOverallPaddingSize(aConn, &currentPaddingSize);
> > +    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
> We don't need to add the aIncreaseSize or aDecreaseSize here because its
> already encompassed within the database?  A comment might be good to
> describe that.

Yes, I'll add comment for that.

Comment 159

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #150)
> Comment on attachment 8890222 [details] [diff] [review]
> P10: Update padding size to the direcotry padding file.
> 
> Review of attachment 8890222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/QuotaClient.h
> @@ +18,5 @@
> >  already_AddRefed<quota::Client>
> >  CreateQuotaClient();
> >  
> > +Mutex&
> > +GetDirPaddingFileMutexLock();
> 
> I think its really dangerous to expose a mutex like this.  Its too easy for
> it to start getting used in a lock that triggers a deadlock.  Its also
> really hard to audit mutex usage when the locks are spread across many files.
> 
> Can you instead change this to a few atomic operations like:
> 
> 1. RemovePaddingFiles();
> 2. template<typename Callable> UpdatePadding(Callable aCommitHook)
> 3. DecreasePadding()
> 
> The template callable thing is to let you perform the database Commit() call
> from within the locked UpdatePadding() method.  So you would do something
> like:
> 
>   UpdateCallable([state]() { return state->Commit() });
> 
> And in UpdateCallable it does the:
> 
>   nsresult rv = aCommitHook();
>   if (NS_WARN_IF(NS_FAILED(rv)) {
>     // delete temp file
>   }
> 
> This will keep all the locking in one QuotaClient.

This way is much better! Really learn a lesson from this! I'll try to implement it. Thanks!

Comment 160

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #156)
> Comment on attachment 8889840 [details] [diff] [review]
> P13: Get padding from DB when there is no directroy padding file.
> 
> Review of attachment 8889840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a question here and dropping flag since I have an earlier r-.
> 
> ::: dom/cache/DBAction.cpp
> @@ +260,5 @@
> >    nsresult rv = RunSyncWithDBOnTarget(aQuotaInfo, aDBDir, aConn);
> >    aResolver->Resolve(rv);
> >  }
> >  
> > +InitDBAction::InitDBAction(const nsACString& aGroup, const nsACString& aOrigin)
> 
> Why do we need a new Action class here?  Are you just using it to get the
> Connection?

I want to open a db connecntion on Quota IO thread if the padding file is missing when the QuotaManger calculates overall usage in EnsureOriginIsInitialized() (it will only be called for the first time we init the origin). It sounds dangerous but it should be safe, because this must be called before executing any cache actions.

I found there is a private function OpenConnection in DBAction. I'm not sure if it's a good idea to expose it since we cannot open a connection in different threads at a same time. So, I try to wirte a helper class to get the connection and assert it to be on Quota IO thread only.

Comment 161

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Tom Tung [:tt] from comment #159)
> This way is much better! Really learn a lesson from this! I'll try to
> implement it. Thanks!

FWIW, I think this approach might be called "Inversion of Control":

https://en.wikipedia.org/wiki/Inversion_of_control

(In reply to Tom Tung [:tt] from comment #160)
> I want to open a db connecntion on Quota IO thread if the padding file is
> missing when the QuotaManger calculates overall usage in
> EnsureOriginIsInitialized() (it will only be called for the first time we
> init the origin). It sounds dangerous but it should be safe, because this
> must be called before executing any cache actions.
> 
> I found there is a private function OpenConnection in DBAction. I'm not sure
> if it's a good idea to expose it since we cannot open a connection in
> different threads at a same time. So, I try to wirte a helper class to get
> the connection and assert it to be on Quota IO thread only.

Ok, so I think you can just have a function that creates a Connection.  It doesn't need to be an Action.  The Action stuff is intended to run async on the Context object, so its a bit confusing to create one but not dispatch it.  You can factor out common routines between the current DBAction and your number function.

Also note, its safe to have multiple connections to the same database in different threads.  You just can't share a single Connection object between threads.

Comment 162

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #161)
> FWIW, I think this approach might be called "Inversion of Control":
> 
> https://en.wikipedia.org/wiki/Inversion_of_control

Thanks! I only heard "dependency injection", but it always hard for me to use it nicely. :(
I need to practice harder...

> Ok, so I think you can just have a function that creates a Connection.  It
> doesn't need to be an Action.  The Action stuff is intended to run async on
> the Context object, so its a bit confusing to create one but not dispatch
> it.  You can factor out common routines between the current DBAction and
> your number function.
> 
> Also note, its safe to have multiple connections to the same database in
> different threads.  You just can't share a single Connection object between
> threads.

Just make usre I understand correctly.

I skim through the document related to multithread [1] in SQLite and implementation of mozStorageService. There are three threading modes and the default mode is Serialized mode. And, It seems we set the thread mode to Serialized mode [2]. It means I don't need to worry about data races on database because the database is protected by mutex lock implemented by sqlite. Thus, I guess I can just create a database connection when restoring padding file on Quota IO thread on any time.

There is an API on preference for users to see the current usage for origins, and it maybe call in any time. I used to worry about causing data race on db, but right now I think I can just create a connection to restore file when the file is missing?

[1] https://sqlite.org/threadsafe.html
[2] http://searchfox.org/mozilla-central/source/db/sqlite3/src/moz.build#37-40

Comment 163

[Tom Tung [:tt, :ttung]]

Attachment: P8: Implement few utility functions to access direcotry padding file.

Comment 164

[Tom Tung [:tt, :ttung]]

Attachment: P10: Update padding size to the direcotry padding file.

Comment 165

[Tom Tung [:tt, :ttung]]

Attachment: P11: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage.

Comment 166

[Tom Tung [:tt, :ttung]]

Attachment: P12: Get the padding size while QM gets usage from DOM Cache

Comment 167

[Tom Tung [:tt, :ttung]]

Attachment: P13: Get padding from DB when there is no directroy padding file.

Comment 168

[Tom Tung [:tt, :ttung]]

Attachment: P8: Implement few utility functions to access direcotry padding file.

Hi Ben,

I address your comment, but I also change the way to use utility functions in FileUtilis.cpp. Thus, I'd like to send this patch to review again.

Comment 169

[Tom Tung [:tt, :ttung]]

Comment on attachment 8891276 [details] [diff] [review]
P10: Update padding size to the direcotry padding file.

Address comment, so send it to review again! (Hope that I understand IoC clearly...)

Comment 170

[Tom Tung [:tt, :ttung]]

Comment on attachment 8891281 [details] [diff] [review]
P12: Get the padding size while QM gets usage from DOM Cache

If we can access DB on the different thread at the same time, we don't need to distingulish initOriginUsage() and GetUsageForOrigin(). Just restore padding file when it's missing or failure happens.

Comment 171

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8891285 [details] [diff] [review]
P8: Implement few utility functions to access direcotry padding file.

Review of attachment 8891285 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/FileUtils.cpp
@@ +821,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv)) || temporaryFileExisted) {
> +    // Fail to read padding size from the dir padding file, so try to restore.
> +    if (rv != NS_ERROR_FILE_NOT_FOUND &&
> +        rv != NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
> +      rv = LockedDirectoryPaddingDeleteFile(aBaseDir, DirPaddingFile::FILE);

Maybe add a comment that we don't need to delete the temp file because we're going to overwrite it below anyway.

::: dom/cache/QuotaClient.h
@@ +19,5 @@
>  CreateQuotaClient();
>  
> +/**
> + * The following functions are used to access the directory padding file. The
> + * directory padding file is lived in DOM Cache base directory

s/is lived/lives/g

@@ +42,5 @@
> +MaybeUpdatePaddingFile(nsIFile* aBaseDir,
> +                       mozIStorageConnection* aConn,
> +                       const int64_t aIncreaseSize,
> +                       const int64_t aDecreaseSize,
> +                       Callable aCommitHook);

Please document that the aCommitHook argument will be invoked while a lock is held.  Callers should be careful not to pass a hook that might lock on something else and trigger a deadlock.

Comment 172

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8891276 [details] [diff] [review]
P10: Update padding size to the direcotry padding file.

Review of attachment 8891276 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/QuotaClient.cpp
@@ +80,5 @@
> +
> +  static CacheQuotaClient*
> +  Get()
> +  {
> +    return sInstance;

This can happen on any thread?  Are their guarantees in QM that we won't destroy the Client while its still active?  Otherwise this is not thread safe.

@@ +245,5 @@
>      return rv;
>    }
>  
> +  mozilla::Mutex&
> +  GetDirPaddingFileMutexLock()

Its better that you are using this in the QuotaClient.cpp routines, but I still don't like having a public getter for the mutex.

Can you instead maybe make methods on CacheQuotaClient itself and have the static methods forward to it.  Something like:

  static void
  CacheQuotaClient::Foo()
  {
    RefPtr<CacheQuotaClient> ref = CacheQuotaClient::Get();
    ref->FooInternal();
  }

And then just use the lock internally.

@@ +291,5 @@
>  {
>    MOZ_DIAGNOSTIC_ASSERT(aBaseDir);
>  
> +  MOZ_DIAGNOSTIC_ASSERT(CacheQuotaClient::Get());
> +  MutexAutoLock lock(CacheQuotaClient::Get()->GetDirPaddingFileMutexLock());

I think you probably want to hold a strong ref to the CacheQuotaClient to ensure it doesn't get destroyed while its in use here.

Comment 173

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8891281 [details] [diff] [review]
P12: Get the padding size while QM gets usage from DOM Cache

Review of attachment 8891281 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/QuotaClient.cpp
@@ +129,5 @@
>      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +    int64_t paddingSize = 0;
> +    {
> +      // If the tempoary file still exists after locking, it means the pervious

s/pervious/previous/g

Comment 174

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8891282 [details] [diff] [review]
P13: Get padding from DB when there is no directroy padding file.

Review of attachment 8891282 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/QuotaClient.cpp
@@ +337,5 @@
> +  int64_t paddingSize = 0;
> +
> +  // If the tempoary file still exists after locking, it means the pervious
> +  // action fails, so restore the padding file.
> +  CacheQuotaClient* cacheQuotaClient = CacheQuotaClient::Get();

Please use a strong ref here.

Comment 175

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #172)
> Comment on attachment 8891276 [details] [diff] [review]
> P10: Update padding size to the direcotry padding file.
> 
> Review of attachment 8891276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/QuotaClient.cpp
> @@ +80,5 @@
> > +
> > +  static CacheQuotaClient*
> > +  Get()
> > +  {
> > +    return sInstance;
> 
> This can happen on any thread?  Are their guarantees in QM that we won't
> destroy the Client while its still active?  Otherwise this is not thread
> safe.

There are two sitautions that we will get this instance to get the mutex lock. One is that QM get the current usage by Quota Clients an the other is Cache actions need to modify the padding file. 

The CacheQuotaClient is initialized in QuotaManager::Init() which is called just after the QuotaManager is created[1]. The QM holds a strong reference for it and not deleted it explicitly [2]. Thus, it lives earlier and longer than any cache actions. Besides, QM won't call it when its nullptr. So, I think it should be thread safe.

[1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#3617
[2] http://searchfox.org/mozilla-central/source/dom/quota/QuotaManager.h#535-537

> 
> @@ +245,5 @@
> >      return rv;
> >    }
> >  
> > +  mozilla::Mutex&
> > +  GetDirPaddingFileMutexLock()
> 
> Its better that you are using this in the QuotaClient.cpp routines, but I
> still don't like having a public getter for the mutex.
> 
> Can you instead maybe make methods on CacheQuotaClient itself and have the
> static methods forward to it.  Something like:
> 
>   static void
>   CacheQuotaClient::Foo()
>   {
>     RefPtr<CacheQuotaClient> ref = CacheQuotaClient::Get();
>     ref->FooInternal();
>   }
> 
> And then just use the lock internally.

Certainly.

> @@ +291,5 @@
> >  {
> >    MOZ_DIAGNOSTIC_ASSERT(aBaseDir);
> >  
> > +  MOZ_DIAGNOSTIC_ASSERT(CacheQuotaClient::Get());
> > +  MutexAutoLock lock(CacheQuotaClient::Get()->GetDirPaddingFileMutexLock());
> 
> I think you probably want to hold a strong ref to the CacheQuotaClient to
> ensure it doesn't get destroyed while its in use here.

Sure.

Comment 176

[Tom Tung [:tt, :ttung]]

Attachment: P10 (v2): Update padding size to the direcotry padding file.

Comment 177

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p10.patch

interdiff patch for P10

Comment 178

[Tom Tung [:tt, :ttung]]

Attachment: P13 (v2): Get padding from DB when there is no directroy padding file.

Since the logic in P10 is changed (lock the mutex lock inside the CacheQuotaClient). Rather than extract a function out to open a db connection and restore padding file, I rewrite it in the original function. Besides, I add a check to avoid restore the padding file again if there is a cache action restore it on unlock.

Comment 179

[Tom Tung [:tt, :ttung]]

Attachment: P10 (v2): Update padding size to the direcotry padding file.

Update patch for changing ref counting to thread safe and adding few assertions.

So, I moved the mutex lock's helper functions into class CacheQuotaClient. Besides, I decided to remove the initPaddingFile(), since initPaddingFile() is only called by CacheQuotaClient::UpgradeStorageFrom2_0To3_0(). Moreover, I move part of code in LockedMaybeUpdatePaddingFile() to avoid from holding mutex when we don't need to update padding file.

I'll provide interdiff patch later.

Comment 180

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p10.patch

interdiff patch for P10

Comment 181

[Tom Tung [:tt, :ttung]]

Comment on attachment 8891910 [details] [diff] [review]
P13 (v2): Get padding from DB when there is no directroy padding file.

Resend this to review since I move the static function to CacheQuotaClient and recheck the temporary file after relock.

Comment 182

[Tom Tung [:tt, :ttung]]

Attachment: P14: Add a test to verify QuotaManager upgrade from version 2 to version 3.

Hi Ben, Jan,

This patch is for verifying QM upgrade (P6).

Comment 183

[Tom Tung [:tt, :ttung]]

Attachment: P14: Add a test to verify QuotaManager upgrade from version 2 to version 3.

Update patch for correcting a typo and indentation error.

Comment 184

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8891910 [details] [diff] [review]
P13 (v2): Get padding from DB when there is no directroy padding file.

Review of attachment 8891910 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/QuotaClient.cpp
@@ +150,5 @@
> +        quotaInfo.mOrigin = aOrigin;
> +        {
> +          // Unlock here, since it takes not a short time to open a db
> +          // connection.
> +          MutexAutoUnlock autoUnlock(mDirPaddingFileMutex);

Is this really necessary?  I think it would be safer to keep it locked even when reading from the database.  In theory this is not going to happen very often, right?

Comment 185

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8892383 [details] [diff] [review]
P14: Add a test to verify QuotaManager upgrade from version 2 to version 3.

Review of attachment 8892383 [details] [diff] [review]:
-----------------------------------------------------------------

rs+

Comment 186

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p1.patch

Hi Ben,

During local testing, I found I didn't copy the padding size if I clone() the response by printing the log message. I put my test script below:
```
caches.open(name)
  .then(c => fetch(url, {mode:"no-cors"})
               .then(r => cache.put(url, r)));
var responsePromise = caches.open(name).then(c => c.match(url));
var response;
responsePromise.then(r => response = r);

caches.open(name).then(c => cache.put(url, response.clone())); // not copy padding size here
```

The reason is that patch P1 copy the padding size from mWrappedResponse, which is always the unknow padding size. Thus, I write this patch to copy padding size earlier in this patch.

Could you help me to review this? Thanks! If it's okay, I'll merge it to P1.

Comment 187

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #184) 
> ::: dom/cache/QuotaClient.cpp
> @@ +150,5 @@
> > +        quotaInfo.mOrigin = aOrigin;
> > +        {
> > +          // Unlock here, since it takes not a short time to open a db
> > +          // connection.
> > +          MutexAutoUnlock autoUnlock(mDirPaddingFileMutex);
> 
> Is this really necessary?  I think it would be safer to keep it locked even
> when reading from the database.  In theory this is not going to happen very
> often, right?

Yes, it should rarely happen. I'll remove this. Thanks!

Comment 188

[Tom Tung [:tt, :ttung]]

Attachment: P13: Get padding from DB when there is no directroy padding file.

Comment 189

[Tom Tung [:tt, :ttung]]

Attachment: P10: Update padding size to the direcotry padding file. r=bkelly.

Comment 190

[Tom Tung [:tt, :ttung]]

In today's storage meeting, Jan and I go through the patch(P4, P6, P14). I put the note below:

For P4,
- Avoid to call getFileSize twice and try to make it use just one mutex lock.
  - Make MaybeUpdateSize -> (Locked)MaybeUpdateSizeInternal()
  - Create two public functions: 
     - MaybeUpdateSize(delta, truncate) : almost small as the current function
     - MaybeIncreaeSize(size, delta)
Note: aTruncate in MaybeUpdateSize() is used to decrease the size.

For P6:
- Fix the comment in Client.h
- use MOZ_ASSERT() in QM for aligning format
- Jan will extract the code in UpgradeStorageFrom1_0To2_0() for searching whole directory out, so that I can resuse it.

Comment 191

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8892761 [details] [diff] [review]
interdiff-p1.patch

Review of attachment 8892761 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalResponse.cpp
@@ +144,5 @@
>  
>    clone->mHeaders = new InternalHeaders(*mHeaders);
> +
> +  // Make sure the clone response will have the same padding size.
> +  clone->mPaddingSize = mPaddingSize;

Ouch.  Sorry I didn't catch this!

Comment 192

[Tom Tung [:tt, :ttung]]

Attachment: P5: Implement a function to generate padding size.

Hi Ben,

I utilize uuid to generate a random number for each opaque response just after fetching it from the network. Then, I pass this number to CacheResponse to generate padding size (round up the random number + file size, and then minus the file size).

Besides, the random number will be passed to response's object, so the cloned response will have the same number which means will have the same padding size.

Could you give some feedback? Thanks!

Comment 193

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p10.patch

Comment 194

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #193)
> Created attachment 8893705 [details] [diff] [review]
> interdiff-p10.patch

Sorry for asking the reveiw again. I find I didn't update the padding file if the marker file exists. I thought if the marker file exists, the temporary padding file exists as well. However, I was wrong. Thus, I recheck the logic here, correct the patches and send the interdiff for review. Thanks!

Comment 195

[Tom Tung [:tt, :ttung]]

Attachment: P15: Add an test to verify opaque response has padding and the usage are still the same between in-memory object in QM and the file-system.

Test for verifying the padding are tracking from both in-memory objects and the file-system(directory padding file)

Comment 196

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8893702 [details] [diff] [review]
P5: Implement a function to generate padding size.

Review of attachment 8893702 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalResponse.cpp
@@ +236,5 @@
> +    return;
> +  }
> +
> +  nsID id;
> +  rv = idGenerator->GenerateUUIDInPlace(&id);

This is pretty wasteful when you are only using part of the UUID.  I think you want to do something like this with only the number of bytes you intend to use:

http://searchfox.org/mozilla-central/source/dom/base/Crypto.cpp#102

Comment 197

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8893705 [details] [diff] [review]
interdiff-p10.patch

Review of attachment 8893705 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: b/dom/cache/Manager.cpp
@@ +112,5 @@
> +      rv = MaybeUpdatePaddingFile(aDBDir, aConn, /* aIncreaceSize */ 0,
> +                                  overallDeletedPaddingSize,
> +                                  [&trans]() mutable { return trans.Commit(); });
> +      // We'll restore padding file below, so just warn here if failure happens.
> +      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }

The comment says you are just warning, but you actually return here.  Is that intentional?

Comment 198

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8893708 [details] [diff] [review]
P15: Add an test to verify opaque response has padding and the usage are still the same between in-memory object in QM and the file-system.

Review of attachment 8893708 [details] [diff] [review]:
-----------------------------------------------------------------

rs+

Comment 199

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #196)
> Comment on attachment 8893702 [details] [diff] [review]
> P5: Implement a function to generate padding size.
> 
> Review of attachment 8893702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fetch/InternalResponse.cpp
> @@ +236,5 @@
> > +    return;
> > +  }
> > +
> > +  nsID id;
> > +  rv = idGenerator->GenerateUUIDInPlace(&id);
> 
> This is pretty wasteful when you are only using part of the UUID.  I think
> you want to do something like this with only the number of bytes you intend
> to use:
> 
> http://searchfox.org/mozilla-central/source/dom/base/Crypto.cpp#102

This one is better! Thanks!

(In reply to Ben Kelly [:bkelly] from comment #197)
> Comment on attachment 8893705 [details] [diff] [review]
> interdiff-p10.patch
> 
> Review of attachment 8893705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> ::: b/dom/cache/Manager.cpp
> @@ +112,5 @@
> > +      rv = MaybeUpdatePaddingFile(aDBDir, aConn, /* aIncreaceSize */ 0,
> > +                                  overallDeletedPaddingSize,
> > +                                  [&trans]() mutable { return trans.Commit(); });
> > +      // We'll restore padding file below, so just warn here if failure happens.
> > +      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
> 
> The comment says you are just warning, but you actually return here.  Is
> that intentional?

Oh, I had planned to do that, but I forgot to do it somehow...
Thanks for catching this! 

BTW, is it okay to not return here?

Comment 200

[Tom Tung [:tt, :ttung]]

Attachment: P5 (v1): Implement a function to generate padding size.

Address feedback!

Comment 201

[Tom Tung [:tt, :ttung]]

Attachment: P4 (v1): Update padding size to the QuotaManager

Address comment #190, so resend the review request

Comment 202

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8894401 [details] [diff] [review]
P5 (v1): Implement a function to generate padding size.

Review of attachment 8894401 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/cache/FileUtils.cpp
@@ +378,5 @@
> +  int64_t randomSize = static_cast<int64_t>(aPaddingInfo);
> +  MOZ_DIAGNOSTIC_ASSERT(INT64_MAX - aBodyFileSize >= randomSize);
> +  randomSize += aBodyFileSize;
> +
> +  return RoundUp(randomSize, kRoundUpNumber) - aBodyFileSize;

Is there a source or explanation for this algorithm?  I don't completely understand it.  A comment might help.

::: dom/fetch/InternalResponse.cpp
@@ +234,5 @@
> +  uint32_t randomNumber = 0;
> +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> +    do_GetService("@mozilla.org/security/random-generator;1");
> +  if (!randomGenerator) {
> +    mPaddingInfo.emplace(PR_Now() % kMaxRandomNumber);

Can we make this method fallible and return an error code here?  I don't think we want to accidentally start returning non-random numbers.

Maybe make GeneratePaddingInfo() return a boolean indicating "success vs failure".  Then make FetchDriver::BeginAndGetFilteredResponse() return nullptr if it fails.  Then make FetchDriver::OnStartRequest() treat nullptr as a failure.

Comment 203

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #202)
> Comment on attachment 8894401 [details] [diff] [review]
> P5 (v1): Implement a function to generate padding size.
> 
> Review of attachment 8894401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/FileUtils.cpp
> @@ +378,5 @@
> > +  int64_t randomSize = static_cast<int64_t>(aPaddingInfo);
> > +  MOZ_DIAGNOSTIC_ASSERT(INT64_MAX - aBodyFileSize >= randomSize);
> > +  randomSize += aBodyFileSize;
> > +
> > +  return RoundUp(randomSize, kRoundUpNumber) - aBodyFileSize;
> 
> Is there a source or explanation for this algorithm?  I don't completely
> understand it.  A comment might help.

I implemented the approach to mitigate the attack in the Github [1]. 

Basically, I generate a random number between 0 and 100 kB. Then, rounding up the sum of the random number and file size to 20kB. And, the padding size will be the result minus the file size. 

I will put a comment in FileUtils.cpp.

[1] https://github.com/whatwg/storage/issues/31

> ::: dom/fetch/InternalResponse.cpp
> @@ +234,5 @@
> > +  uint32_t randomNumber = 0;
> > +  nsCOMPtr<nsIRandomGenerator> randomGenerator =
> > +    do_GetService("@mozilla.org/security/random-generator;1");
> > +  if (!randomGenerator) {
> > +    mPaddingInfo.emplace(PR_Now() % kMaxRandomNumber);
> 
> Can we make this method fallible and return an error code here?  I don't
> think we want to accidentally start returning non-random numbers.
> 
> Maybe make GeneratePaddingInfo() return a boolean indicating "success vs
> failure".  Then make FetchDriver::BeginAndGetFilteredResponse() return
> nullptr if it fails.  Then make FetchDriver::OnStartRequest() treat nullptr
> as a failure.

Sure.

Comment 204

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P1: Make InternalResponse and CacheResponse be able to keep padding size. r=bkelly

Rebase patches and update the commit message.

Comment 205

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite. r=bkelly

Comment 206

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P3: Record padding size into cache.sqlite. r=bkelly

Comment 207

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P4 (v1): Update padding size to the QuotaManager. r=bkelly, r?janv

Sorry for the disturbance. 
I found I forgot to release the quotaObject manually. So, correct it and update commit message as well.

Comment 208

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P5: Implement a function to generate padding size. r=bkelly

Hi Ben,

Although you've r+'d this patch, I still want to let you review it again, the comment for explaining the generating padding size algorithm in FileUtilis.cpp and the code for returning network error in FetchDriver.cpp especially. Thanks!

Comment 209

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, r?janv

Addressed part of comment #190 (except extracting the code for searching directory out), so update the patch and resend the review request

Comment 210

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P7: Create a mutex lock on CacheQuotaClient for protected accessing directory padding file. r=bkelly

Comment 211

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P8: Implement few utility functions to access direcotry padding file. r=bkelly

Comment 212

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P9: Implement a function to get overall padding size for an origin from cache.sqlite. r=bkelly

Comment 213

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P10: Update padding size to the direcotry padding file. r=bkelly

Comment 214

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P11: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage. r=bkelly

Comment 215

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P12: Get the padding size while QM gets usage from DOM Cache. r=bkelly

Comment 216

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P13: Get padding from DB when there is no directroy padding file. r=bkelly

Comment 217

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P15: Add an test to verify opaque response has padding and the usage are still the same between in-memory object in QM and the file-system. r=bkelly

Comment 218

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7450ef0c2b1660ba703efe65f6cbd692290b8771&selectedJob=121576093

Comment 219

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8894776 [details] [diff] [review]
Bug 1290481 - P5: Implement a function to generate padding size. r=bkelly

Review of attachment 8894776 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: dom/cache/FileUtils.cpp
@@ +50,5 @@
> +
> +// The alogrithm for generating padding refers to the mitigation approach in
> +// https://github.com/whatwg/storage/issues/31
> +// First, generate a random number between 0 and 100kB.
> +// Next, round up the sum of random number and response size to 20kB.

nit: s/to 20kb/to the nearest 20kb/g

@@ +51,5 @@
> +// The alogrithm for generating padding refers to the mitigation approach in
> +// https://github.com/whatwg/storage/issues/31
> +// First, generate a random number between 0 and 100kB.
> +// Next, round up the sum of random number and response size to 20kB.
> +// Final, the virtual padding size will be the result minus the response size.

nit: s/Final/Finally/g

::: dom/fetch/FetchDriver.cpp
@@ +597,5 @@
>  
>    // Resolves fetch() promise which may trigger code running in a worker.  Make
>    // sure the Response is fully initialized before calling this.
>    mResponse = BeginAndGetFilteredResponse(response, foundOpaqueRedirect);
> +  if (!mResponse) {

You could make this NS_WARN_IF() as well.  I don't think it can happen due to anything content does.  Its always an internal error AFAICT.

Comment 220

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P5: Implement a function to generate padding size. r=bkelly

Thanks for the review!! Addressed the comment.

Comment 221

[Jan Varga [:janv]]

Comment on attachment 8894774 [details] [diff] [review]
Bug 1290481 - P4 (v1): Update padding size to the QuotaManager. r=bkelly, r?janv

Review of attachment 8894774 [details] [diff] [review]:
-----------------------------------------------------------------

This is too complicated.

I originally meant to do something like this:
1. MaybeUpdateSize() should be renamed to LockedMaybeUpdateSize() and just assert that the lock is acquired
2. MaybeUpdateSize(size, truncate) should be added, and it should call LockedMaybeUpdateSize()
3. IncreaseSize(delta) should be added, and it should call LockedMaybeUpdateSize()
4. GetQuotaObject() should return file size as an ouput argument.

Comment 222

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P4 (v2): Update padding size to the QuotaManager. r=bkelly, r?jan

Addressed comment

Comment 223

[Hsin-Yi Tsai (she/her) [:hsinyi]]

There are about two weeks before 57 merge date. In consideration of the big change this bug brings and the soft freeze period, to play it safer, there is just about1 week remaining. Is it possible that the patches get reviewed this week? Thank you.

Comment 224

[Tom Tung [:tt, :ttung]]

Yesterday, Jan told me that he needs one more day to review my patch (which means the review might be done around our 9/5 ~ 9/6).

Comment 225

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Tom Tung [:tt] from comment #224)
> Yesterday, Jan told me that he needs one more day to review my patch (which
> means the review might be done around our 9/5 ~ 9/6).

Cool, thanks for letting me know. :)

Comment 226

[Jan Varga [:janv]]

Yes, it's coming today.

Comment 227

[Jan Varga [:janv]]

Comment on attachment 8898101 [details] [diff] [review]
Bug 1290481 - P4 (v2): Update padding size to the QuotaManager. r=bkelly, r?jan

Review of attachment 8898101 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/ActorsParent.cpp
@@ +3829,5 @@
>  QuotaManager::GetQuotaObject(PersistenceType aPersistenceType,
>                               const nsACString& aGroup,
>                               const nsACString& aOrigin,
> +                             nsIFile* aFile,
> +                             int64_t* aFileSizeOut)

This could be written as:
int64_t* aFileSizeOut /* = nullptr */)

@@ +3834,2 @@
>  {
>    NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");

What about adding this here:
if (aFileSizeOut) {
  *aFileSizeOut = 0;
}

then you don't have to do it every time we "return nullptr"

There's a little penalty, in the case of success the variable is set twice, but I think we can live with that.

@@ +3836,5 @@
>  
>    if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) {
> +    // QM doesn't track usage for the persistent type of origin, so forbid
> +    // getting file size for this type.
> +    MOZ_ASSERT(!aFileSizeOut);

Hm, in theory we shouldn't allow passing PERSISTENCE_TYPE_PERSISTENT here at all by asserting |aPersistenceType != PERSISTENCE_TYPE_PERSISTENT|, but we currently just "return nullptr". So asserting !aFileSizeOut is not consistent with current policy. Let's just keep it as it is and initialize *aFileSizeOut in the beginning of the method.

::: dom/quota/QuotaManager.h
@@ +183,5 @@
>    already_AddRefed<QuotaObject>
>    GetQuotaObject(PersistenceType aPersistenceType,
>                   const nsACString& aGroup,
>                   const nsACString& aOrigin,
>                   const nsAString& aPath);

This variation of the method should support the out parameter too, for correctness.

Comment 228

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #227)
Thanks for the reivew!

> Comment on attachment 8898101 [details] [diff] [review]
> Bug 1290481 - P4 (v2): Update padding size to the QuotaManager. r=bkelly,
> r?jan
> 
> Review of attachment 8898101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +3829,5 @@
> >  QuotaManager::GetQuotaObject(PersistenceType aPersistenceType,
> >                               const nsACString& aGroup,
> >                               const nsACString& aOrigin,
> > +                             nsIFile* aFile,
> > +                             int64_t* aFileSizeOut)
> 
> This could be written as:
> int64_t* aFileSizeOut /* = nullptr */)

Sure

> @@ +3834,2 @@
> >  {
> >    NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
> 
> What about adding this here:
> if (aFileSizeOut) {
>   *aFileSizeOut = 0;
> }
> 
> then you don't have to do it every time we "return nullptr"
> 
> There's a little penalty, in the case of success the variable is set twice,
> but I think we can live with that.

Sure

> @@ +3836,5 @@
> >  
> >    if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) {
> > +    // QM doesn't track usage for the persistent type of origin, so forbid
> > +    // getting file size for this type.
> > +    MOZ_ASSERT(!aFileSizeOut);
> 
> Hm, in theory we shouldn't allow passing PERSISTENCE_TYPE_PERSISTENT here at
> all by asserting |aPersistenceType != PERSISTENCE_TYPE_PERSISTENT|, but we
> currently just "return nullptr". So asserting !aFileSizeOut is not
> consistent with current policy. Let's just keep it as it is and initialize
> *aFileSizeOut in the beginning of the method.

Sure.
 
> ::: dom/quota/QuotaManager.h
> @@ +183,5 @@
> >    already_AddRefed<QuotaObject>
> >    GetQuotaObject(PersistenceType aPersistenceType,
> >                   const nsACString& aGroup,
> >                   const nsACString& aOrigin,
> >                   const nsAString& aPath);
> 
> This variation of the method should support the out parameter too, for
> correctness.

Sure. 

To do this, I need to add |if (aFileSizeOut) { *aFileSizeOut = 0; }| on the top of this function as well. I'll also add the assertion for checking if it's not in main thread.

Comment 229

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P4 : Update padding size to the QuotaManager. r=bkelly, janv

Address comment

Comment 230

[Jan Varga [:janv]]

Comment on attachment 8892383 [details] [diff] [review]
P14: Add a test to verify QuotaManager upgrade from version 2 to version 3.

Review of attachment 8892383 [details] [diff] [review]:
-----------------------------------------------------------------

create_cache.js:

>function* testSteps()
>{
>  const cachesName = "create_cache";

Super nit: Add a blank line here

>  info("Clearing");
>
>  clear(continueToNextStepSync);
>  yield undefined;
>
>  caches.open(cachesName).then(continueToNextStepSync);
>  yield undefined;
>
>  finishTest();
>}

Nit: storage/temporary doesn't have to be included in the package

Also, it would be nice to add another origin like |http+++www.mozilla.org| besides |chrome| just to be sure standard origins get upgraded w/o any problems.

::: dom/quota/test/unit/test_upgrade_v2_to_v3.js
@@ +6,5 @@
> +var testGenerator = testSteps();
> +
> +function* testSteps()
> +{
> +  function getPaddingSizeFromPaddingFile(readBuffer)

This function is called only once.

@@ +10,5 @@
> +  function getPaddingSizeFromPaddingFile(readBuffer)
> +  {
> +    let view =
> +      readBuffer instanceof Float64Array ? readBuffer
> +                                         : new Float64Array(readBuffer);

Hm, file reader always return an ArrayBuffer (not Float64Array).

@@ +33,5 @@
> +  installPackage("cache_profile");
> +
> +  info("Checking padding file before upgrade (QM version 2.0)");
> +
> +  let file = getRelativeFile(origin.paddingFile);

this can be called paddingFile

@@ +40,5 @@
> +  ok(!exists, "Padding file doesn't exist");
> +
> +  info("Initializing");
> +
> +  // Initialize the second origin to trigger upgrade the QM to version 3.0

This comment should be update if you do what I suggested below.

@@ +42,5 @@
> +  info("Initializing");
> +
> +  // Initialize the second origin to trigger upgrade the QM to version 3.0
> +  let request =
> +    initOrigin(getPrincipal(otherOrigin), "default", continueToNextStepSync);

You don't have to init any specific origin to trigger upgrade.
There's the init() method that doesn't require to pass any argument, otherOrigin can be removed all together.

@@ +47,5 @@
> +  yield undefined;
> +
> +  ok(request.resultCode == NS_OK, "Initialization succeeded");
> +
> +  info("Checking storage file after upgrade (QM version 3.0)");

"storage" file, you meant padding file ?

@@ +49,5 @@
> +  ok(request.resultCode == NS_OK, "Initialization succeeded");
> +
> +  info("Checking storage file after upgrade (QM version 3.0)");
> +
> +  file = getRelativeFile(origin.paddingFile);

You don't have to call this again I think.

@@ +57,5 @@
> +
> +  info("Reading out contents of padding file");
> +
> +  File.createFromNsIFile(file).then(grabArgAndContinueHandler);
> +  file = yield undefined;

let domFile ... ?

@@ +65,5 @@
> +  fileReader.readAsArrayBuffer(file);
> +  yield undefined;
> +
> +  let paddingSize = getPaddingSizeFromPaddingFile(fileReader.result);
> +  ok(paddingSize == 0, "Padding size does initial to be zero.");

"Padding size does default to zero" ?

::: dom/quota/test/unit/xpcshell.ini
@@ +5,5 @@
>  [DEFAULT]
>  head = head.js
>  support-files =
>    basics_profile.zip
> +  cache_profile.zip

version3_0upgrade_profile.zip

@@ +31,5 @@
>  [test_removeAppsUpgrade.js]
>  [test_storagePersistentUpgrade.js]
>  [test_tempMetadataCleanup.js]
>  [test_unknownFiles.js]
> +[test_upgrade_v2_to_v3.js]

test_version3_0upgrade.js

Comment 231

[Jan Varga [:janv]]

Comment on attachment 8894779 [details] [diff] [review]
Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, r?janv

Review of attachment 8894779 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly ok, except that metadata files should be handled too.

I give this r+, but it's only valid if you fix bug 1395102 ASAP, ideally for 57.

::: dom/quota/ActorsParent.cpp
@@ +9493,5 @@
> +    // Only update DOM Cache directory for adding padding file.
> +    rv = MaybeUpgradeClients(originProps);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }

This is a major storage version upgrade. Although the reason for this is to create empty padding files for DOM cache, I would like to check metadata files here too, just like we do in the previous storage upgrade. It means that you should duplicate GetDirectoryMetadata, GetDirectoryMetadata2 calls from UpgradeStorageFrom1_0To2_0Helper::DoUpgrade and add elements to mOriginProps.
These should be then processed in UpgradeStorageFrom2_0To3_0Helper::ProcessOriginDirectory

There's already a lot of duplicated code, so adding more is not a problem, especially given that r+ for this patch is conditional (bug 1395102 should be fixed ASAP).

Comment 232

[Jan Varga [:janv]]

Comment on attachment 8904476 [details] [diff] [review]
Bug 1290481 - P4 : Update padding size to the QuotaManager. r=bkelly, janv

Review of attachment 8904476 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 233

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #231)
Thanks for the review!

> Comment on attachment 8894779 [details] [diff] [review]
> Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding
> file to existing DOM Cache directory. r=bkelly, r?janv
> 
> Review of attachment 8894779 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks mostly ok, except that metadata files should be handled too.
> 
> I give this r+, but it's only valid if you fix bug 1395102 ASAP, ideally for
> 57.

Sure, I'll try to resolve it ASAP.

> ::: dom/quota/ActorsParent.cpp
> @@ +9493,5 @@
> > +    // Only update DOM Cache directory for adding padding file.
> > +    rv = MaybeUpgradeClients(originProps);
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      return rv;
> > +    }
> 
> This is a major storage version upgrade. Although the reason for this is to
> create empty padding files for DOM cache, I would like to check metadata
> files here too, just like we do in the previous storage upgrade. It means
> that you should duplicate GetDirectoryMetadata, GetDirectoryMetadata2 calls
> from UpgradeStorageFrom1_0To2_0Helper::DoUpgrade and add elements to
> mOriginProps.
> These should be then processed in
> UpgradeStorageFrom2_0To3_0Helper::ProcessOriginDirectory

Sure

Comment 234

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, janv

Comment 235

[Tom Tung [:tt, :ttung]]

Attachment: interdiff-p6.patch

Hi Jan,

Although you've r+'ed the P6, I'm not pretty sure if I get everything right. Could you help me to check it again? This patch is the inter-diff patch for P6 between the current patch and the last patch. Thanks!

Comment 236

[Jan Varga [:janv]]

Comment on attachment 8904866 [details] [diff] [review]
interdiff-p6.patch

Review of attachment 8904866 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/ActorsParent.cpp
@@ +1837,3 @@
>    nsresult
> +  MaybeStripObsoleteOriginAttributes(const OriginProps& aOriginProps,
> +                                     bool* aStripped);

Don't duplicate this method.

@@ +9647,5 @@
> +{
> +  AssertIsOnIOThread();
> +
> +  bool stripped;
> +  nsresult rv = MaybeStripObsoleteOriginAttributes(aOriginProps, &stripped);

No, MaybeStripObsoleteOriginAttributes() should only be done in From1_0To2_0 upgrade.

Comment 237

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #230)
Thanks for the review!

> Comment on attachment 8892383 [details] [diff] [review]
> P14: Add a test to verify QuotaManager upgrade from version 2 to version 3.
> 
> Review of attachment 8892383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> create_cache.js:
> 
> >function* testSteps()
> >{
> >  const cachesName = "create_cache";
> 
> Super nit: Add a blank line here

Sure

> >  info("Clearing");
> >
> >  clear(continueToNextStepSync);
> >  yield undefined;
> >
> >  caches.open(cachesName).then(continueToNextStepSync);
> >  yield undefined;
> >
> >  finishTest();
> >}
> 
> Nit: storage/temporary doesn't have to be included in the package
> 
> Also, it would be nice to add another origin like |http+++www.mozilla.org|
> besides |chrome| just to be sure standard origins get upgraded w/o any
> problems.

Sure.

> ::: dom/quota/test/unit/test_upgrade_v2_to_v3.js
> @@ +6,5 @@
> > +var testGenerator = testSteps();
> > +
> > +function* testSteps()
> > +{
> > +  function getPaddingSizeFromPaddingFile(readBuffer)
> 
> This function is called only once.

I'll remove it. I thought extracting it out can increase a bit readability.

> @@ +10,5 @@
> > +  function getPaddingSizeFromPaddingFile(readBuffer)
> > +  {
> > +    let view =
> > +      readBuffer instanceof Float64Array ? readBuffer
> > +                                         : new Float64Array(readBuffer);
> 
> Hm, file reader always return an ArrayBuffer (not Float64Array).

Yes, you're right. I was thinking to make it cover more cases so that it could be easy to be reused. I'll new the array directly.

> @@ +33,5 @@
> > +  installPackage("cache_profile");
> > +
> > +  info("Checking padding file before upgrade (QM version 2.0)");
> > +
> > +  let file = getRelativeFile(origin.paddingFile);
> 
> this can be called paddingFile

Sure

> @@ +40,5 @@
> > +  ok(!exists, "Padding file doesn't exist");
> > +
> > +  info("Initializing");
> > +
> > +  // Initialize the second origin to trigger upgrade the QM to version 3.0
> 
> This comment should be update if you do what I suggested below.
> 
> @@ +42,5 @@
> > +  info("Initializing");
> > +
> > +  // Initialize the second origin to trigger upgrade the QM to version 3.0
> > +  let request =
> > +    initOrigin(getPrincipal(otherOrigin), "default", continueToNextStepSync);
> 
> You don't have to init any specific origin to trigger upgrade.
> There's the init() method that doesn't require to pass any argument,
> otherOrigin can be removed all together.

Sure

> @@ +47,5 @@
> > +  yield undefined;
> > +
> > +  ok(request.resultCode == NS_OK, "Initialization succeeded");
> > +
> > +  info("Checking storage file after upgrade (QM version 3.0)");
> 
> "storage" file, you meant padding file ?

Yes, sorry for the typo.

> @@ +49,5 @@
> > +  ok(request.resultCode == NS_OK, "Initialization succeeded");
> > +
> > +  info("Checking storage file after upgrade (QM version 3.0)");
> > +
> > +  file = getRelativeFile(origin.paddingFile);
> 
> You don't have to call this again I think.

Yes, but I change the test to verify two origins (chrome, http+++www.mozilla.org). 
So that I need to get it again.

> @@ +57,5 @@
> > +
> > +  info("Reading out contents of padding file");
> > +
> > +  File.createFromNsIFile(file).then(grabArgAndContinueHandler);
> > +  file = yield undefined;
> 
> let domFile ... ?

Sure

> @@ +65,5 @@
> > +  fileReader.readAsArrayBuffer(file);
> > +  yield undefined;
> > +
> > +  let paddingSize = getPaddingSizeFromPaddingFile(fileReader.result);
> > +  ok(paddingSize == 0, "Padding size does initial to be zero.");
> 
> "Padding size does default to zero" ?

Sure

> ::: dom/quota/test/unit/xpcshell.ini
> @@ +5,5 @@
> >  [DEFAULT]
> >  head = head.js
> >  support-files =
> >    basics_profile.zip
> > +  cache_profile.zip
> 
> version3_0upgrade_profile.zip

Sure

> @@ +31,5 @@
> >  [test_removeAppsUpgrade.js]
> >  [test_storagePersistentUpgrade.js]
> >  [test_tempMetadataCleanup.js]
> >  [test_unknownFiles.js]
> > +[test_upgrade_v2_to_v3.js]
> 
> test_version3_0upgrade.js

Sure

Comment 238

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P14: Add a test to verify QuotaManager upgrade from version 2 to version 3. r=bkelly, janv

Comment 239

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #236)
> Comment on attachment 8904866 [details] [diff] [review]
> interdiff-p6.patch
> 
> Review of attachment 8904866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +1837,3 @@
> >    nsresult
> > +  MaybeStripObsoleteOriginAttributes(const OriginProps& aOriginProps,
> > +                                     bool* aStripped);
> 
> Don't duplicate this method.
> 
> @@ +9647,5 @@
> > +{
> > +  AssertIsOnIOThread();
> > +
> > +  bool stripped;
> > +  nsresult rv = MaybeStripObsoleteOriginAttributes(aOriginProps, &stripped);
> 
> No, MaybeStripObsoleteOriginAttributes() should only be done in From1_0To2_0
> upgrade.

Oh, I see. Sorry for the misunderstanding...

Comment 240

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, janv

Update the patch

Comment 241

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #236)
Hi Jan,

The content of my inter-diff patch for P6 is somehow wrong, Would you mind helping me to skim my P6 patch one more times? Thanks! I only remove the function 2_0To3_0::MaybeStripObsoleteOriginAttributes() and update the comment in this update.

Comment 242

[Jan Varga [:janv]]

Comment on attachment 8904951 [details] [diff] [review]
Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, janv

Review of attachment 8904951 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/ActorsParent.cpp
@@ +1832,5 @@
> +  MaybeUpgradeClients(const OriginProps& aOriginProps);
> +
> +  // XXXtt: The following function is duplicated from
> +  // UpgradeStorageFrom1_0To2_0Helper and it should be extracted out in
> +  // bug 1395102.

Nit: I think this comment applies to the whole class.

@@ +9567,5 @@
> +                                                const OriginProps& aOriginProps)
> +{
> +  AssertIsOnIOThread();
> +
> +  nsresult rv = NS_OK;

Nit: don't need to initialize to NS_OK;

Nit: add a blank line here

Comment 243

[Jan Varga [:janv]]

Comment on attachment 8904943 [details] [diff] [review]
Bug 1290481 - P14: Add a test to verify QuotaManager upgrade from version 2 to version 3. r=bkelly, janv

Review of attachment 8904943 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/test/unit/test_version3_0upgrade.js
@@ +8,5 @@
> +function* testSteps()
> +{
> +  const origins = [
> +    {
> +      paddingFile: "storage/default/chrome/cache/.padding",

Both entries reference .padding file and the name of this array is "origins", so it would be slightly better if you removed the "paddingFile" property.

For example:
const origins = [
  "storage/default/chrome",
  "storage/default/http+++www.mozilla.org"
];

@@ +27,5 @@
> +  // The file create_cache.js in the package was run locally, specifically it
> +  // was temporarily added to xpcshell.ini and then executed:
> +  //   mach xpcshell-test --interactive dom/quota/test/unit/create_cache.js
> +  // Note: it only creates the directory "storage/default/chrome/cache".
> +  // To make it become the profile in the test, two more manual steps is needed.

*are* needed

@@ +36,5 @@
> +
> +  info("Checking padding file before upgrade (QM version 2.0)");
> +
> +  for (let origin of origins) {
> +    let paddingFile = getRelativeFile(origin.paddingFile);

this would become:
let paddingFile = getRelativeFile(origin + "cache/.paddingFile");

@@ +68,5 @@
> +    fileReader.onload = continueToNextStepSync;
> +    fileReader.readAsArrayBuffer(domFile);
> +    yield undefined;
> +
> +    let paddingFileInfo = new Float64Array(fileReader.result);

Maybe check the length of the Float64Array too.

Comment 244

[Jan Varga [:janv]]

This probably needs one more try push before landing.

Comment 245

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #244)
> This probably needs one more try push before landing.

Thanks! I'll address the comments and push them to try before landing!

Comment 246

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, janv

Address comment

Comment 247

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P14: Add a test to verify QuotaManager upgrade from version 2 to version 3. r=bkelly, janv

Comment 248

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P1: Make InternalResponse and CacheResponse be able to keep padding size. r=bkelly

Update P1 since the base has been changed.

Comment 249

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aaf253ee150b6365769cf6d045481939ddbbfe9

Comment 250

[Jan Varga [:janv]]

Comment on attachment 8904993 [details] [diff] [review]
Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, janv

Review of attachment 8904993 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/ActorsParent.cpp
@@ +1793,1 @@
>  class UpgradeStorageFrom1_0To2_0Helper final

:)

duplicated from itself ?

should go before:
"class UpgradeStorageFrom2_0To3_0Helper final"

Comment 251

[Jan Varga [:janv]]

Comment on attachment 8904994 [details] [diff] [review]
Bug 1290481 - P14: Add a test to verify QuotaManager upgrade from version 2 to version 3. r=bkelly, janv

Review of attachment 8904994 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/test/unit/test_version3_0upgrade.js
@@ +10,5 @@
> +  const origins = [
> +    "storage/default/chrome/",
> +    "storage/default/http+++www.mozilla.org/"
> +  ];
> +  const paddingFileDir = "cache/.padding";

Nit: paddingFilePath

@@ +68,5 @@
> +    yield undefined;
> +
> +    let paddingFileInfo = new Float64Array(fileReader.result);
> +    ok(paddingFileInfo[0] == 0, "Padding size does default to zero.");
> +    ok(paddingFileInfo.length == 1, "Padding file does take 64 bytes.");

It makes more sense to check length first and then access the first element in the array.

Comment 252

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #250)
> :)
> 
> duplicated from itself ?
> 
> should go before:
> "class UpgradeStorageFrom2_0To3_0Helper final"

Oops, sorry for that...

(In reply to Jan Varga [:janv] from comment #251)
> Comment on attachment 8904994 [details] [diff] [review]
> Bug 1290481 - P14: Add a test to verify QuotaManager upgrade from version 2
> to version 3. r=bkelly, janv
> 
> Review of attachment 8904994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/test/unit/test_version3_0upgrade.js
> @@ +10,5 @@
> > +  const origins = [
> > +    "storage/default/chrome/",
> > +    "storage/default/http+++www.mozilla.org/"
> > +  ];
> > +  const paddingFileDir = "cache/.padding";
> 
> Nit: paddingFilePath

Sure

> @@ +68,5 @@
> > +    yield undefined;
> > +
> > +    let paddingFileInfo = new Float64Array(fileReader.result);
> > +    ok(paddingFileInfo[0] == 0, "Padding size does default to zero.");
> > +    ok(paddingFileInfo.length == 1, "Padding file does take 64 bytes.");
> 
> It makes more sense to check length first and then access the first element
> in the array.

Sure

Comment 253

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, janv

Update patch per comment 250

Comment 254

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1290481 - P14: Add a test to verify QuotaManager upgrade from version 2 to version 3. r=bkelly, janv

Update patch per comment 251

Comment 255

[Tom Tung [:tt, :ttung]]

Try looks good, so I'm preparing to land them to inbound.

Hi Pascal, Romain,

Since landing these patches triggers the QuotaMnager's major upgrade and (DOM) Cache schema upgrade, it implies users cannot fallback/switch to the previous Firefox version without breaking their profile (or websites using IndexedDB, (DOM)Cache (Service Worker), ASMJS Cache will be broken). 

To avoid this problem, users who often switch between different Firefox versions need to separate their profile. I quote the solution provided in bug 1357428 comment #20 (thanks to :asuth \o/) below:

Use a separate profile when testing with multiple versions of Firefox as described at https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles.  If you accidentally use a profile in Nightly that you would like to make work in an earlier version of Firefox again, you can follow the steps at https://support.mozilla.org/en-US/kb/refresh-firefox-reset-add-ons-and-settings. 

Could you give me suggestions about whether we need to inform users before landing these patches and how do we inform the Nightly users? Thanks!

Comment 256

[Romain Testard [:RT]]

Hi Tom, Joni helped put together a SUMO page for using dedicated Nightly profiles for this issue in particular: https://support.mozilla.org/en-US/kb/using-dedicated-profile-firefox-nightly
Asa was going to communicate this through the Nightly twitter account. Asa can you please confirm if this can now be shared?

Comment 257

[Pascal Chevrel:pascalc]

(In reply to Romain Testard [:RT] from comment #256)
> Hi Tom, Joni helped put together a SUMO page for using dedicated Nightly
> profiles for this issue in particular:
> https://support.mozilla.org/en-US/kb/using-dedicated-profile-firefox-nightly
> Asa was going to communicate this through the Nightly twitter account. Asa
> can you please confirm if this can now be shared?

Asa is not behind the Nightly twitter account, that's me. And this article is already documented in our Nightly Wiki page https://wiki.mozilla.org/Nightly#Platform_specific_instructions

Comment 258

[Pascal Chevrel:pascalc]

(In reply to Tom Tung [:tt] (OoO: 9/22 ~ 9/29) from comment #255)
> Try looks good, so I'm preparing to land them to inbound.
> 
> Hi Pascal, Romain,
> 
> Could you give me suggestions about whether we need to inform users before
> landing these patches and how do we inform the Nightly users? Thanks!

This is not the first time it happens, during the 55 cycle it happened as well and we put a release note warning Nightly users that we don't support downgrades (https://www.mozilla.org/en-US/firefox/55.0a1/releasenotes/#note-787051).

I have just updated our Nightly documentation with a stronger wording in bold (https://wiki.mozilla.org/Nightly#Platform_specific_instructions) and I will tweet about it this afternoon to remind our user base that they should always use a separate profile for Nightly, thanks for the heads up!

Comment 259

[Tom Tung [:tt, :ttung]]

(In reply to Pascal Chevrel:pascalc from comment #258)
> This is not the first time it happens, during the 55 cycle it happened as
> well and we put a release note warning Nightly users that we don't support
> downgrades
> (https://www.mozilla.org/en-US/firefox/55.0a1/releasenotes/#note-787051).
> 
> I have just updated our Nightly documentation with a stronger wording in
> bold (https://wiki.mozilla.org/Nightly#Platform_specific_instructions) and I
> will tweet about it this afternoon to remind our user base that they should
> always use a separate profile for Nightly, thanks for the heads up!

Thanks! So, let's land it!

Comment 260

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e149be931c1
P1: Make InternalResponse and CacheResponse be able to keep padding size. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/d21a58f0e341
P2: Upgrade the version of cache.sqlite to store opaque responses' padding size in SQLite. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b635d9ede1b
P3: Record padding size into cache.sqlite. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c60914e10e9
P4: Update padding size to the QuotaManager. r=bkelly, janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/bafa89b7f292
P5: Implement a function to generate padding size. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3844ec447b4
P6: Upgrade QuotaManager to v3.0 for adding directory padding file to existing DOM Cache directory. r=bkelly, janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8969011f02
P7: Create a mutex lock on CacheQuotaClient for protected accessing directory padding file. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5a573de663
P8: Implement few utility functions to access direcotry padding file. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/885e9a3b821e
P9: Implement a function to get overall padding size for an origin from cache.sqlite. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c4ad5e1f2d
P10: Update padding size to the direcotry padding file. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/851077b7eb92
P11: Add an assertion to ensure DOM Cache running in the Quota IO thread while getting overall usage. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6431977fa7a
P12: Get the padding size while QM gets usage from DOM Cache. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/b170ee312182
P13: Get padding from DB when there is no directroy padding file. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/35cc0e2f8b6c
P14: Add a test to verify QuotaManager upgrade from version 2 to version 3. r=bkelly, janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aa1cc819058
P15: Add an test to verify opaque response has padding and the usage are still the same between in-memory object in QM and the file-system. r=bkelly

Comment 261

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9e149be931c1
https://hg.mozilla.org/mozilla-central/rev/d21a58f0e341
https://hg.mozilla.org/mozilla-central/rev/3b635d9ede1b
https://hg.mozilla.org/mozilla-central/rev/8c60914e10e9
https://hg.mozilla.org/mozilla-central/rev/bafa89b7f292
https://hg.mozilla.org/mozilla-central/rev/a3844ec447b4
https://hg.mozilla.org/mozilla-central/rev/9a8969011f02
https://hg.mozilla.org/mozilla-central/rev/de5a573de663
https://hg.mozilla.org/mozilla-central/rev/885e9a3b821e
https://hg.mozilla.org/mozilla-central/rev/69c4ad5e1f2d
https://hg.mozilla.org/mozilla-central/rev/851077b7eb92
https://hg.mozilla.org/mozilla-central/rev/f6431977fa7a
https://hg.mozilla.org/mozilla-central/rev/b170ee312182
https://hg.mozilla.org/mozilla-central/rev/35cc0e2f8b6c
https://hg.mozilla.org/mozilla-central/rev/6aa1cc819058

Comment 262

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Congrats! Thanks all for the good work and caring community communication. :)

Comment 263

[Tom Tung [:tt, :ttung]]

Thanks for all the effort from bkelly and janv! Also, thanks to Tom Van Goethem! They clarify my confusions and teach me design patterns and help me to design the padding structure. I do learn a lot in this bug. :)

Comment 264

[Tom Tung [:tt, :ttung]]

The padding size for each response is recorded in the db of cache. To make the QM don't need to initialize each db of cache while QM initialization, we store the overall padding size of an origin as a copy in the cache directory (the directory padding file).

To avoid from accessing padding size in multiple threads (QM access size in Quota IO thread when getting usage and Cache changes padding size in cache IO threads), we use a mutex held by QuotaClient to protect it. If something really bad happens which lead them unsync. We trust the size recorded in the db since the file is just a copy.

To avoid from getting the different size from padding file and db,  we divide updating padding size into two steps: First, write the value into a temporary padding file (.padding-tmp). Then, after updating things into the db, renaming the temporary padding file to padding file (.padding). (Note: we update the delta of padding size the QM before updating the size to file and db. If the failure happens, we update the correct value to the QM)

To make this mechanism realizing the failure happens, we need to ensure either the temporary padding file should exist, or the padding file shouldn't exist until the next cache action. So that the upcoming cache action knows it should trust the padding size from the db rather than the file. If the temporary file doesn't exist and the padding file does exist, we trust the size from the file.