1522511 - [remote-dbg-next] Do not prevent service-worker debugging if new sw implementation is enabled

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

The old warning about multi-e10s should only be displayed if dom.serviceWorkers.parent_intercept is set to false.

In this bug we should:

• modify devtools' multi-e10s helper to check dom.serviceWorkers.parent_intercept
• skip tests related to this behavior when dom.serviceWorkers.parent_intercept is true
• stop forcing await pushPref("dom.ipc.processCount", 1); for service-workers tests when dom.serviceWorkers.parent_intercept is true (this way our mochitests for service workers will start running in multi e10s)

Comment 1

[Julian Descottes [:jdescottes]]

Check

Comment 2

[Julian Descottes [:jdescottes]]

@sole, here are some pointers to work on this bug.

1 - Identify the module to update.

The helper multi-e10s-helper.js should be updated. This helper is only used to check if a service worker debugging can be used or not, so our goal here will be to make it check both the multi-e10s status and the sw-refactor status in order to decide if sw-debugging is possible on the current runtime.

2 - Renaming

(This is optional and can be handled in a follow up)
It will probably be easier to reason about this if we start by renaming all the occurrences of isMultiE10s by isServiceWorkerDebuggingDisabled under /devtools (https://searchfox.org/mozilla-central/search?q=isMultiE10s&path=). Also optional, we could rename multi-e10s-helper.js to service-worker-debugging-helper.js.

3 - Implementation

The method isMultiE10s (or isServiceWorkerDebuggingDisabled if you renamed it) should also call ServiceWorkerManager's isParentInterceptEnabled(). You can see how this API is used in service-worker-registration.js for instance. If isParentInterceptEnabled() is true, then we are using the refactor and the method from our helper should return false. If isParentInterceptEnabled() is false, then we should still use the current implementation (ie check browserTabsRemoteAutostart and maxWebProcessCount).

4 - Test updates

Two tests will start failing with this change, when they run with the refactor enabled:

• devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_serviceworker_multie10s.js
• devtools/client/aboutdebugging/test/browser_service_workers_multi_content_process.js
  You can add skip-if = serviceworker_e10s for their respective entries in the browser.ini file located in the same folder as the test.

Comment 3

[Soledad Penades [:sole] [:spenades]]

Thanks, Julian---I'm going to try and do this one O:)

Comment 4

[Soledad Penades [:sole] [:spenades]]

Good news! I made progress on this: I modified the helper to take into account if the SW refactor is turned on, and now I can debug service workers if I turn the pref on (and restart), regardless of the number of content processes.

However, I'm a bit unsure about a couple of things:

1. disabling the tests

You suggest we disable them using skip-if = serviceworker_e10s. But the tests are specifically written to test the behaviour when e10s is on, changing between single or multiple processes... so I wonder if we should disable them if the new SW implementation is on instead. Something like skip-if = sw_intercept ??

2. Renaming to isServiceWorkerDebuggingDisabled - I like the idea of not leaking implementation details anymore (i.e. mentioning e10s everywhere), and just caring about whether service workers can or not be debugged. However I wonder if the name should be a true value and not a negated one, as it makes boolean logic easier to reason about: canDebugServiceWorkers

The usages of the old function would need to be altered obviously, and it won't be an immediate replacement, but I think it's easier to read something like:

if(canDebugServiceWorkers) rather than if(!serviceWorkerDebuggingDisabled)

I also don't know if we prefer is* rather than can* in the function naming (maybe there's no explicit style guide).

This would be a little more tedious to implement so I propose it should be a follow up, together with the renaming so it's more atomic, and the code would look much nicer.

Thoughts? :)

Comment 5

[Julian Descottes [:jdescottes]]

Thanks Sole, some answers below:

You suggest we disable them using skip-if = serviceworker_e10s. But the tests are specifically written to test the behaviour when e10s is on, changing between single or multiple processes... so I wonder if we should disable them if the new SW implementation is on instead. Something like skip-if = sw_intercept ??

This is exactly what this skip if is doing. It disables the test when running with the refactor.

However I wonder if the name should be a true value and not a negated one, as it makes boolean logic easier to reason about: canDebugServiceWorkers

I would prefer "canDebugServiceWorkers", but I didn't want to force you to "reverse" the logic the logic everywhere, as it means more work :) If you feel like doing it, go for it!

I also don't know if we prefer is* rather than can* in the function naming (maybe there's no explicit style guide).

I think can* is good here. Otherwise we have to use isServiceWorkerDebuggerEnabled, and that's not even really accurate. The question is not whether some option is toggled to debug service workers, but rather "is it possible at all in this environment". So can* would be a better name.

This would be a little more tedious to implement so I propose it should be a follow up, together with the renaming so it's more atomic, and the code would look much nicer.

I initially assumed this would be tackled a follow up, but again if you feel like doing everything here, it's definitely better ;)

Comment 6

[Julian Descottes [:jdescottes]]

One final note about the skip-if rules, you cannot really test them locally most of the time. Only way to check if the skip if works as expected is probably to push to try.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Julian Descottes [:jdescottes] from comment #6)

One final note about the skip-if rules, you cannot really test them locally most of the time. Only way to check if the skip if works as expected is probably to push to try.

mozinfo should report "serviceworker_e10s" if the "dom.serviceWorkers.parent_intercept" preference is set to true, which you would do via "--setpref". Here's where the logic happens in:

• mochitests: https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/testing/mochitest/runtests.py#2545
• xpcshell tests: https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/testing/xpcshell/runxpcshelltests.py#1202

That said, running on try and including the "-sw" substring builds via the fuzzy builds is the easiest way to make sure you've got builds with that preference set. (And that way you also get normal child-intercept tests to run too, they're the ones withOUT "-sw" in them.)

Comment 8

[Soledad Penades [:sole] [:spenades]]

Thanks for the info, Julian and Andrew. The name of the flag confused me, I later saw another flag for just "e10s". I see now that they're two different things.

Julian - when adding the skip-if to the second test, I noticed it has a skip-if mac for a bug that has been closed already; should I use this opportunity to also remove that skip if?
https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser.ini#43-44

It's bug 1333759

Comment 9

[Julian Descottes [:jdescottes]]

The skip-ifs apply the other way around:

[test_1.js]
skip-if = something # <-- this applies to test_1.js, not test_2.js
[test_2.js]

Might seem counter intuitive, but a good tip to remember this is that browser.ini is an INI file. Therefore, [test_1.js] opens a section, and everything until the next section applies to [test_1.js]. So here skip-if = os == 'mac' # bug 1333759 applies to [browser_service_workers_fetch_flag.js].

Regardless of that, should we remove it because the bug is fixed? I would say no here, because the bug referenced is an intermittent which was "fixed" in two steps: 1/ adding this skip-if, 2/ doing a fix when failures came back on linux64. There is no way to know if fix done for linux64 also silenced the intermittent on MacOS. We could open another bug to attempt to remove the skip-if, but old about:debugging tests should go away soon so I don't think it's worth doing.

Comment 10

[Soledad Penades [:sole] [:spenades]]

Attachment: Bug 1522511 - [remote-dbg-next] Do not prevent service-worker debugging if new sw implementation is enabled

Take into account new service worker implementation to decide if workers can be debugged

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4db188219e47
[remote-dbg-next] Do not prevent service-worker debugging if new sw implementation is enabled r=ladybenko

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4db188219e47