1459974 - Sync StorageActivityService data on disk

Status: NEW

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

Description

[Andrea Marchesini [:baku]]

Currently StorageActivityService keeps the list of principals and the timestamps of pages doing writing operations via any Storage API. But this list is just in memory. This can be an issue if firefox is restarted and then, the user wants to clean the history of the last X hours/minutes.

This bug is about writing the list of activities on disk in order to have it back after a restart.

Comment 1

[Andrea Marchesini [:baku]]

Johann, I think this bug is quite important. We should prioritize it. If you agree, I'm happy to work on it.

Comment 2

[Johann Hofmann [:johannh]]

Oh, right, that's a good point. Leaving this bug unattended for too long could be harmful, I definitely agree. (Though it's a bit unsatisfactory that we have to introduce another on-disk datastore for cleaning up data).

Thanks for catching that!

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - Sync on disk

Do you mind to review this set of patches?
The first patch is about creating a syncOnDisk method and use it when needed.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - Read from disk

Here the reading when the profile directory becomes available

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - getActiveOrigins must be async

GetActiveOrigin must be async.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - gtest

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 4 - gtest

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8974400 [details] [diff] [review]
part 1 - Sync on disk

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

Sorry for the big delay, was digging my way out of post-PTO backlog.

This looks pretty reasonable to me, but I don't feel comfortable reviewing big chunks of storage code. I'll forward to asuth.

::: dom/storage/StorageActivityService.cpp
@@ +361,5 @@
> +
> +void
> +StorageActivityService::ScheduleSyncOnDisk(const MutexAutoLock& aProofOfLock)
> +{
> +  // Already scheduleted.

typo: scheduled

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8974402 [details] [diff] [review]
part 3 - getActiveOrigins must be async

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

r=me on the browser parts

::: browser/base/content/test/sanitize/browser_sanitize-offlineData.js
@@ +85,5 @@
>    let endDate = Date.now() * 1000;
> +
> +  let principals = await new Promise(resolve => {
> +    sas.getActiveOrigins(endDate - oneHour, endDate, (status, data) => {
> +      if (status != Cr.NS_OK) {

For tests, shouldn't we rather fail here?

::: browser/modules/Sanitizer.jsm
@@ +394,5 @@
> +                resolve([]);
> +                return;
> +              }
> +
> +              let principals = data.QueryInterface(Ci.nsIArray);

Doesn't this fail ESLint because of shadowing? I kinda dislike the no-shadow rule :(

::: dom/interfaces/storage/nsIStorageActivityService.idl
@@ +23,5 @@
>  interface nsIStorageActivityService : nsISupports
>  {
> +  // This calls the callback |cb| when the array of nsIPrincipals, active
> +  // between |from| and |to| timestamps, is ready. Note activities older than 1
> +  // day are forgotten.  Activity details are not persisted, so this only

They are persisted now, right?

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8974419 [details] [diff] [review]
part 4 - gtest

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

This looks all pretty good to me in general, but that's probably an indicator that I don't have enough experience with this code yet. :)

Thanks for the patches!

Comment 11

[Marion Daly [:mdaly]]

Where are we with this bug?

Comment 12

[Andrea Marchesini [:baku]]

Would be great if we can finish the review process and land this code. Who can review this code? It seems not so high priority...

Comment 13

[Johann Hofmann [:johannh]]

We're using storage activity service for cookie purging now, so we want to finish this.

Comment 14

[Andrea Marchesini [:baku]]

Are you going to review the remaining bit? Let me know if I can help here.

Comment 15

[Tim Huang[:timhuang]]

Attachment: Bug 1459974 - Part 1: Sync the storage activites to the file 'activities.txt'.

This patch implements the sync mechanism which syncs the storage
activities to the file 'activities.txt' in the profile folder. Any
update to the stoarge acgtivites will be synced to the file. And both
the file and the activites are protected by a mutex.

Comment 16

[Tim Huang[:timhuang]]

Attachment: Bug 1459974 - Part 2: Read the stroage activites from the disk after the profile has been fully loaded.

This patch implements the reading of the activites file. The reading
will happen after the profile has been fully loaded.

Depends on D84148

Comment 17

[Tim Huang[:timhuang]]

Attachment: Bug 1459974 - Part 3: Change nsIStorageActivityService.getActiveOrigins into async manner.

We have to convert the nsIStorageActivityService.getActiveOrigins into
async since it might be called while the activity hasn't been loaded
yet.

Depends on D84149

Comment 18

[Tim Huang[:timhuang]]

Attachment: Bug 1459974 - Part 4: Add gtests.

Depends on D84150

Comment 19

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

Does this mean we can obsolete :baku's original 4 patches?

Comment 20

[Tim Huang[:timhuang]]

Comment on attachment 8974419 [details] [diff] [review]
part 4 - gtest

Yes, we can. Basically, I rebased his patches and put them on phabricator.

Comment 21

[Johann Hofmann [:johannh]]

Tim, are you still working on this? Do you want me to take it up instead if you're busy (thanks for rebasing those patches)?

Comment 22

[Tim Huang[:timhuang]]

I am not working on this. It would be good if you can take it. Thanks.

Comment 23

[Johann Hofmann [:johannh]]

Time to accept that I won't get to this in the near future :|

Comment 24

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

(Resetting the assignee that I believe was changed by the phabricator automation because :jstutte updated the review group from dom-workers-and-storage to dom-storage. I believe comment 22 accurately and then comment 23 reflect this.)