Disable https:// only load for ServiceWorkers when Developer Tools are open

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nsm, Assigned: jaoo)

Tracking

(Blocks 1 bug)

unspecified
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

ServiceWorkers are only allowed to be loaded from secure websites. This behaviour should be disabled when the page is being inspected by Developer Tools.

The intention is to add a DocShell flag similar to Disable Cache (Bug 864098) that can be set by devtools.

I'm not sure where to introduce the flag (LOAD_ALLOW_INSECURE_SERVICEWORKERS?), in nsIDocShell's internal flags or in nsIRequest.

For now the check may be disabled by setting the pref "dom.workers.serviceWorkers.testing.enabled" to true.

Updated

5 years ago
Assignee: nobody → jefry.reyes

Comment 1

5 years ago
Attachment #8510329 - Flags: review?(nsm.nikhil)
Jefry, it seems the attachment doesn't have all the files.
Comment on attachment 8510329 [details] [diff] [review]
devtools_disable_ssl_serviceworkers.patch

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

This is not the correct way to implement the check since flipping the pref will disable checks throughout firefox and not just for the window with devtools open. If another tab that is not being inspected were to register serviceworkers then, it would bypass the check.
I'd recommend asking on the #devtools channel the right way you can query for whether devtools are open for the current global from C++.

Nick, would you or someone else from devtools know about this?
Attachment #8510329 - Flags: review?(nsm.nikhil)
Flags: needinfo?(nfitzgerald)
AFAIK, we don't have a single "are devtools observing this global/tab/docshell?" hook, because:

(a) We don't start observing everything as soon as the toolbox is opened. The debugger, for example, currently slows down JS execution about 4x just when you open it and if a dev is only using the inspector we shouldn't slow down the JS.

(b) The platform APIs we use to observe the page come from many different (non-unified) places: SpiderMonkey's Debugger API, normal web APIs, docshell, XPCOM, WebIDL [Chrome Only] interfaces, etc...

(Ignoring debugging the whole browser for a second...) The devtools speak the Remote Debugging Protocol[0] and if a tool wants to debug a tab, at some point it has to attach to the corresponding TabActor. You could find the TabActor that is observing your given global and check if its state is "attached" or not. This seems like a pretty heavy solution.

Maybe we don't want to disable these checks whenever the devtools are open, but whenever the network monitor is open, or something like that? Narrowing down the scope might make it easier, but I'm not sure.

CC'ing Mike (who did the disable-cache stuff) and Victor (who wrote the network monitor).
Flags: needinfo?(nfitzgerald)

Comment 6

5 years ago
I would like to write a test for this, but since ServiceWorkers are disabled in browser chrome tests. How would it be possible to test this? Isn't devtools only accessible within chrome tests?
Flags: needinfo?(nsm.nikhil)
(In reply to Jefry Lagrange from comment #6)
> I would like to write a test for this, but since ServiceWorkers are disabled
> in browser chrome tests. How would it be possible to test this? Isn't
> devtools only accessible within chrome tests?

Nick, can we use some SpecialPowers API to open devtools from mochitests?
Flags: needinfo?(nsm.nikhil) → needinfo?(nfitzgerald)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #7)
> (In reply to Jefry Lagrange from comment #6)
> > I would like to write a test for this, but since ServiceWorkers are disabled
> > in browser chrome tests. How would it be possible to test this? Isn't
> > devtools only accessible within chrome tests?
> 
> Nick, can we use some SpecialPowers API to open devtools from mochitests?

Here is how we open the debugger in our mochitests:

http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/head.js#504

Usage:

http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_breakpoints-other-tabs.js#12
Flags: needinfo?(nfitzgerald)

Comment 9

5 years ago
I have attached a patch that I believe fixes this issue. Maybe some of the variable names or values of flags should change, but I believe this captures the idea of what we should be doing here. I manually tested this and it seems to work. 

I've been unable to write the javascript test. I'm totally lost and have no idea how to write it. If someone else could help by writing the test, that would be grand.
Attachment #8510329 - Attachment is obsolete: true
Attachment #8529918 - Flags: feedback?(nsm.nikhil)
Comment on attachment 8529918 [details] [diff] [review]
Disable secure strict workers with devtools.

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

While the approach is good, I'm not happy with abusing docshell and nsIRequest flags. Could you add an attribute to nsPIDOMWindow instead? We can check that in ServiceWorkerManager::Register.
Attachment #8529918 - Flags: feedback?(nsm.nikhil) → feedback+

Comment 11

4 years ago
I'm releasing this ticket so someone else can work on it.
Assignee: jefry.reyes → nobody
This bug is quite similar to bug 1137245. Once it gets landed a fix for this one should be fairly easy.
Assignee: nobody → josea.olivera
OS: Linux → All
Hardware: x86_64 → All
See Also: → 1137245
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #12)
> This bug is quite similar to bug 1137245. Once it gets landed a fix for this
> one should be fairly easy.

I meant bug 1134329, sorry for the noise.
See Also: 11372451134329
Posted patch v1 (obsolete) — Splinter Review
This patch depends on the work done at bug 1134329. Once that bug lands we could review this one.
Status: NEW → ASSIGNED
Posted patch v2 (obsolete) — Splinter Review
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #14)
> Created attachment 8572144 [details] [diff] [review]
> v1
> 
> This patch depends on the work done at bug 1134329. Once that bug lands we
> could review this one.

New patch with tests. It doesn't depends on bug 1134329 anymore. Actually from no own bug 1134329 depends on this one.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=274bd69cfb82
Attachment #8572144 - Attachment is obsolete: true
Posted patch v3 (obsolete) — Splinter Review
Some tweaks and changes here and there.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57a3eec8834
Attachment #8574576 - Attachment is obsolete: true
Bug 1099370 reproduces in tests added in the patch in this bug (some mochitest-devtools tests e10s mode are not passing). My bet is another test in browser/devtools/framework/test/ is not destroying the toolbox correctly. I am following the tips at bug 1099370 so I hope to find it.
See Also: → 1099370
By disabling the inspector tests everything is green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0350a083654a
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #18)
> By disabling the inspector tests everything is green.

Oops, profiler tests I mean!
Posted patch v4 (obsolete) — Splinter Review
This patch adds the test fix I needed to add to the profiler devtool tests. Everything should be green now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12bfe580cb95

Nikhil, should we request review at someone else? I mean someone from the devtool team. I leave that to you. Thanks!
Attachment #8574913 - Attachment is obsolete: true
Attachment #8576502 - Flags: review?(nsm.nikhil)
Blocks: 1134329
Comment on attachment 8576502 [details] [diff] [review]
v4

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

Your patch has a lot of trailing whitespace, please remove that.
Then fix the preferences as pointed below. You'll definitely need to ask devtools peers for review on this patch. I can only review ServiceWorkerManager.cpp, which seems satisfactory, but I want to take another look once everything else is fixed.

::: modules/libpref/init/all.js
@@ +139,5 @@
>  pref("dom.serviceWorkers.enabled", false);
>  
> +// Service workers testing
> +pref("dom.serviceWorkers.testing.enabled", false);
> +

Please remove this.

::: testing/profiles/prefs_general.js
@@ +303,5 @@
>  user_pref("browser.tabs.remote.autostart.1", false);
>  // Don't forceably kill content processes after a timeout
>  user_pref("dom.ipc.tabs.shutdownTimeoutSecs", 0);
> +
> +user_pref("dom.serviceWorkers.enabled", false);

Please don't enable serviceworkers test wide like this. SpecialPowers allows per-test pref changes like https://dxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/test_fetch_event.html?from=test_fetch_event.html&case=true#52

Also, make sure when you run this test that dom.serviceWorkers.testing.enabled is set to false, so that we know it is actually your change and not the pref that makes it work.
Attachment #8576502 - Flags: review?(nsm.nikhil)
Posted patch v5 (obsolete) — Splinter Review
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #21)
> Comment on attachment 8576502 [details] [diff] [review]
> v4

Thanks for the review!

> Review of attachment 8576502 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your patch has a lot of trailing whitespace, please remove that.
> Then fix the preferences as pointed below. You'll definitely need to ask
> devtools peers for review on this patch. I can only review
> ServiceWorkerManager.cpp, which seems satisfactory, but I want to take
> another look once everything else is fixed.

New version of the patch with comments addressed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a720b864c6

Nikhil, I'll request review at you again once it gets reviewed by someone from the devtool team.

Mike, would you mind to review the devtool bits please? Thanks!
Seem everything is fine.
Attachment #8576502 - Attachment is obsolete: true
Attachment #8578033 - Flags: review?(mratcliffe)
Attachment #8578033 - Flags: review?(mratcliffe) → review+
Comment on attachment 8578033 [details] [diff] [review]
v5

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #22)
> Created attachment 8578033 [details] [diff] [review]
> v5

> Nikhil, I'll request review at you again once it gets reviewed by someone
> from the devtool team.
> 
> Mike, would you mind to review the devtool bits please? Thanks!

Thanks Mike!
Attachment #8578033 - Flags: review?(nsm.nikhil)
No longer blocks: 1134329
Comment on attachment 8578033 [details] [diff] [review]
v5

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

r=me with the following changes.

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing.html
@@ +42,5 @@
> +          if (error.name === "SecurityError") {
> +            var button = document.getElementById("button");
> +            log("SecurityError");
> +            button.click();
> +          }

If this fails with some other error, the test will timeout. Always call button.click(), even if it is not a security error.

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js
@@ +15,5 @@
> +function test() {
> +  SpecialPowers.pushPrefEnv({"set": [
> +    ["dom.serviceWorkers.exemptFromPerDomainMax", true],
> +    ["dom.serviceWorkers.enabled", true],
> +    ["dom.serviceWorkers.testing.enabled", false]

Please add a comment here that this is false since we are testing that the same capabilities are enabled with the devtools pref.

@@ +55,5 @@
> +    doTheCheck();
> +  }
> +
> +  button.addEventListener('click', function onClick() {
> +    button.addEventListener('click', onClick);

Here and everywhere else, this should be removeEventListener.

::: browser/devtools/framework/test/worker.js
@@ +1,1 @@
> +// empty worker, always succeed!

empty service worker.

Also rename this file to serviceworker.js
Attachment #8578033 - Flags: review?(nsm.nikhil) → review+
Posted patch v6 (obsolete) — Splinter Review
Review comments from comment 24 addressed.

Carrying out r=nsm,miker.

Try results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63443d914cd
Attachment #8578033 - Attachment is obsolete: true
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #24)
> Comment on attachment 8578033 [details] [diff] [review]
> v5
> 
> Review of attachment 8578033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the following changes.
> 
> :::
> browser/devtools/framework/test/
> browser_toolbox_options_enable_serviceworkers_testing.html
> @@ +42,5 @@
> > +          if (error.name === "SecurityError") {
> > +            var button = document.getElementById("button");
> > +            log("SecurityError");
> > +            button.click();
> > +          }
> 
> If this fails with some other error, the test will timeout. Always call
> button.click(), even if it is not a security error.

Just noticed I need to take out the 'var button = ...' statement as well from the `if` statement.
Keywords: checkin-needed
Posted patch v7Splinter Review
Review comments from comment 24 addressed (really).

Carrying out r=nsm,miker.

Try results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=444944db3552
Attachment #8582470 - Attachment is obsolete: true
Does this only need the v7 patch or also the patch from November?
Flags: needinfo?(josea.olivera)
(In reply to Wes Kocher (:KWierso) from comment #28)
> Does this only need the v7 patch or also the patch from November?

Only the v7 one. As you can see it's the one pushed to try at comment 27. Thanks!
Flags: needinfo?(josea.olivera)
https://hg.mozilla.org/mozilla-central/rev/1723cae4f6b2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.