Closed
Bug 1345932
Opened 7 years ago
Closed 7 years ago
disable service worker debugging in about:debugging when multi-e10s is enabled
Categories
(DevTools :: about:debugging, defect)
DevTools
about:debugging
Tracking
(firefox54 fixed, firefox55 fixed)
RESOLVED
FIXED
Firefox 55
People
(Reporter: bkelly, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
jdescottes
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jdescottes
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jdescottes
:
review+
|
Details |
(deleted),
patch
|
jdescottes
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
In the long term (bug 1231208) we will be fixing service workers to only spawn a SW in a single process at a time. In the short term, however, this is a desire to ship multi-e10s before the SW refactor is complete. To that end we would like to simply disable service worker debugging when multi-e10s is enabled in the browser. Ideally about:debugging would have text saying something like: "Service worker debugging is not supported in this mode. Please disable multiple processes in order to debug service workers. <Button to do that and restart>" I think we should completely hide the start/debug buttons in about:debugging in this case.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
I have attached a first WIP patch here, but I would like to check my current approach makes sense. Ben feel free to forward the ni? to someone else, not sure who to contact here. 1. First, I am using the pref "dom.ipc.processCount" to check whether or not we can have multiple content process. Something I am not sure about is what happens when this preference is updated after starting the browser. Are new processes allocated/removed dynamically ? Or is this only taking effect when restarting the browser? Maybe be there is a better way to feature detect here? 2. Then I could use a general feedback on the current solution: If dom.ipc.processCount is set to 1 and is the default value, no message is displayed Otherwise, a warning message is displayed with a checkbox that will "force single process" (ie set the pref to 1). I also keep track of the initial value of the preference when about:debugging started. If it is different from the current value, "browser restart required" is displayed next to the checkbox label. Finally the start and debug buttons are disabled if the current value or the initial value of `dom.ipc.processCount` is different from 1. Does this make sense?
Flags: needinfo?(bkelly)
Reporter | ||
Updated•7 years ago
|
Blocks: ServiceWorkers-bandaids
Reporter | ||
Comment 5•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #4) > 1. First, I am using the pref "dom.ipc.processCount" to check whether or not > we can have multiple content process. > Something I am not sure about is what happens when this preference is > updated after starting the browser. Are new processes allocated/removed > dynamically ? Or is this only taking effect when restarting the browser? > Maybe be there is a better way to feature detect here? The "dom.ipc.processCount" is the maximum allowed number of content processes. Its the right thing to use here. > 2. Then I could use a general feedback on the current solution: > > If dom.ipc.processCount is set to 1 and is the default value, no message is > displayed > Otherwise, a warning message is displayed with a checkbox that will "force > single process" (ie set the pref to 1). > > I also keep track of the initial value of the preference when > about:debugging started. If it is different from the current value, "browser > restart required" is displayed next to the checkbox label. > > Finally the start and debug buttons are disabled if the current value or the > initial value of `dom.ipc.processCount` is different from 1. > > Does this make sense? This sounds good to me. I suggest we land it and see how it works in practice. Thanks for working this!
Flags: needinfo?(bkelly)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
Alex, I attached a first patch here which implements the approach mentioned in the previous comments. I'm not 100% convinced we should go for a UI with a checkbox, rather than a simple explanatory text. So I have another changeset ready if you prefer to go with something simpler.
Assignee | ||
Comment 9•7 years ago
|
||
This is a screenshot of the other approach I mentioned.
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8845945 [details] Bug 1345932 - cleanup service worker panel render method; https://reviewboard.mozilla.org/r/119036/#review122592 ::: devtools/client/aboutdebugging/components/workers/panel.js:145 (Diff revision 2) > let isWindowPrivate = PrivateBrowsingUtils.isContentWindowPrivate(window); > let isPrivateBrowsingMode = PrivateBrowsingUtils.permanentPrivateBrowsing; > let isServiceWorkerDisabled = !Services.prefs > .getBoolPref("dom.serviceWorkers.enabled"); > - let errorMsg = isWindowPrivate || isPrivateBrowsingMode || > - isServiceWorkerDisabled ? > + > + let hasError = isWindowPrivate || isPrivateBrowsingMode || isServiceWorkerDisabled; `hasError` isn't perfect, it looks more like "is disabled".
Attachment #8845945 -
Flags: review?(poirot.alex) → review+
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8845946 [details] Bug 1345932 - show warning info in about:debugging#workers if multi-e10s is on; https://reviewboard.mozilla.org/r/119038/#review122594 This checkbox with state saving looks too complex. What Ben suggests in first comments looks more than enough. You can request firefox to restart gracefuly with: Services.startup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); I think it is still better than just a text message. ::: devtools/client/aboutdebugging/aboutdebugging.css:187 (Diff revision 2) > } > > +.addons-install-error .warning, > +.service-worker-multi-process-warning .warning { > + filter: brightness(0%); > +} What is that filter brighness 0%?
Attachment #8845946 -
Flags: review?(poirot.alex)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8845946 [details] Bug 1345932 - show warning info in about:debugging#workers if multi-e10s is on; https://reviewboard.mozilla.org/r/119038/#review122594 As discussed, I agree the checkbox thing was way too complex. Since this is only temporary I guess we don't need to over-complicate things. > What is that filter brighness 0%? The warning icon is orange, so when it is displayed on top of a red/yellow background, it is hard to see. I just used it to make the icon completely black. There might be a better way though!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
try is green-ish https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f0bab681bae11e4e7a87479379ed6195ffc37a2
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8845946 [details] Bug 1345932 - show warning info in about:debugging#workers if multi-e10s is on; https://reviewboard.mozilla.org/r/119038/#review122934 I think we can make the code and behavior simplier. But it looks good overall. ::: devtools/client/aboutdebugging/aboutdebugging.css:191 (Diff revision 4) > } > } > > +.addons-install-error .warning, > +.service-worker-multi-process .warning { > + filter: brightness(0%); Please comment about this, this is all but obvious. ::: devtools/client/aboutdebugging/components/workers/multi-e10s-warning.js:53 (Diff revision 4) > + let { value, initialValue, hasUserValue } = multiE10S; > + > + // MultiE10S is considered as disabled if 1 is the default value, and if it was not > + // enabled when about:debugging was opened. > + let multiE10SDisabled = value === 1 && initialValue === 1 && !hasUserValue; > + if (!isE10S || multiE10SDisabled) { I would only display this warning if processCount > 1. Making a difference between user value or default value doesn't sound that important. It makes code complex and displays the warning even if the pref is set to one. `props` would end up being just `{ isE10S, processCount }`. It sounds easier to keep the pref name rather than coming up with another name. The check here would just be: ``` if (!isE10S || processCount == 1) { return ""; } ``` If you are afraid of people forgetting about running in one process, you may add a comment about that and suggest they may set process count back in about:config later. But I'm not sure it is really important, the existing comment looks good to me. ::: devtools/client/aboutdebugging/components/workers/panel.js:198 (Diff revision 4) > let { client, id } = this.props; > - let { workers } = this.state; > + let { workers, multiE10S } = this.state; > + let isE10S = this.isE10S; > + > + let multiE10SEnabled = multiE10S.value !== 1 || multiE10S.initialValue !== 1; > + let swDebugDisabled = isE10S && multiE10SEnabled; Same here, I don't see much value in initialValue usage. I would just show/hide based on the current value and simplify `multiE10S`. Have just processCount in state being the pref value. ::: devtools/client/aboutdebugging/components/workers/panel.js:213 (Diff revision 4) > id: id + "-header", > name: Strings.GetStringFromName("workers") > }), > + MultiE10SWarning({ > + multiE10S, > + isE10S: isE10S, nit: e10S: e10S -> e10S ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:193 (Diff revision 4) > } > > return dom.a({ > onClick: this.unregister, > - className: "unregister-link" > + className: "unregister-link", > }, Strings.GetStringFromName("unregister")); Should we also disable unregister? (I have no idea how well it works with multi-e10s)
Attachment #8845946 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8845946 [details] Bug 1345932 - show warning info in about:debugging#workers if multi-e10s is on; https://reviewboard.mozilla.org/r/119038/#review122934 Thanks for the review, sorry for doing a complicated mess :/ > Please comment about this, this is all but obvious. Done > I would only display this warning if processCount > 1. > Making a difference between user value or default value doesn't sound that important. > It makes code complex and displays the warning even if the pref is set to one. > `props` would end up being just `{ isE10S, processCount }`. It sounds easier to keep the pref name rather than coming up with another name. > The check here would just be: > ``` > if (!isE10S || processCount == 1) { > return ""; > } > ``` > > If you are afraid of people forgetting about running in one process, you may add a comment about that and suggest they may set process count back in about:config later. But I'm not sure it is really important, the existing comment looks good to me. Let's go with the simple solution for now and only rely on processCount === 1 > Should we also disable unregister? > (I have no idea how well it works with multi-e10s) I think it works but we'll check with bkelly and file a follow up if needed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
Ben, do you know if "unregister" should also be disabled in multi e10s ?
Flags: needinfo?(bkelly)
Reporter | ||
Comment 23•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #22) > Ben, do you know if "unregister" should also be disabled in multi e10s ? Unregister should work as well as it does in single content process e10s I think.
Flags: needinfo?(bkelly)
Comment 24•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a2501c4d00c7 cleanup service worker panel render method;r=ochameau https://hg.mozilla.org/integration/autoland/rev/b94a7f7b3980 show warning info in about:debugging#workers if multi-e10s is on;r=ochameau
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8848064 [details] Bug 1345932 - disable aboutdebugging multi e10s test if e10s disabled; https://reviewboard.mozilla.org/r/121036/#review122960
Attachment #8848064 -
Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 56287fac0050 -d b94a7f7b3980: rebasing 382368:56287fac0050 "Bug 1345932 - cleanup service worker panel render method;r=ochameau" merging devtools/client/aboutdebugging/components/workers/panel.js warning: conflicts while merging devtools/client/aboutdebugging/components/workers/panel.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Assignee | ||
Updated•7 years ago
|
Attachment #8845945 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8845946 -
Attachment is obsolete: true
Comment 30•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/231a2fc0fe33 disable aboutdebugging multi e10s test if e10s disabled;r=jdescottes
Comment 31•7 years ago
|
||
Backed out for failing browser_misused_characters_in_strings.js for aboutdebugging.properties' multiProcessWarningMessage: https://hg.mozilla.org/integration/autoland/rev/b267bced06cc3ab82e5b4cfd513d267e3279dc23 https://hg.mozilla.org/integration/autoland/rev/bf4f8080c6ce190e2945b15126d710f11f8b3f97 https://hg.mozilla.org/integration/autoland/rev/7116a3baad2ba51989f83a782acaf80bcf1d6b2c Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b94a7f7b3980dcc0dfcfbf03d37845e0925d16f4&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=84303941&repo=autoland > TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_misused_characters_in_strings.js | jar:file:///builds/slave/test/build/application/Nightly.app/Contents/Resources/browser/omni.ja!/chrome/en-US/locale/en-US/devtools/client/aboutdebugging.properties with key=multiProcessWarningMessage has a misused double-quote. Double-quoted strings should use Unicode “foo” instead of "foo". -
Flags: needinfo?(jdescottes)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•7 years ago
|
||
Comment on attachment 8848110 [details] Bug 1345932 - cleanup service worker panel render method; forwarding r+
Flags: needinfo?(jdescottes)
Attachment #8848110 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 36•7 years ago
|
||
Comment on attachment 8848111 [details] Bug 1345932 - show warning info in about:debugging#workers if multi-e10s is on; forwarding r+
Attachment #8848111 -
Flags: review?(poirot.alex) → review+
Comment 37•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3ce010352d cleanup service worker panel render method;r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/67599d8864b8 show warning info in about:debugging#workers if multi-e10s is on;r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/242462674581 disable aboutdebugging multi e10s test if e10s disabled;r=jdescottes
Assignee | ||
Comment 38•7 years ago
|
||
fixed the browser_misused_characters_in_strings.js issue and pushed to inbound.
Comment 39•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb3ce010352d https://hg.mozilla.org/mozilla-central/rev/67599d8864b8 https://hg.mozilla.org/mozilla-central/rev/242462674581
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•7 years ago
|
Comment 40•7 years ago
|
||
RyanVM wants to uplift bug 1348547 to aurora, but that patch relies on this bug (I added to enableServiceWorkerDebugging()). Can these patches be uplifted to aurora?
Flags: needinfo?(jdescottes)
Reporter | ||
Comment 41•7 years ago
|
||
This bug really needs to be uplifted to FF54.
Assignee | ||
Comment 42•7 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: blocker for multiple content process. If users have > 1 content process, service worker debugging can not work. This bug adds a notification message in about:debugging to let users know about that. [Is this code covered by automated tests?]: yes, included in this changeset [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: (all the changesets for this bug have been rebased on aurora and folded here) [Is the change risky?]: no [Why is the change risky/not risky?]: This is only a UI change for about:debugging. Users that don't visit about debugging will not be impacted. [String changes made/needed]: 4 strings have been added in devtools/client/locales/en-US/aboutdebugging.properties : multiProcessWarningTitle = Service Worker debugging is not compatible with multiple content processes at the moment. multiProcessWarningMessage = The preference “dom.ipc.processCount” can be set to 1 to force a single content process. multiProcessWarningUpdateLink = Set dom.ipc.processCount to 1 multiProcessWarningConfirmUpdate = Set “dom.ipc.processCount” to 1 and restart the browser?
Flags: needinfo?(jdescottes)
Attachment #8856435 -
Flags: review+
Attachment #8856435 -
Flags: approval-mozilla-aurora?
Updated•7 years ago
|
status-firefox54:
--- → affected
Comment 43•7 years ago
|
||
I don't think we can change the string in Aurora right now. NI :flod for confirmation.
Flags: needinfo?(francesco.lodolo)
Comment 44•7 years ago
|
||
No, we really can't. There's already a mess going on with Dawn, I really don't want to pile on top of it. Also, how come this wasn't caught before? Patch has been in m-c for almost a month. If you really need this in aurora, please create a patch with those strings hard-coded and not exposed for localization.
Flags: needinfo?(francesco.lodolo)
Assignee | ||
Comment 45•7 years ago
|
||
> Also, how come this wasn't caught before? Patch has been in m-c for almost a month. I was simply unaware this had to be uplifted. > If you really need this in aurora, please create a patch with those strings hard-coded and not exposed for localization. Will do.
Assignee | ||
Comment 46•7 years ago
|
||
New version of the patch with hardcoded english strings. Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: blocker for multiple content process. If users have > 1 content process, service worker debugging can not work. This bug adds a notification message in about:debugging to let users know about that. [Is this code covered by automated tests?]: yes, included in this changeset [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: (all the changesets for this bug have been rebased on aurora and folded here) [Is the change risky?]: no [Why is the change risky/not risky?]: This is only a UI change for about:debugging. Users that don't visit about debugging will not be impacted. [String changes made/needed]: No
Attachment #8856435 -
Attachment is obsolete: true
Attachment #8856435 -
Flags: approval-mozilla-aurora?
Attachment #8856868 -
Flags: review+
Attachment #8856868 -
Flags: approval-mozilla-aurora?
Comment 47•7 years ago
|
||
Comment on attachment 8856868 [details] [diff] [review] bug1345932.aurora.patch Fix an issue that service worker debugging won't work if there are more than one content process. Aurora54+.
Attachment #8856868 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 48•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/87dc33c4e226
Flags: in-testsuite+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•