Closed Bug 1401878 Opened 2 years ago Closed 2 years ago

SiteDataManager.jsm removeAll should clean up ServiceWorkers and QuotaManager data

Categories

(Firefox :: Preferences, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 + fixed
firefox58 --- verified
firefox59 --- verified

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(2 files)

Currently ServiceWorkers are not removed, and in bug 1401850 I have introduced a better way to clean up QuotaManager data.
I don't want to ask for a review because bug 1401850 is not landed yet.
Attachment #8910644 - Flags: review?(bkelly)
Comment on attachment 8910644 [details] [diff] [review]
part 2 - ServiceWorkers

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

::: browser/components/preferences/SiteDataManager.jsm
@@ +261,5 @@
>      Services.cache2.clear();
>      Services.cookies.removeAll();
>      OfflineAppCacheHelper.clear();
>  
> +    // Iterate through the service workers and remove them.

Again, we should instead be fixing bug 1183245.  If quota manager triggers the wipe then the service worker registration is going to remain.

For the short term I guess this is ok, though.  Please add a comment both here and in bug 1183245 to remove this later.
Attachment #8910644 - Flags: review?(bkelly) → review+
Component: General → Preferences
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04888373f12b
Remove ServiceWorkers in SiteDataManager.jsm, r=bkelly
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f63440391b0
Backed out changeset 04888373f12b for xpcshell bustage a=backout
I believe I've corrected the underlying problem in bug 1047098 as part of my corrective push of https://bugzilla.mozilla.org/show_bug.cgi?id=1047098#c59 where s/do_get_profile()/do_get_profile(true)/ induced pre-existing breakage has been resolved.  I'm running some local tests now but assuming they seem good, I'll push this fix again as well.
https://hg.mozilla.org/mozilla-central/rev/e2b1cd9fd4a6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
We may want this for 57 release for protecting privacy.
Comment on attachment 8910644 [details] [diff] [review]
part 2 - ServiceWorkers

Approval Request Comment
[Feature/Bug causing the regression]: SiteDataManager.jsm
[User impact if declined]: ServiceWorkers are not unregistered when the user wants to clean up site data.
[Is this code covered by automated tests?]: none
[Has the fix been verified in Nightly?]: not yet?
[Needs manual test from QE? If yes, steps to reproduce]: yes. Cleanup a website with a SW and check if the SW has been correctly unregistered. 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: This cleanup approach exists in many other parts. A simple loop.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8910644 - Flags: approval-mozilla-beta?
Comment on attachment 8910644 [details] [diff] [review]
part 2 - ServiceWorkers

Fix a privacy issue, taking it.
Should be in 57b3
Attachment #8910644 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I tested this bugs using these steps:

1. Access twitter and log in
2. Access youtube
3. Access about:debugging#workers (And saw in the list both twitter and youtube)
4. Access about:preferences#privacy - Site Data - Settings...
5. From the pop-up window I deleted twitter and saved the changes.
6. I refreshed and checked again about:debugging#workers.

As far as I know, after these steps, twitter shouldn't be in the Service Workers list, but it's still there, even after I opened it again. 

I tested this on latest Nightly 58 using Windows 10 X64, Ubuntu 16.04 x64 and Mac OS X 10.11. 

Did I you something wrong or did I understand wrong what I should have done?
Ni for comment #15...
Flags: needinfo?(amarchesini)
Oana, can you please test it again now that bug 1410416 is landed? Thanks.
I tried locally and it works correctly for me.
Flags: needinfo?(amarchesini) → needinfo?(oana.botisan)
(In reply to Andrea Marchesini [:baku] from comment #17)
> Oana, can you please test it again now that bug 1410416 is landed? Thanks.
> I tried locally and it works correctly for me.

I can still reproduce the bug on latest Nightly. Twitter still appears in the Service Workers list. 
I retested everything on Windows 10x64, Mac OS X 10.11 and Ubuntu 12.04 x32.
Flags: needinfo?(oana.botisan)
Re-needinfo'ing for comment 18. Seems something is still going wrong here, but I'm not sure what...
Flags: needinfo?(amarchesini)
I cannot reproduce this issue. Oana, can you please ping me on IRC? Thanks a lot!
Flags: needinfo?(amarchesini)
I retested everything on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 using latest Nightly 59 and beta 58.0b3 and the bug is not reproducing anymore.
On the other hand, on Firefox 57.0 - build 4 using the same platforms, the bug is still reproducing. Considering the fact that this bug depends on bug 1410416 and that one is marked as won't fix on firefox57, my guess is that this one won't be fixed either. 
Based on this information, I could closed the bug and if the problem is reproducible then it can be opened again.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.