Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Description • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 2 • 10 years ago

Jefry, it seems the attachment doesn't have all the files.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 3 • 10 years ago

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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 4 • 10 years ago

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).

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 5 • 10 years ago

[0] https://wiki.mozilla.org/Remote_Debugging_Protocol

	Jefry Lagrange
	Comment 6 • 10 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?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 7 • 10 years ago

(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?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 8 • 10 years ago

(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

	Jefry Lagrange
	Comment 9 • 10 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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 10 • 10 years ago

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.

	Jefry Lagrange
	Comment 11 • 9 years ago

I'm releasing this ticket so someone else can work on it.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 12 • 9 years ago

This bug is quite similar to bug 1137245. Once it gets landed a fix for this one should be fairly easy.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 13 • 9 years ago

(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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 14 • 9 years ago

This patch depends on the work done at bug 1134329. Once that bug lands we could review this one.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 15 • 9 years ago

(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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 16 • 9 years ago

Some tweaks and changes here and there.

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 17 • 9 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 18 • 9 years ago

By disabling the inspector tests everything is green.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0350a083654a

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 19 • 9 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #18)
> By disabling the inspector tests everything is green.

Oops, profiler tests I mean!

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 20 • 9 years ago

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!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 21 • 9 years ago

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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 22 • 9 years ago

(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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 23 • 9 years ago

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!

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 24 • 9 years ago

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

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 25 • 9 years ago

Review comments from comment 24 addressed.

Carrying out r=nsm,miker.

Try results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63443d914cd

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 26 • 9 years ago

(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.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 27 • 9 years ago

Review comments from comment 24 addressed (really).

Carrying out r=nsm,miker.

Try results at https://treeherder.mozilla.org/#/jobs?repo=try&revision=444944db3552

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 28 • 9 years ago

Does this only need the v7 patch or also the patch from November?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 29 • 9 years ago

(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!

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1723cae4f6b2

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1723cae4f6b2