Closed Bug 1415342 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android Graveyard :: General, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
enhancement

Tracking

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

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
fennec + ---
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 + wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(4 files)

Bug 1415342 - Part 1 - Move OfflineAppCacheHelper to Toolkit.
59 bytes, text/x-review-board-request
Details
Bug 1415342 - Part 2 - Copy offlineApps sanitizing code from desktop implementation.
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1415342 - Part 3 - Clear MediaMmanager deviceIds when clearing cookies.
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1415342 - Part 4 - Document differences between desktop and mobile Sanitizer implementations.
59 bytes, text/x-review-board-request
asuth
: review+
Details 
[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.
(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
tracking-fennec: ? → +
Priority: -- → P2
Track 58+.

Hi Nevin,
Can you take this on or help us find an owner? Thanks.
tracking-firefox58: ? → +
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)
(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)
Depends on: 1424523
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 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 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 :(
Depends on: 1252998
See Also: → 1425338
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.
Attachment #8936153 - Flags: review?(s.kaspari)
Attachment #8936154 - Flags: review?(s.kaspari)
[triage] CRitical - not deleting private data.
Priority: P2 → P1
Sure put on your critical list Mike.
Flags: needinfo?(sdaswani)
See Also: 1425338 →
Attachment #8936152 - Flags: review?(mak77)
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 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 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 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!
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
Oops, yes, I overlooked that.
Flags: needinfo?(jh+bugzilla)
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)
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)
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.