1410416 - SiteDateManager.jsm should call `propagateUnregister` to remove service workers

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Fischer [:Fischer]]

As the bug 1407936 comment 10, SiteDateManager.jsm should call `propagateUnregister` to remove service workers

Comment 1

[Fischer [:Fischer]]

Attachment: Bug 1410416 - Part 1 - Make SiteDateManager.jsm call `propagateUnregister` to remove service workers.patch

This is Part 1. Writing Part 2, tests...

Comment 2

[Fischer [:Fischer]]

Attachment: Bug 1410416 - Part 1 - Have SiteDateManager.jsm call `propagateUnregister` to remove service workers.patch

Comment 3

[Fischer [:Fischer]]

Attachment: Bug 1410416 - Part 2 - Add test cases about removing service workers.patch

Comment 4

[Fischer [:Fischer]]

Comment on attachment 8921065 [details] [diff] [review]
Bug 1410416 - Part 1 - Have SiteDateManager.jsm call `propagateUnregister` to remove service workers.patch

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

::: browser/components/preferences/SiteDataManager.jsm
@@ +50,4 @@
>      this._getQuotaUsagePromise = new Promise(resolve => {
>        let onUsageResult = request => {
>          let items = request.result;
> +        if (items) {

Not sure if that was expected (seems not from the idl[1] and the cpp[2]. please correct me if i was wrong) but did see the error that `request.result` is null during test so added this `if (items)` quard.

[1] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/dom/quota/nsIQuotaManagerService.idl#56
[2] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/dom/quota/ActorsChild.cpp#177

@@ +228,5 @@
> +        unregisterSucceeded: resolve,
> +        unregisterFailed: resolve, // We don't care about failures.
> +        QueryInterface: XPCOMUtils.generateQI([Ci.nsIServiceWorkerUnregisterCallback])
> +      };
> +      serviceWorkerManager.propagateUnregister(serviceWorker.principal, unregisterCallback, serviceWorker.scope);

This part is borrowed from bug 1407936, thanks.

@@ +240,5 @@
>      for (let i = 0; i < serviceWorkers.length; i++) {
>        let sw = serviceWorkers.queryElementAt(i, Ci.nsIServiceWorkerRegistrationInfo);
> +      // Sites are grouped and removed by host so we unregister service workers by the same host as well
> +      if (targetHosts.includes(sw.principal.URI.host)) {
> +        promises.push(this._unregisterServiceWorker(sw));

Just looked up `removeAndPropagate`, it dose the similar host-matching batch job[1][2] with scope uri part. Here try to do batch unregistration as well so only have to call `getAllRegistrations` once. But here went with `principal.URI.host` (should be ok. here compares hosts both from principal. Expect the same format returned) if something might be wrong please correct me. Thank you 

[1] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/dom/workers/ServiceWorkerManager.cpp#3797
[2] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/dom/workers/ServiceWorkerManager.cpp#3684

Comment 5

[Fischer [:Fischer]]

Comment on attachment 8921067 [details] [diff] [review]
Bug 1410416 - Part 2 - Add test cases about removing service workers.patch

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

::: browser/components/preferences/in-content/tests/browser_siteData.js
@@ +50,5 @@
>  function getQuotaUsage(origin) {
>    return new Promise(resolve => {
> +    let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(origin);
> +    Services.qms.getUsageForPrincipal(principal, request => {
> +      let usage = request.result ? request.result.usage : 0;

Add `request.result ?` guard because saw null `request.result` error during testing...

@@ +315,5 @@
> +  await promiseServiceWorkersCleared();
> +  quotaUsage = await getQuotaUsage(TEST_QUOTA_USAGE_ORIGIN);
> +  is(quotaUsage, 0, `The quota usage of ${TEST_QUOTA_USAGE_ORIGIN} should be removed`);
> +  // Try to check quota usage again after a while
> +  await new Promise(resolve => setTimeout(resolve, 3000));

Try to wait for a while then check quota usage again to see if it is about timing issue but still failed.

Comment 6

[Fischer [:Fischer]]

Comment on attachment 8921065 [details] [diff] [review]
Bug 1410416 - Part 1 - Have SiteDateManager.jsm call `propagateUnregister` to remove service workers.patch

Hi Baku,

This patch makes SiteDateManager.jsm follow the bug 1407936 comment 10 to call `propagateUnregister` to remove service workers.

However, I face the intermittent failure in the Part 2 tests[1]. Not sure where could be the problem. That would be great you could provide some suggestion, thanks.

The intermittent failure is we could see the service worker was removed but the quota usage wasn't able to be removed sometimes.

The code flow in this Part 1 is as below:
Case 1: Remove all
  1. Call `SiteDateManager.removeAll`
  2. In `removeAll`, it will clear cookies, appcahe, http cache then
  3. Get all sw registrations with `serviceWorkerManager.getAllRegistrations` then
  4. loop all registrations to unregister with `propagateUnregister` then wait for this work done
  5. After unregistrations done, call QuotaManagerService's `GetUsage` to refresh all usages
  6. loop usages and remove one by one
  7. After all usages removal promises are resolved, call `SiteDateManager.updateSites` to refresh sites list

Case 2: Remove multiple sites
  1. Call `SiteDateManager.remove`
  2. In `remove`, it will remove cookies, appcahe, http cache for target sites then
  3. Get all sw registrations with `serviceWorkerManager.getAllRegistrations` then
  4. filter out service workers for target sites and unregister those workers then wait for this work done
  5. After unregistrations done, loop sites to remove quota usage one by one
  7. After usages removal promises are resolved, call `SiteDateManager.updateSites` to refresh sites list


The Part 2 tests are testing the above 2 cases. Basically a test site_data_test.html is loaded and site_data_test_service_worker.js is registered for that test page.
The test flow is like:
  1. Open site_data_test.html#with_test_service_worker
  2. In the test page, it saves to indexedDB and registers service worker
  3. After saving and registration, do `window.dispatchEvent(new Event("site_data_test_html_loaded")`
  4. The test knows the test is loaded then close the test page
  5. Open about:preferences and go to the Site Data section
  6. Check quota usage and service worker really exist for the test page
  7. Manipulate the Site Data section to remove the test page's site data
  8. Check the service worker and the quota usage are removed


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f9668fc86275f09a4ce63e68327ad2ca8863c1

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8921065 [details] [diff] [review]
Bug 1410416 - Part 1 - Have SiteDateManager.jsm call `propagateUnregister` to remove service workers.patch

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

::: browser/components/preferences/SiteDataManager.jsm
@@ +50,4 @@
>      this._getQuotaUsagePromise = new Promise(resolve => {
>        let onUsageResult = request => {
>          let items = request.result;
> +        if (items) {

This should be:

if (request.resultCode != Cr.NS_OK) {
  return;
}

let items = request.result;
...

@@ +266,5 @@
> +    if (targetSites.length > 0) {
> +      this._removeServiceWorkersForSites(targetSites)
> +          .then(() => {
> +            let promises = targetSites.map(s => this._removeQuotaUsage(s));
> +            Promise.all(promises).then(() => this.updateSites());

I would do:


this._removeServiceWorkersForSites(targetSites)
    .then(() => {
      let promises = targetSites.map(s => this._removeQuotaUsage(s));
      return Promise.all(promises);
    })
    .then(() => this.updateSites());

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8921067 [details] [diff] [review]
Bug 1410416 - Part 2 - Add test cases about removing service workers.patch

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

::: browser/components/preferences/in-content/tests/browser_siteData.js
@@ +50,5 @@
>  function getQuotaUsage(origin) {
>    return new Promise(resolve => {
> +    let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(origin);
> +    Services.qms.getUsageForPrincipal(principal, request => {
> +      let usage = request.result ? request.result.usage : 0;

check here the request.resultCode instead.

@@ +315,5 @@
> +  await promiseServiceWorkersCleared();
> +  quotaUsage = await getQuotaUsage(TEST_QUOTA_USAGE_ORIGIN);
> +  is(quotaUsage, 0, `The quota usage of ${TEST_QUOTA_USAGE_ORIGIN} should be removed`);
> +  // Try to check quota usage again after a while
> +  await new Promise(resolve => setTimeout(resolve, 3000));

This could generate intermittent failures... Can you find another better solution?

Comment 9

[Fischer [:Fischer]]

(In reply to Andrea Marchesini [:baku] from comment #7)
> Comment on attachment 8921065 [details] [diff] [review]
> Bug 1410416 - Part 1 - Have SiteDateManager.jsm call `propagateUnregister`
> to remove service workers.patch
> 
> Review of attachment 8921065 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/SiteDataManager.jsm
> @@ +50,4 @@
> >      this._getQuotaUsagePromise = new Promise(resolve => {
> >        let onUsageResult = request => {
> >          let items = request.result;
> > +        if (items) {
> 
> This should be:
> 
> if (request.resultCode != Cr.NS_OK) {
>   return;
> }
> 
> let items = request.result;
> ...
> 
> @@ +266,5 @@
> > +    if (targetSites.length > 0) {
> > +      this._removeServiceWorkersForSites(targetSites)
> > +          .then(() => {
> > +            let promises = targetSites.map(s => this._removeQuotaUsage(s));
> > +            Promise.all(promises).then(() => this.updateSites());
> 
> I would do:
> 
> 
> this._removeServiceWorkersForSites(targetSites)
>     .then(() => {
>       let promises = targetSites.map(s => this._removeQuotaUsage(s));
>       return Promise.all(promises);
>     })
>     .then(() => this.updateSites());
Thanks both updated

Comment 10

[Fischer [:Fischer]]

(In reply to Andrea Marchesini [:baku] from comment #8)
> Comment on attachment 8921067 [details] [diff] [review]
> Bug 1410416 - Part 2 - Add test cases about removing service workers.patch
> Review of attachment 8921067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/tests/browser_siteData.js
> @@ +50,5 @@
> >  function getQuotaUsage(origin) {
> >    return new Promise(resolve => {
> > +    let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(origin);
> > +    Services.qms.getUsageForPrincipal(principal, request => {
> > +      let usage = request.result ? request.result.usage : 0;
> 
> check here the request.resultCode instead.
> 
Updated thanks

> @@ +315,5 @@
> > +  // Try to check quota usage again after a while
> > +  await new Promise(resolve => setTimeout(resolve, 3000));
> 
> This could generate intermittent failures... Can you find another better solution?
Definitely will remove this 2nd check in the production code. This is just for double checking if the uncleared quota usage would be removed after a while.

Comment 11

[Fischer [:Fischer]]

Hi Baku,

It seems the intermittent failure we encountered here has to do with bug 1407936 comment 29 and bug 1407936 comment 30.
I tried 2 experiment test cases and the results are confused.

# Experiment 1: Register and unregister service worker:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1204a1ecaa2996bc8e3c5c969a0f676460ce4a38

We can see the service worker was removed but the quota usage wasn't cleared in the end.
From the log, we can see
```
// Before `SiteDataManager.removeAll, there are 152213 bytes of quota usage
10:21:33     INFO -  255 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_siteData_reduced_test.js | The quota usage should not be 0 - 152213 > 0 -

// After `SiteDataManager.removeAll, there are 65536 bytes of quota usage.
// Looks like this 65536 bytes was that newly created DOM cache
10:21:33    ERROR -  256 INFO TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_siteData_reduced_test.js | The quota usage should be removed - Got 65536, expected 0
```
All the failures found an unexpected 65536 bytes of quota usages 

# Experiment 2: Register but NOT unregister service worker
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db34f15e06859c510938d5913b2d4ce6c6630d53
All the tests are passed. We can see even we didn't unregister sw but this time quota usage was cleared in the end.

What may cause this?

Comment 12

[Andrea Marchesini [:baku]]

I would do a couple of checks:

1. can you check if the code calls the cleaning of the quota for that particular origin? And that has to be done before continuing the test.

2. can you also check if the quota directory contains something? And what's the timestamp of the files in the folder for that origin?

Comment 13

[Fischer [:Fischer]]

(In reply to Andrea Marchesini [:baku] from comment #12)
> I would do a couple of checks:
> 
> 1. can you check if the code calls the cleaning of the quota for that
> particular origin? And that has to be done before continuing the test.
> 
Yes, the test did check quota usage after call of clearing quota usage.
Try[1] to not save to indexedDB but only register service workers so only have quota usage brought by service worker.
The result is the same: we got intermittent failure about uncleared quota usage of 65536 bytes.

Looks like the intermittent failure we encountered here is the same as the video test in bug 1407936 comment 31.
In the test video, the 1st time clearing, the DOM cache still exists. Have to clear again to remove that DOM cache.
Try[2] to clear twice then the results basically are positive.

But the same intermittent failure still could be seen with `removeAndPropagate` api[3] and another "SQLite database connection is busy" error happens with `removeAndPropagate`.
As the bug 1407936 comment 10, better switch to `propagateUnregister` api.

Will update the patch to switch to `propagateUnregister` first.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=76803be15b06b41b6374a7df722a3482334c44e3
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbc8aa6ac699d40ebcbeea413c73f8a13d7475a8
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c218738bda04c47b4d4c3c7d9c91ec43f8b48fdd


> 2. can you also check if the quota directory contains something? And what's
> the timestamp of the files in the folder for that origin?
Looks like the DOM cache folder and caches.splite wasn't removed. The creation time is still unchanged before and after unregistering service worker and clearing quota usage. (Try to wait for 1 min then clear all)

Comment 14

[Fischer [:Fischer]]

Attachment: Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

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

Comment 15

[Fischer [:Fischer]]

Attachment: Bug 1410416 - Part 2 - Add test cases about removing service workers,

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

Comment 16

[Fischer [:Fischer]]

Comment on attachment 8924089 [details]
Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

https://reviewboard.mozilla.org/r/195284/#review200364

::: commit-message-d3910:1
(Diff revision 1)
> +Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers, r?baku

Hi Baku,

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c02b11150c71484332509b7c5027096a294e8ca2

p.s. please let me know if you prefer the Bugzilla Splinter Review

Thanks

::: browser/components/preferences/SiteDataManager.jsm:52
(Diff revision 1)
>      // Clear old data and requests first
>      this._sites.clear();
>      this._cancelGetQuotaUsage();
>      this._getQuotaUsagePromise = new Promise(resolve => {
>        let onUsageResult = request => {
> +        if (request.resultCode == Cr.NS_OK) {

Use `Cr.NS_OK` based on the last feedback: https://bugzilla.mozilla.org/show_bug.cgi?id=1410416#c7

::: browser/components/preferences/SiteDataManager.jsm:274
(Diff revision 1)
> +      this._removeServiceWorkersForSites(targetSites)
> +          .then(() => {
> +            let promises = targetSites.map(s => this._removeQuotaUsage(s));
> +            return Promise.all(promises);
> +          })
> +          .then(() => this.updateSites());

Updated based on the last feedback: https://bugzilla.mozilla.org/show_bug.cgi?id=1410416#c7

Comment 17

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

https://reviewboard.mozilla.org/r/195286/#review200366

::: browser/components/preferences/in-content/tests/head.js:257
(Diff revision 1)
>      let result = this.fakeSites.map(site => ({
>        origin: site.principal.origin,
>        usage: site.usage,
>        persisted: site.persisted
>      }));
> -    onUsageResult({ result });
> +    onUsageResult({ result, resultCode: Components.results.NS_OK });

For some tests not about real data manipulation, we feed them fake data. This update is for that now SiteDataManager will check `Cr.NS_OK`.

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8924089 [details]
Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

https://reviewboard.mozilla.org/r/195284/#review200398

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

https://reviewboard.mozilla.org/r/195286/#review200400

Comment 20

[Fischer [:Fischer]]

Comment on attachment 8924089 [details]
Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195284/diff/1-2/

Comment 21

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195286/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2d97da8bfed
Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers, r=baku
https://hg.mozilla.org/integration/autoland/rev/b0d6deea2bca
Part 2 - Add test cases about removing service workers, r=baku

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for linting failures in browser/components/preferences/in-content/tests/browser_siteData.js:

https://hg.mozilla.org/integration/autoland/rev/6c869780c87ec220a3530de3ae9bb06e340b7151

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b0d6deea2bca0a798a2966630f7cfc25e494507c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141779764&repo=autoland

 TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/preferences/in-content/tests/browser_siteData.js:286:22 | gBrowser.selectedBrowser.contentWindow is a possible Cross Process Object Wrapper (CPOW). (mozilla/no-cpows-in-tests)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/preferences/in-content/tests/browser_siteData.js:308:22 | gBrowser.selectedBrowser.contentWindow is a possible Cross Process Object Wrapper (CPOW). (mozilla/no-cpows-in-tests)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/preferences/in-content/tests/browser_siteData.js:315:13 | gBrowser.selectedBrowser.contentWindow is a possible Cross Process Object Wrapper (CPOW). (mozilla/no-cpows-in-tests)

Comment 24

[Fischer [:Fischer]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #23)
> Backed out for linting failures in
> browser/components/preferences/in-content/tests/browser_siteData.js:
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 6c869780c87ec220a3530de3ae9bb06e340b7151
> 
> Push with failure:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=b0d6deea2bca0a798a2966630f7cfc25e494507c&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=141779764&repo=autoland
> 
>  TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/browser/components/preferences/in-content/
> tests/browser_siteData.js:286:22 | gBrowser.selectedBrowser.contentWindow is
> a possible Cross Process Object Wrapper (CPOW). (mozilla/no-cpows-in-tests)
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/browser/components/preferences/in-content/
> tests/browser_siteData.js:308:22 | gBrowser.selectedBrowser.contentWindow is
> a possible Cross Process Object Wrapper (CPOW). (mozilla/no-cpows-in-tests)
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/browser/components/preferences/in-content/
> tests/browser_siteData.js:315:13 | gBrowser.selectedBrowser.contentWindow is
> a possible Cross Process Object Wrapper (CPOW). (mozilla/no-cpows-in-tests)
We don't like cpow in tests. This new eslint rule was added in the recent days...

Comment 25

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195286/diff/2-3/

Comment 26

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195286/diff/3-4/

Comment 27

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

https://reviewboard.mozilla.org/r/195286/#review201682

::: browser/components/preferences/in-content/tests/browser_siteData.js:93
(Diff revision 4)
>    return TestUtils.topicObserved("cookie-changed", (subj, data) => {
>      return data === "cleared";
>    });
>  }
>  
> +async function loadServiceWorkerTestPage(url) {

`loadServiceWorkerTestPage` is added for not using cpow

::: browser/components/preferences/in-content/tests/browser_siteData.js:294
(Diff revision 4)
>  
> +// Test clearing service wroker through the "Clear All" button
> +add_task(async function() {
> +  await SpecialPowers.pushPrefEnv({set: [["browser.storageManager.enabled", true]]});
> +  // Register a test service
> +  await loadServiceWorkerTestPage(TEST_SERVICE_WORKER_URL);

Updated to load the test without cpow codes

::: browser/components/preferences/in-content/tests/browser_siteData.js:314
(Diff revision 4)
> +
> +// Test clearing service wroker through the settings panel
> +add_task(async function() {
> +  await SpecialPowers.pushPrefEnv({set: [["browser.storageManager.enabled", true]]});
> +  // Register a test service worker
> +  await loadServiceWorkerTestPage(TEST_SERVICE_WORKER_URL);

Updated to load the test without cpow codes

::: browser/components/preferences/in-content/tests/browser_siteData.js:322
(Diff revision 4)
> +  await promiseServiceWorkerRegisteredFor(TEST_SERVICE_WORKER_URL);
> +  // Open the Site Data Settings panel and remove the site
> +  await openSiteDataSettingsDialog();
> +  let acceptRemovePromise = promiseAlertDialogOpen("accept");
> +  let updatePromise = promiseSiteDataManagerSitesUpdated();
> +  ContentTask.spawn(gBrowser.selectedBrowser, { TEST_OFFLINE_HOST }, args => {

Updated to manipulate the page without cpow codes

Comment 28

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

Hi Baku,

Bug 1412778 enables no cpow in tests so made a bit updates.
Would you have a look?

I left the comments where the updates are made.

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14dd62b0543f8102059bb5bb3f2885c89f101812

Thanks

Comment 29

[Andrea Marchesini [:baku]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

https://reviewboard.mozilla.org/r/195286/#review201688

Comment 30

[Fischer [:Fischer]]

Comment on attachment 8924089 [details]
Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195284/diff/2-3/

Comment 31

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195286/diff/4-5/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ef662f3fa22
Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers, r=baku
https://hg.mozilla.org/integration/autoland/rev/f89f5660e3d7
Part 2 - Add test cases about removing service workers, r=baku

Comment 33

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8ef662f3fa22
https://hg.mozilla.org/mozilla-central/rev/f89f5660e3d7

Comment 34

[Fischer [:Fischer]]

Comment on attachment 8924089 [details]
Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

Approval Request Comment
[Feature/Bug causing the regression]: 1401878

[User impact if declined]: FF can freezes on shutdown.

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Yes at least by the auto  and manual tests

[Needs manual test from QE? If yes, steps to reproduce]: 
1. Login to Twitter
2. Open about:serviceworkers to see there is a service worker for twitter.com
3. Open about:preferences#privacy and close Twitter and about:serviceworkers
4. Go to the Site Data preferences section
5. Click "Clear All Data" button to clear all data
6. Open about:serviceworkers to see there is no service worker for twitter.com
7. Login to Twitter again
8. Open about:serviceworkers to see there is a service worker for twitter.com
9. Open about:preferences#privacy and close Twitter and about:serviceworkers
10. Go to the Site Data preferences section
11. Click "Settings" button to open the Settings panel
12. Select twitter.com and remove it in the Settings panel
13. Open about:serviceworkers to see there is a service worker for twitter.com

[List of other uplifts needed for the feature/fix]:
Part 1 and Part 2

[Is the change risky?]: Low

[Why is the change risky/not risky?]: Basically this bug is doing the same as bug 1407936, which is uplifted around 2 weeks ago. This bug like bug 1407936 switches to the proper API to remove service worker. As the bug 1407936 comment 19 uplift reason, the old API may cause FF to freezes on shutdown.

[String changes made/needed]: None

Comment 35

[Fischer [:Fischer]]

Comment on attachment 8924090 [details]
Bug 1410416 - Part 2 - Add test cases about removing service workers,

Approval Request Comment
[Feature/Bug causing the regression]: 1401878

[User impact if declined]: FF can freezes on shutdown.

[Is this code covered by automated tests?]: Yes, this is the auto test part

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: No. This is the auto test part

[List of other uplifts needed for the feature/fix]:
Part 1 and Part 2(this part)

[Is the change risky?]: Low

[Why is the change risky/not risky?]: This is the auto test

[String changes made/needed]: None

Comment 36

[Fischer [:Fischer]]

Correct the typo:
(In reply to Fischer [:Fischer] from comment #34)
> Comment on attachment 8924089 [details]
> Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to
> remove service workers,
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: 1401878
> 
> [User impact if declined]: FF can freezes on shutdown.
> 
> [Is this code covered by automated tests?]: Yes
> 
> [Has the fix been verified in Nightly?]: Yes at least by the auto  and
> manual tests
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> 1. Login to Twitter
> 2. Open about:serviceworkers to see there is a service worker for twitter.com
> 3. Open about:preferences#privacy and close Twitter and about:serviceworkers
> 4. Go to the Site Data preferences section
> 5. Click "Clear All Data" button to clear all data
> 6. Open about:serviceworkers to see there is no service worker for
> twitter.com
> 7. Login to Twitter again
> 8. Open about:serviceworkers to see there is a service worker for twitter.com
> 9. Open about:preferences#privacy and close Twitter and about:serviceworkers
> 10. Go to the Site Data preferences section
> 11. Click "Settings" button to open the Settings panel
> 12. Select twitter.com and remove it in the Settings panel
   13. Open about:serviceworkers to see there is no service worker for twitter.com
> 
> [List of other uplifts needed for the feature/fix]:
> Part 1 and Part 2
> 
> [Is the change risky?]: Low
> 
> [Why is the change risky/not risky?]: Basically this bug is doing the same
> as bug 1407936, which is uplifted around 2 weeks ago. This bug like bug
> 1407936 switches to the proper API to remove service worker. As the bug
> 1407936 comment 19 uplift reason, the old API may cause FF to freezes on
> shutdown.
> 
> [String changes made/needed]: None

Comment 37

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8924089 [details]
Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

57 is on mozilla-release now.

Comment 38

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8924089 [details]
Bug 1410416 - Part 1: Have SiteDateManager.jsm call `propagateUnregister` to remove service workers,

This comes too late in the 57 cycle, and the benefit doesn't sound huge, so we'll let this ride the 58 train.