Port recent Quota Manager/Service Worker related changes in sanitizer.js to Android

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

Trunk
Firefox 61
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+, firefox56 wontfix, firefox57 wontfix, firefox58+ wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
[Tracking Requested - why for this release]:

Recently there was some brouhaha about some bits and pieces of data relating to IndexedDB, Service Workers, etc. not being properly cleared up when clearing private data.
This has been fixed on Desktop, but Android's Sanitizer fork (https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js) apparently has been forgotten about.

We should port the relevant changesets (https://hg.mozilla.org/mozilla-central/log?rev=0fbe00ad0203%3A3624656ebe46+and+filelog(browser%2Fbase%2Fcontent%2Fsanitize.js)) to Android as well.
(Assignee)

Comment 1

2 years ago
(In reply to Jan Henning [:JanH] from comment #0)
Android's Sanitizer fork
> (https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/
> content/browser.js)

Wrong link - that should read
https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm

Updated

2 years ago
tracking-fennec: ? → +
Priority: -- → P2
Track 58+.

Hi Nevin,
Can you take this on or help us find an owner? Thanks.
Flags: needinfo?(cnevinchen)
Hi Jan 
I think you are the owner and it's related to sesson restore.
Please correct me if I'm wrong.
Thanks!
Component: General → Session Restore
Flags: needinfo?(cnevinchen) → needinfo?(jh+bugzilla)
(Assignee)

Comment 5

2 years ago
(In reply to Nevin Chen [:nechen] from comment #4)
> I think you are the owner
That's news to me as far as the Sanitizer is concerned.
> and it's related to sesson restore.
Only in so far as some of the data that can be cleaned is part of the session store, but other than that not really.

But if there's no one else around I suppose I can take a look some time this or next week.
Assignee: nobody → jh+bugzilla
Component: Session Restore → General
Flags: needinfo?(jh+bugzilla)
(Assignee)

Updated

2 years ago
Depends on: 1424523
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
In case we still want this in 58, too, I can do a separate Beta patch just for part 2, so we don't have to uplift the the additional changes from part 1 and bug 1424523 as well.
Attachment #8936152 - Flags: review?(mak77) → review?(standard8)
I've stolen review from Marco to help him out with his queue.

Comment 11

a year ago
mozreview-review
Comment on attachment 8936152 [details]
Bug 1415342 - Part 1 - Move OfflineAppCacheHelper to Toolkit.

https://reviewboard.mozilla.org/r/206884/#review213444

This looks good. r=Standard8

::: toolkit/modules/moz.build:110
(Diff revision 1)
>  
>  with Files('ObjectUtils.jsm'):
>      BUG_COMPONENT = ('Toolkit', 'Telemetry')
>  
> +with Files("offlineAppCache.jsm"):
> +    BUG_COMPONENT = ("Firefox", "Preferences")

nit: Since there's a Toolkit / Preferences component I think you could change "Firefox" to "Toolkit" here now.
Attachment #8936152 - Flags: review?(standard8) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8936153 [details]
Bug 1415342 - Part 2 - Copy offlineApps sanitizing code from desktop implementation.

https://reviewboard.mozilla.org/r/206886/#review213472

::: browser/base/content/sanitize.js:196
(Diff revision 1)
> +  // When making any changes to the sanitize implementations here,
> +  // please check whether the changes are applicable to Android
> +  // (mobile/android/modules/Sanitizer.jsm) as well.

It would be great to add comments to Fennec's Sanitizer.jsm that help describe how and why it deviates from its upstream to make it easier to accomplish this.  For example, stating when things are directly copied-and-pasted, or where things have to be altered for various reasons.

As I understand things, Fennec has a number of subsystems like History where it has its own implementation where it's inappropriate and/or misleading to use the non-Fennec targeted logic, leading to the fork.

Short-term, but longer-term than this bug, I think we want to try and do a better job of de-forking the sanitization logic from here and also in the somewhat redundant implementations in ForgetAboutSite.jsm and other places.  It's an emergent mess and not Fennec's fault, but anything you cando here to make it easier for someone to take those next steps will be very helpful.

::: mobile/android/modules/Sanitizer.jsm:165
(Diff revision 1)
> -        return new Promise(function(resolve, reject) {
> -          // AppCache
> +        // AppCache
> -          // This doesn't wait for the cleanup to be complete.
> +        // This doesn't wait for the cleanup to be complete.
> -          OfflineAppCacheHelper.clear();
> +        OfflineAppCacheHelper.clear();
>  
> +        // LocalStorage

This logic is imminently about to change in bug 1252998 :(
(Assignee)

Updated

a year ago
Depends on: 1252998
(Assignee)

Updated

a year ago
See Also: → 1425338
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8936153 [details]
Bug 1415342 - Part 2 - Copy offlineApps sanitizing code from desktop implementation.

https://reviewboard.mozilla.org/r/206886/#review213472

> It would be great to add comments to Fennec's Sanitizer.jsm that help describe how and why it deviates from its upstream to make it easier to accomplish this.  For example, stating when things are directly copied-and-pasted, or where things have to be altered for various reasons.
> 
> As I understand things, Fennec has a number of subsystems like History where it has its own implementation where it's inappropriate and/or misleading to use the non-Fennec targeted logic, leading to the fork.
> 
> Short-term, but longer-term than this bug, I think we want to try and do a better job of de-forking the sanitization logic from here and also in the somewhat redundant implementations in ForgetAboutSite.jsm and other places.  It's an emergent mess and not Fennec's fault, but anything you cando here to make it easier for someone to take those next steps will be very helpful.

Good idea, will do that in a separate bug (bug 1425338).

> This logic is imminently about to change in bug 1252998 :(

I guess I'll prepare a patch based on those changes then and wait for that bug to land first. Thanks for the heads-up.
(Assignee)

Updated

a year ago
Attachment #8936153 - Flags: review?(s.kaspari)
(Assignee)

Updated

a year ago
Attachment #8936154 - Flags: review?(s.kaspari)
[triage] CRitical - not deleting private data.
Priority: P2 → P1
Flags: needinfo?(sdaswani)

Comment 15

a year ago
Sure put on your critical list Mike.
Flags: needinfo?(sdaswani)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1449920
(Assignee)

Updated

a year ago
See Also: 1425338
Duplicate of this bug: 1425338
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8936152 - Flags: review?(mak77)
(Assignee)

Comment 22

a year ago
mozreview-review-reply
Comment on attachment 8936153 [details]
Bug 1415342 - Part 2 - Copy offlineApps sanitizing code from desktop implementation.

https://reviewboard.mozilla.org/r/206886/#review213472

> Good idea, will do that in a separate bug (bug 1425338).

Or actually now in part 4 of this bug.

Comment 23

a year ago
mozreview-review
Comment on attachment 8936153 [details]
Bug 1415342 - Part 2 - Copy offlineApps sanitizing code from desktop implementation.

https://reviewboard.mozilla.org/r/206886/#review241938
Attachment #8936153 - Flags: review?(esawin) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8936154 [details]
Bug 1415342 - Part 3 - Clear MediaMmanager deviceIds when clearing cookies.

https://reviewboard.mozilla.org/r/206888/#review241940
Attachment #8936154 - Flags: review?(esawin) → review+

Comment 25

a year ago
mozreview-review
Comment on attachment 8967492 [details]
Bug 1415342 - Part 4 - Document differences between desktop and mobile Sanitizer implementations.

https://reviewboard.mozilla.org/r/236150/#review241958

Thank you so much for adding this documentation.  I really do think it will be invaluable moving forward!
Attachment #8967492 - Flags: review?(bugmail) → review+
(In reply to Andrew Sutherland [:asuth] from comment #25)
> Comment on attachment 8967492 [details]
> Bug 1415342 - Part 4 - Document differences between desktop and mobile
> Sanitizer implementations.
> 
> https://reviewboard.mozilla.org/r/236150/#review241958
> 
> Thank you so much for adding this documentation.  I really do think it will
> be invaluable moving forward!

I'd like to second that, really useful, thank you!

Comment 27

a year ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/25d665693e38
Part 1 - Move OfflineAppCacheHelper to Toolkit. r=standard8
https://hg.mozilla.org/integration/autoland/rev/4913f1571ece
Part 2 - Copy offlineApps sanitizing code from desktop implementation. r=esawin
https://hg.mozilla.org/integration/autoland/rev/d7e30b8499cd
Part 3 - Clear MediaMmanager deviceIds when clearing cookies. r=esawin
https://hg.mozilla.org/integration/autoland/rev/36ff321fc1b8
Part 4 - Document differences between desktop and mobile Sanitizer implementations. r=asuth
(Assignee)

Comment 30

a year ago
Oops, yes, I overlooked that.
Flags: needinfo?(jh+bugzilla)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 31cacf1ce7f18595563fc6e57202cdb5a5118978 -d 154e987909aa: rebasing 459372:31cacf1ce7f1 "Bug 1415342 - Part 1 - Move OfflineAppCacheHelper to Toolkit. r=standard8"
merging browser/base/content/test/general/browser_offlineQuotaNotification.js
merging browser/components/preferences/in-content/tests/siteData/head.js
merging browser/modules/Sanitizer.jsm
merging browser/modules/moz.build
merging mobile/android/modules/Sanitizer.jsm
merging toolkit/modules/moz.build
warning: conflicts while merging browser/modules/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c0977906362b1b54b2d504a7bd9fe56f388f59a6 -d 96867a07e1a3: rebasing 459375:c0977906362b "Bug 1415342 - Part 1 - Move OfflineAppCacheHelper to Toolkit. r=standard8"
merging browser/modules/moz.build
warning: conflicts while merging browser/modules/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

a year ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/40e13ce2180c
Part 1 - Move OfflineAppCacheHelper to Toolkit. r=standard8
https://hg.mozilla.org/integration/autoland/rev/6ed075663426
Part 2 - Copy offlineApps sanitizing code from desktop implementation. r=esawin
https://hg.mozilla.org/integration/autoland/rev/78ea2b0c5f65
Part 3 - Clear MediaMmanager deviceIds when clearing cookies. r=esawin
https://hg.mozilla.org/integration/autoland/rev/c204a2731307
Part 4 - Document differences between desktop and mobile Sanitizer implementations. r=asuth
(Assignee)

Updated

a year ago
Duplicate of this bug: 1456216
You need to log in before you can comment on or make changes to this bug.