1401681 - WebExtension API: browsingData.removeHistory also removes service workers

Status: RESOLVED WONTFIX

(WebExtensions :: General, defect, P3)

Description

[dessant]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20100101

Steps to reproduce:

1) Visit pinterest.com or any other site which registers a service worker.
2) Open about:serviceworkers and verify that the service worker is there.
3) Call browser.browsingData.removeHistory({}) from a background script.
4) Reload about:serviceworkers.


Actual results:

The service worker is no longer listed. It was removed together with the browsing history.


Expected results:

The service worker should have been left intact.

The API works as expected in Chrome and Opera, the service worker stays registered after removing the browsing history (chrome://inspect/#service-workers).

Comment 1

[Bob Silverberg [:bsilverberg]]

I have confirmed this behaviour in Nightly and will investigate further.

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1401681 - browsingData.removeHistory should not remove service workers,

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

Comment 3

[Bob Silverberg [:bsilverberg]]

Marco, the reason this API currently removes history *and* service workers is because behind the scenes it calls sanitizer.items.history.clear() [1], which does more than just specifically clear history. I proposed to change the behaviour of browsingData.removeHistory to just remove history (basically just doing what happens at [2]), but some questions have been raised.

1. Is it intentional that sanitizer.items.history.clear() removes service workers?
2. Do you feel that removing service workers should be part of an API called removeHistory? Kris feels that this is appropriate, but I'm not sure.

Note that we do have a separate browsingData API that removes just service workers, which is what led me to believe that removeHistory should just removing browser history.

[1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/base/content/sanitize.js#330
[2] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/base/content/sanitize.js#335-343

Comment 4

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> 1. Is it intentional that sanitizer.items.history.clear() removes service
> workers?

The sanitizer groups things according to the UI needs. Looks like service workers ended up grouped with history.
It could make sense, Service workers data, afaik, is volatile by its own nature, and represents a sort of history tracking. If a page has SW data, it has been visited. So it seems to make sense that it's cleared along with history.

> 2. Do you feel that removing service workers should be part of an API called
> removeHistory? Kris feels that this is appropriate, but I'm not sure.

This is honestly a question for someone involved in Service Workers development.

Note that, if you only clear history (like call PlacesUtils.history.clear()) you'll not clear the network predictor and everything that depends on "browser:purge-session-history" that is a bunch of stuff. Indeed it's exactly this notification that clears Service Workers: http://searchfox.org/mozilla-central/search?q=symbol:M_3fb73d7c51f57dcb12d932f58a4d097499b9bbdc&redirect=false
So, if we don't want SW to be cleared with history, the only solution I see is that SW doesn't use the "browser:purge-session-history" notification for its cleanup.

But as I said, imo the current behavior is correct, this needs an overlook by someone involved with SW. I think bkelly is working in that area.

Comment 5

[Bob Silverberg [:bsilverberg]]

Thanks for the reply, Marco. aswan also pointed me to bug 1047098, which seems to be where this was added.

I can see how in some senses of the meaning, removeHistory could be considered to include SW, but I'm not sure about the browsingData API because it includes granular items [1] which include both "history" and "serviceWorkers". Because of this I feel like "history" should just remove browsing history and "serviceWorkers" should be used if one wants to remove SW's.

Ben's feedback on this would be appreciated as well.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/DataTypeSet

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Service workers must be cleared if storage like IDB, Cache API, etc are cleared.  Users were surprised when clearing history left potential tracking data in IDB.  So now that we are clearing that we must clear the service workers as well.  If we don't clear service workers then we risk sites breaking if they configured IDB state in their install or activate event.

Comment 7

[Bob Silverberg [:bsilverberg]]

Fair enough. Based on everything above it sounds like we _should_ be clearing service workers when clearing history, so I'm going to mark this as wontfix, but an update to the docs in MDN specifically calling out this incompatibility with Chrome is in order.

Comment 8

[Will Bamberg [:wbamberg]]

I've opened https://github.com/mdn/browser-compat-data/pull/569 to update the compat notes.

Comment 9

[Will Bamberg [:wbamberg]]

These pages now have updated notes:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/remove#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeHistory#Browser_compatibility

Marking as dev-doc-complete.