1460768 - Site Data Manager should watch for site data changes and update itself

Status: NEW

(Firefox :: Settings UI, defect, P3)

Description

[johnjones1979]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180503143129

Steps to reproduce:

1. Visit a site (in my case logging in to mail.yahoo.com)
2. In options->Cookies and Site Data select Manage Data
3. Type in "yah" as the search filter.
4. Note that most of the cookies do not show up. They all showed up prior to this update.
5. Select "remove all shown" and save
6. Note that the cookies that were not shown were not cleared and mail has not been logged out (in previous versions cookies were cleared and logout was immediate).
7. In options->Cookies and Site Data select Clear Data and clear it
8. Now logout occurs in mail
9. In options->Cookies and Site Data select Manage Data, the type in "yah"
10. Go back to mail.yahoo.com. Note that in the manage data tab no cookies have appeared. Continuing to log in to mail etc also does not add cookies to this list. Basically it no longer updates after it has been opened and must be reopened to update the cookie list (previous version updated in real-time)
11. The Clear Data option also shows 0 bytes for cookies even after logging into mail.


Actual results:

Cookie are no longer cleared or tracked correctly and can only be cleared by clearing all data.


Expected results:

All cookies should show in the Manage Data list of cookies. Clearing from this list should also result in the cookies being cleared. At the moment since not all cookies are shown, they are not all cleared.

Comment 1

[Johann Hofmann [:johannh]]

I can not reproduce this by doing the following:

- Go to mail.yahoo.com
- Sign in
- Go to about:preferences, Cookies and Site Data
- Search for "yah"
- Remove all selected and Save Changes

Could it be that you already had about:preferences open before signing into Yahoo? I assume the "multiple" issues boil down to the limitation that this dialog does not update in real time, which has been a commonly voiced complaint (though I don't think we actually have a bug to track it so far) and I agree that we should fix that. Reloading about:preferences should do the trick for you.

I'll morph this bug to cover updating the site data whenever something changes, maybe using the new storage observer...

Thanks!

Comment 3

[johnjones1979]

Thank you. I can confirm that I already had about:preferences open before signing in, so my steps to reproduce were wrong (sorry). Opening it afterwards resolves my issues, but the real-time update was definitely preferable.

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 1 - Have StorageActivityService emit an observer notification when storage is modified.

Review commit: https://reviewboard.mozilla.org/r/255130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255130/

Comment 6

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 2 - Make SiteDataManager automatically update itself when on storage changes.

Review commit: https://reviewboard.mozilla.org/r/255132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255132/

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 3 - Update consumers of SiteDataManager.jsm.

Review commit: https://reviewboard.mozilla.org/r/255134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255134/

Comment 8

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 4 - Update tests

Review commit: https://reviewboard.mozilla.org/r/255136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255136/

Comment 9

[Johann Hofmann [:johannh]]

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

Comment 10

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07d9e40c1501290e7a76c2db90dd97e2cfc3529e

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8990131 [details]
Bug 1460768 - Part 1 - Have StorageActivityService emit an observer notification when storage is modified.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255130/diff/1-2/

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8990132 [details]
Bug 1460768 - Part 2 - Make SiteDataManager automatically update itself when on storage changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255132/diff/1-2/

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8990133 [details]
Bug 1460768 - Part 3 - Update consumers of SiteDataManager.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255134/diff/1-2/

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8990134 [details]
Bug 1460768 - Part 4 - Update tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255136/diff/1-2/

Comment 16

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 1 - Have StorageActivityService emit an observer notification when storage is modified.

MozReview-Commit-ID: KO1Y75AfgVc

Comment 17

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 2 - Make SiteDataManager automatically update itself when on storage changes.

MozReview-Commit-ID: CI6axhFWdSL

Comment 18

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 3 - Update consumers of SiteDataManager.jsm.

MozReview-Commit-ID: 51Zbg0ZtrUv

Comment 19

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 4 - Update tests

MozReview-Commit-ID: 1fPCAW13plZ

Comment 20

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

Johann - did this land at some point or is it still waiting on other work? Thanks.

Comment 21

[Johann Hofmann [:johannh]]

Hey Liz, thanks for checking in. This work is pretty complete but hasn't landed yet. I need to investigate some final test failures (and get review, of course), but the anti-tracking work has consumed most of my time recently.

I should really finish this. Maybe we can get it done in time for an early beta uplift.

Comment 22

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 1 - Have StorageActivityService emit an observer notification when storage is modified. r=baku

Comment 23

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 2 - Make SiteDataManager automatically update itself when on storage changes. r=Gijs

Comment 24

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 3 - Update consumers of SiteDataManager.jsm. r=Gijs

Comment 25

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 4 - Update tests for SiteDataManager changes. r=Gijs

Comment 26

[Johann Hofmann [:johannh]]

Attachment: Bug 1460768 - Part 5 - New tests for SiteDataManager. r=Gijs

Comment 27

[Johann Hofmann [:johannh]]

Please take your time with the reviews, I'm not hoping to land before 66 merges to beta. This feels like it would need a good beta cycle to confirm it doesn't impact perf and to iron out possible issues.

Comment 28

[:Gijs (out for now; he/him)]

(In reply to Johann Hofmann [:johannh] from comment #27)

Please take your time with the reviews, I'm not hoping to land before 66 merges to beta. This feels like it would need a good beta cycle to confirm it doesn't impact perf and to iron out possible issues.

Can you do a trypush with talos and raptor tests (at least 5 runs of each) to check for perf issues pre-emptively, if we expect there might be some?

Did you consider an alternative solution, like refreshing the list when the tab gets a pageshow event (ie. because the user switched to a different tab and then back)? That seems like it'd require less infrastructure and have less perf impact.

If we need to use this mechanism, can we add/remove the cache/cookie/whatever listeners based on when the dialog is actually open, instead of doing it on startup? As it is, the perf impact even just on startup seems like it'd likely not be acceptable (even if it probably won't end up showing up on try because it's so small on the really fast hardware we run the tests on, and the noise is substantial).

Comment 29

[Johann Hofmann [:johannh]]

Yeah, there might be better ways to do this. One problem here is that I'm trying to solve both this bug and bug 1468355 in a single set of patches as well as doing a bunch of unrelated cleanups.

The more I think about this the more I come to the conclusion that having a constantly updating SiteDataManager might not be worth it, even if the perf hit is minimal. I like the idea of attaching listeners while the dialog (or about:preferences) is open much better, I'll only have to find a separate solution to bug 1468355 then.

I'm going to come up with a plan on how to fix all these intermingled issues in separate patches instead and file the outstanding bugs.

Comment 30

[Johann Hofmann [:johannh]]

With bug 1468355 on its way to resolution I have to admit that I have bigger fish to fry right now and am resigning from this bug for now...