Closed
Bug 1003991
Opened 10 years ago
Closed 9 years ago
Disable https:// only load for ServiceWorkers when Developer Tools are open
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: nsm, Assigned: jaoo)
References
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
nsm
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
Blocks: ServiceWorkers
Updated•10 years ago
|
Assignee: nobody → jefry.reyes
Comment 1•10 years ago
|
||
Attachment #8510329 -
Flags: review?(nsm.nikhil)
Reporter | ||
Comment 2•10 years ago
|
||
Jefry, it seems the attachment doesn't have all the files.
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?
Attachment #8510329 -
Flags: review?(nsm.nikhil)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(nfitzgerald)
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).
Flags: needinfo?(nfitzgerald)
Comment 5•10 years ago
|
||
[0] https://wiki.mozilla.org/Remote_Debugging_Protocol
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?
Flags: needinfo?(nsm.nikhil)
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?
Flags: needinfo?(nsm.nikhil) → needinfo?(nfitzgerald)
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
Flags: needinfo?(nfitzgerald)
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.
Attachment #8510329 -
Attachment is obsolete: true
Attachment #8529918 -
Flags: feedback?(nsm.nikhil)
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.
Attachment #8529918 -
Flags: feedback?(nsm.nikhil) → feedback+
Comment 11•10 years ago
|
||
I'm releasing this ticket so someone else can work on it.
Assignee: jefry.reyes → nobody
Assignee | ||
Comment 12•10 years ago
|
||
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
Assignee | ||
Comment 13•10 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.
Assignee | ||
Comment 14•10 years ago
|
||
This patch depends on the work done at bug 1134329. Once that bug lands we could review this one.
Updated•9 years ago
|
Status: NEW → ASSIGNED
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
Attachment #8572144 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
Some tweaks and changes here and there. https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57a3eec8834
Attachment #8574576 -
Attachment is obsolete: true
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.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 18•9 years ago
|
||
By disabling the inspector tests everything is green. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0350a083654a
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!
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!
Attachment #8574913 -
Attachment is obsolete: true
Attachment #8576502 -
Flags: review?(nsm.nikhil)
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.
Attachment #8576502 -
Flags: review?(nsm.nikhil)
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.
Attachment #8576502 -
Attachment is obsolete: true
Attachment #8578033 -
Flags: review?(mratcliffe)
Attachment #8578033 -
Flags: review?(mratcliffe) → review+
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!
Attachment #8578033 -
Flags: review?(nsm.nikhil)
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
Attachment #8578033 -
Flags: review?(nsm.nikhil) → review+
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
Attachment #8578033 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
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.
Keywords: checkin-needed
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
Attachment #8582470 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Does this only need the v7 patch or also the patch from November?
Flags: needinfo?(josea.olivera)
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!
Flags: needinfo?(josea.olivera)
Comment 30•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1723cae4f6b2
Flags: in-testsuite+
Keywords: checkin-needed
Comment 31•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1723cae4f6b2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Blocks: 1150946
You need to log in
before you can comment on or make changes to this bug.
Description
•