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)

Product:
DevTools
Component:
about:debugging
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: bkelly, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

service-worker-disabled-text_only.png
189.53 KB, image/png
Details
Bug 1345932 - disable aboutdebugging multi e10s test if e10s disabled;
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1345932 - cleanup service worker panel render method;
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1345932 - show warning info in about:debugging#workers if multi-e10s is on;
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
bug1345932.aurora.patch
32.24 KB, 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: nobody → jdescottes
Status: NEW → ASSIGNED
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)
(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)
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.
This is a screenshot of the other approach I mentioned.
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 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 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 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 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.
Ben, do you know if "unregister" should also be disabled in multi e10s ?
Flags: needinfo?(bkelly)
(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)
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 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+
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)
Attachment #8845945 - Attachment is obsolete: true
Attachment #8845946 - Attachment is obsolete: true
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/231a2fc0fe33
disable aboutdebugging multi e10s test if e10s disabled;r=jdescottes
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 on attachment 8848110 [details]
Bug 1345932 - cleanup service worker panel render method;

forwarding r+
Flags: needinfo?(jdescottes)
Attachment #8848110 - Flags: review?(poirot.alex) → review+
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+
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
fixed the browser_misused_characters_in_strings.js issue and pushed to inbound.
See Also: → 1348547
Blocks: 1348547
See Also: 1348547 → 
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)
This bug really needs to be uplifted to FF54.
Attached patch bug1345932.aurora.patch (obsolete) — — Splinter Review 
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?
I don't think we can change the string in Aurora right now. NI :flod for confirmation.
Flags: needinfo?(francesco.lodolo)
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)
> 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.
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 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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: