Details
- Reviewers
mkaply jesup edenchuang tobyp - Group Reviewers
dom-worker-reviewers necko-reviewers - Commits
- rFIREFOXAUTOLAND828679599427: Bug 2045811: Add a site policy to disable service workers. r=mkaply,dom-worker…
- Bugzilla Bug ID
- 2045811
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
Event Timeline
Code analysis found 1 defect in diff 1341073:
- 1 defect found by eslint (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1341073.
I am confused about the word "disable."
"Disable" would make me think the ServiceWorker registration/unregistration is also disabled since ServiceWorker doesn't only intercept fetch.
In fact, the patch just introduces a new mechanism for skipping the ServiceWorker interception by the document's EnterprisePolicies.
Maybe we can have a better name than "Disable."
And considering we already have an interception-skipping mechanism there, which is by setting nsLoadFlags with nsIChannel::LOAD_BYPASS_SERVICE_WORKER.
I am thinking we could reuse the original one instead of introducing a new one.
Disabling is what we want, it is intended that registration/unregistration is also disabled for matching sites and I believe that the patch implements this.
In fact, the patch just introduces a new mechanism for skipping the ServiceWorker interception by the document's EnterprisePolicies.
Maybe we can have a better name than "Disable."
By patching the ServiceWorkersEnabled function it also disables access to the JS APIs exposed on the window object. Are there other things we should be disabling?
And considering we already have an interception-skipping mechanism there, which is by setting nsLoadFlags with nsIChannel::LOAD_BYPASS_SERVICE_WORKER.
I am thinking we could reuse the original one instead of introducing a new one.
Thanks, I hadn't spotted that, I will try to update the patch to use it.
Code analysis found 2 defects in diff 1345771:
- 2 build errors found by clang-tidy
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1345771.
Code analysis found 2 defects in diff 1345893:
- 2 build errors found by clang-tidy
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1345893.
Aha, my fault; I did not see that patched part in ServiceWorkerEnabled(). So it is really disabled ServiceWorker according to the policy.
Thank you for clarifying this.
And considering we already have an interception-skipping mechanism there, which is by setting nsLoadFlags with nsIChannel::LOAD_BYPASS_SERVICE_WORKER.
I am thinking we could reuse the original one instead of introducing a new one.Thanks, I hadn't spotted that, I will try to update the patch to use it.
And thank you for adapting the implementation to nsIChannel::LOAD_BYPASS_SERVICE_WORKER.
This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!
