Open Bug 1460768 Opened 6 years ago Updated 2 years ago

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

Categories

(Firefox :: Settings UI, defect, P3)

Product:
Firefox
Component:
Settings UI
Version:
60 Branch
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fix-optional

People

(Reporter: johnjones1979, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(13 obsolete files) 

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.
Has STR: --- → yes
Component: Untriaged → Preferences
Flags: needinfo?(jhofmann)
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!
Blocks: storage-v2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jhofmann)
Priority: -- → P3
Summary: Cookies and Site Data multiple (related) issues introduced in version 60.0 → Site Data Manager does not update until reload
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.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Summary: Site Data Manager does not update until reload → Site Data Manager should watch for site data changes and update itself
MozReview-Commit-ID: 51Zbg0ZtrUv
Attached file Bug 1460768 - Part 4 - Update tests (obsolete) — 
MozReview-Commit-ID: 1fPCAW13plZ
Johann - did this land at some point or is it still waiting on other work? Thanks.
status-firefox62: --- → wontfix
status-firefox63: --- → wontfix
status-firefox64: --- → ?
Flags: needinfo?(jhofmann)
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.
status-firefox64: ? → fix-optional
Flags: needinfo?(jhofmann)
Attachment #8990131 - Attachment is obsolete: true
Attachment #8990132 - Attachment is obsolete: true
Attachment #8990133 - Attachment is obsolete: true
Attachment #8990134 - Attachment is obsolete: true
Attachment #9008660 - Attachment is obsolete: true
Attachment #9008661 - Attachment is obsolete: true
Attachment #9008662 - Attachment is obsolete: true
Attachment #9008663 - Attachment is obsolete: true
Attachment #9019266 - Attachment description: Bug 1460768 - Part 1 - Have StorageActivityService emit an observer notification when storage is modified. → Bug 1460768 - Part 1 - Have StorageActivityService emit an observer notification when storage is modified. r=baku
Attachment #9019267 - Attachment description: Bug 1460768 - Part 2 - Make SiteDataManager automatically update itself when on storage changes. → Bug 1460768 - Part 2 - Make SiteDataManager automatically update itself when on storage changes. r=Gijs
Attachment #9019268 - Attachment description: Bug 1460768 - Part 3 - Update consumers of SiteDataManager.jsm. → Bug 1460768 - Part 3 - Update consumers of SiteDataManager.jsm. r=Gijs
Attachment #9019269 - Attachment description: Bug 1460768 - Part 4 - Update tests → Bug 1460768 - Part 4 - Update tests for SiteDataManager changes. r=Gijs
Attachment #9022557 - Attachment description: Bug 1460768 - Part 5 - New tests for SiteDataManager. → Bug 1460768 - Part 5 - New tests for SiteDataManager. r=Gijs

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.

(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).

Flags: needinfo?(jhofmann)

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.

Flags: needinfo?(jhofmann)
Depends on: 1523272
No longer blocks: 1468355

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...

Assignee: jhofmann → nobody
Status: ASSIGNED → NEW
Attachment #9019266 - Attachment is obsolete: true
Attachment #9019267 - Attachment is obsolete: true
Attachment #9019268 - Attachment is obsolete: true
Attachment #9019269 - Attachment is obsolete: true
Attachment #9022557 - Attachment is obsolete: true
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: