Probably this can be done using the active/waiting cache name.

I guess it should block v2. Please, change it if it is not the case. Thanks!

We should probably move this to a devtools bug.

Julian, is this something you would be interested in taking on?

Yes please! Based on the discussion in Bug 1250905, on top of displaying the status of the service worker, the "Debug" button should probably be enabled/disabled depending on the worker's status.

(In reply to Julian Descottes [:jdescottes] from comment #4) > Yes please! Awesome! > Based on the discussion in Bug 1250905, on top of displaying the status of > the service worker, the "Debug" button should probably be enabled/disabled > depending on the worker's status. Agreed.

Andrea: activeCacheName/waitingCacheName are no longer available (cf your first comment in this bug), so I tried something relying on the ServiceWorker's state. Is this fine? If the approach is fine for you, I could use feedback on the patch itself, since I have never tried modifying C++/idl files before.

Jan: Sorry for the long wait... finally took the time to propose something. The current patch is displaying a single worker state. I know we discussed the idea of separating running/stopped and waiting/installing/activated, but is the added value is worth the added UI? Let's discuss. Also could you try the patch and let me know what you think about the current layout : - position of the state name on the left (we also discussed putting it on a new line below the url?) - colors (do we want colors, if yes which colors) - ... anything else (FYI : patch is not complete, still need to add tests and to listen to worker state changes, for now, we refresh only when the list of workers is updated)

Comment on attachment 8734024 [details] [diff] [review] bug1153292.part1.wip.patch (add worker state to ServiceWorkerInfo) Review of attachment 8734024 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +5123,5 @@ > } > > NS_IMETHODIMP > +ServiceWorkerInfo::GetState(uint16_t* aState) > +{ MOZ_ASSERT(aState);

Comment on attachment 8734025 [details] [diff] [review] bug1153292.part2.wip.patch (add worker state info to ServiceWorkerRegistrationInfo) Review of attachment 8734025 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot for working on this feature! Your patches look good to me, but as usual I have a few nits. ::: devtools/server/actors/worker.js @@ +327,5 @@ > + url: this._registration.scriptSpec, > + installingWorker: this._getWorkerForm( > + this._registration.installingWorker), > + activeWorker: this._getWorkerForm(this._registration.activeWorker), > + waitingWorker: this._getWorkerForm(this._registration.waitingWorker), Nit: Generally for elements like these, devtools actors will implement a dedicated getter (i.e. `get activeWorkerForm()`) instead of a transform method for delegate components (i.e. `_getWorkerForm()` for `_registration.{installing,active,waiting}Worker`). Please do the same here, and move the 3 getters above the `form` method. @@ +328,5 @@ > + installingWorker: this._getWorkerForm( > + this._registration.installingWorker), > + activeWorker: this._getWorkerForm(this._registration.activeWorker), > + waitingWorker: this._getWorkerForm(this._registration.waitingWorker), > + waiting: this._registration.waitingWorker != null, Nit: Is `form.waiting` used anywhere? @@ +345,5 @@ > + activating: worker.state == worker.STATE_ACTIVATING, > + installed: worker.state == worker.STATE_INSTALLED, > + installing: worker.state == worker.STATE_INSTALLING, > + redundant: worker.state == worker.STATE_REDUNDANT, > + } Nit: It seems weird to re-implement `worker.state` as a group of mutually exclusive booleans. - Maybe we could simply pass along `ServiceWorkerInfo.state`, like we do with `WorkerDebugger.type`, and make the client code use the same enum? - Or if you feel we can't assume that this state enum will always remain consistent between actor and client (e.g. when we'll remote debug workers from runtimes with different versions), maybe we can translate it to a string here directly?

Comment on attachment 8734033 [details] [diff] [review] bug1153292.part3.wip.patch (ui) Review of attachment 8734033 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me in general! Thanks for implementing these labels. We're adding more service-worker-specific code to worker-target.js, and at some point we'll need to split it up further (and probably use a mixin to share the openWorkerToolbox implementation), but let's leave that for a follow-up. Also, this patch may change a little if we decide that the actor should send worker states differently (see other patch). ::: devtools/client/aboutdebugging/components/workers-tab.js @@ +100,5 @@ > + registrationActor: form.actor, > + installingWorker: form.installingWorker, > + waitingWorker: form.waitingWorker, > + activeWorker: form.activeWorker, > + id: form.id, Nit: Why add `form.id` to this target description?

Helen, we want to show service worker status in about:debugging, and we'd love your input on our approach. The screenshot (sorry for the bad quality) shows two different service workers states (Julian added the labels). Here are all the states a service worker can be in (with suggested CSS colors): - Stopped (grey border, lightgrey background): Correctly installed, but not currently running - Running (limegreen, palegreen): Installed and currently running - Installing (orange, moccasin): Currently installing (shouldn't last long, but may get stuck here in some cases) - Activating (gold, lightyellow): Installed, currently activating (same as above) - Error (red, peachpuff): There was a problem during install What do you think about these status labels that combine install and running state? Do you think we could improve the UI? (Maybe use a colored circle instead of a label in some cases?)

Thanks for the feedback Andrea & Jan! (In reply to Jan Keromnes [:janx] from comment #11) > Comment on attachment 8734025 [details] [diff] [review] > bug1153292.part2.wip.patch (add worker state info to > ServiceWorkerRegistrationInfo) > > Nit: Generally for elements like these, devtools actors will implement a > dedicated getter (i.e. `get activeWorkerForm()`) instead of a transform > method for delegate components (i.e. `_getWorkerForm()` for > `_registration.{installing,active,waiting}Worker`). Please do the same here, > and move the 3 getters above the `form` method. > Not sure I understood 100% what should be done here, I will upload a new version shortly, let me know if this is what you had in mind ? I could not find similar examples in our existing actors though. Would you mind explaining what this guideline is for (I'm fine with "for consistency" but then I'd like to have an example from our codebase :) ) > @@ +328,5 @@ > > + installingWorker: this._getWorkerForm( > > + this._registration.installingWorker), > > + activeWorker: this._getWorkerForm(this._registration.activeWorker), > > + waitingWorker: this._getWorkerForm(this._registration.waitingWorker), > > + waiting: this._registration.waitingWorker != null, > > Nit: Is `form.waiting` used anywhere? Leftover, thanks! > > @@ +345,5 @@ > > + activating: worker.state == worker.STATE_ACTIVATING, > > + installed: worker.state == worker.STATE_INSTALLED, > > + installing: worker.state == worker.STATE_INSTALLING, > > + redundant: worker.state == worker.STATE_REDUNDANT, > > + } > > Nit: It seems weird to re-implement `worker.state` as a group of mutually > exclusive booleans. > > - Maybe we could simply pass along `ServiceWorkerInfo.state`, like we do > with `WorkerDebugger.type`, and make the client code use the same enum? > > - Or if you feel we can't assume that this state enum will always remain > consistent between actor and client (e.g. when we'll remote debug workers > from runtimes with different versions), maybe we can translate it to a > string here directly? Good point about the remote debugging, and I also want to avoid duplicating the enum for maintenance reasons. I am OK with generating a string here. > Nit: Why add `form.id` to this target description? Leftover again, sorry :/

Jan: some questions for you in Comment 14, thanks!

(In reply to Julian Descottes [:jdescottes] from comment #14) > (In reply to Jan Keromnes [:janx] from comment #11) > > Nit: Generally for elements like these, devtools actors will implement a > > dedicated getter (i.e. `get activeWorkerForm()`) instead of a transform > > method for delegate components (i.e. `_getWorkerForm()` for > > `_registration.{installing,active,waiting}Worker`). Please do the same here, > > and move the 3 getters above the `form` method. > > > > Not sure I understood 100% what should be done here, I will upload a new > version shortly, let me know if this is what you had in mind ? I could not > find similar examples in our existing actors though. Would you mind > explaining what this guideline is for (I'm fine with "for consistency" but > then I'd like to have an example from our codebase :) ) Fair enough, sorry for being a little vague about this. You addressed most of my comment, but I was referring to actual JS getters like in [0] or [1]. [0] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js#479 [1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/common.js#331 This would give something like: get activeWorkerForm() { return this._getWorkerForm(this._registration.activeWorker); }, form: function(detail) { return { activeWorker: this.activeWorkerForm, }; }, This doesn't apply to _getWorkerForm(workerInfo) however. Speaking of which, this method doesn't quite feel right in ServiceWorkerRegistrationActor (ok this is getting very vague), but maybe at some point it will become a proper form() getter within a dedicated ServiceWorkerActor class. > > Nit: It seems weird to re-implement `worker.state` as a group of mutually > > exclusive booleans. > > > > - Maybe we could simply pass along `ServiceWorkerInfo.state`, like we do > > with `WorkerDebugger.type`, and make the client code use the same enum? > > > > - Or if you feel we can't assume that this state enum will always remain > > consistent between actor and client (e.g. when we'll remote debug workers > > from runtimes with different versions), maybe we can translate it to a > > string here directly? > > Good point about the remote debugging, and I also want to avoid duplicating > the enum for maintenance reasons. I am OK with generating a string here. If you prefer to generate an intermediate string, that's fine by me, but I still believe that just passing `ServiceWorkerInfo.state` along also works fine. To illustrate, here is where the worker actor sends `WorkerDebugger.type` as-is[2], and here where the client code figures it out[3]. As you can see, no intermediate string was generated, and there is no duplicated enum to maintain. [2] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/worker.js#63 [3] https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/workers-tab.js#111

Thanks for having a look! (In reply to Jan Keromnes [:janx] from comment #19) > (In reply to Julian Descottes [:jdescottes] from comment #14) > > (In reply to Jan Keromnes [:janx] from comment #11) > > > Nit: Generally for elements like these, devtools actors will implement a > > > dedicated getter (i.e. `get activeWorkerForm()`) instead of a transform > > > method for delegate components (i.e. `_getWorkerForm()` for > > > `_registration.{installing,active,waiting}Worker`). Please do the same here, > > > and move the 3 getters above the `form` method. > > > > > > > Not sure I understood 100% what should be done here, I will upload a new > > version shortly, let me know if this is what you had in mind ? I could not > > find similar examples in our existing actors though. Would you mind > > explaining what this guideline is for (I'm fine with "for consistency" but > > then I'd like to have an example from our codebase :) ) > > Fair enough, sorry for being a little vague about this. You addressed most > of my comment, but I was referring to actual JS getters like in [0] or [1]. > > [0] > https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script. > js#479 > [1] > https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/common. > js#331 > > This would give something like: > > get activeWorkerForm() { > return this._getWorkerForm(this._registration.activeWorker); > }, > > form: function(detail) { > return { > activeWorker: this.activeWorkerForm, > }; > }, > > This doesn't apply to _getWorkerForm(workerInfo) however. Speaking of which, > this method doesn't quite feel right in ServiceWorkerRegistrationActor (ok > this is getting very vague), but maybe at some point it will become a proper > form() getter within a dedicated ServiceWorkerActor class. > Thanks for the precisions, I updated the code to use real getters (still not used to those apparently). I agree about having a dedicated worker and I was unsure about this. I opted for keeping things simple for now, but we can change this before landing. > If you prefer to generate an intermediate string, that's fine by me, but I > still believe that just passing `ServiceWorkerInfo.state` along also works > fine. > > To illustrate, here is where the worker actor sends `WorkerDebugger.type` > as-is[2], and here where the client code figures it out[3]. As you can see, > no intermediate string was generated, and there is no duplicated enum to > maintain. > I did not realize I could simply reuse the enum from my react component, that's why I kept mentioning double maintenance etc ... :) It's gone now, thanks!

(In reply to Jan Keromnes [:janx] from comment #13) > Created attachment 8734702 [details] > service-worker-status.png I actually really like what you currently have—I think these labels should work without issue. > - Installing (orange, moccasin): Currently installing (shouldn't last long, > but may get stuck here in some cases) > - Activating (gold, lightyellow): Installed, currently activating (same as > above) These are the only two we might consider changing; yellow and orange are normally associated with error/warning states, and installing/activating aren't warnings but rather installation steps. Maybe we could cycle through a blue and a purple, respectively. A nice change would be installing changing to orange if it got stuck, but I'm not sure if it's currently designed in such a way where you can tell?

Julian, could you please reply to comment 23 when you get a chance?

(In reply to Jan Keromnes [:janx] from comment #24) > Julian, could you please reply to comment 23 when you get a chance? Missed the question, thanks for pinging me. (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #23) > (In reply to Jan Keromnes [:janx] from comment #13) > > Created attachment 8734702 [details] > > service-worker-status.png > I actually really like what you currently have—I think these labels should > work without issue. > > > - Installing (orange, moccasin): Currently installing (shouldn't last long, > > but may get stuck here in some cases) > > - Activating (gold, lightyellow): Installed, currently activating (same as > > above) > These are the only two we might consider changing; yellow and orange are > normally associated with error/warning states, and installing/activating > aren't warnings but rather installation steps. Maybe we could cycle through > a blue and a purple, respectively. Sure, thanks for the suggestions! > > A nice change would be installing changing to orange if it got stuck, but > I'm not sure if it's currently designed in such a way where you can tell? In most of the examples I've seen the service worker installs almost instantly. Technically we should receive an event when the service worker state changes. We could set a timer that would modify the displayed state after reaching a certain threshold. I'm still facing technical problems to actually receive service worker state change events, so for now I would like to push this to a follow up and assume that in most cases the waiting/installation/activation will be fast.

Hey ! I'm currently trying out the tools around service workers we have in Firefox, and I found this bug which seems really important to me. I'd like to make sure you thought about the updating process as well, where 2 versions of a service worker cohabits for some time. The Chrome Devtools get this part quite right, where we clearly see the current state of the service worker, making it also possible to trigger the various lifecycle steps: especially triggering update, and calling skipWaiting.

Trying to resume my work on this bug, but I still hit a wall when trying to get information about service workers in other states than ACTIVATED. For my tests I am using https://juliandescottes.github.io/sw-test-delay/ . It is a modified version of https://github.com/mdn/sw-test/ which waits for 5 seconds between the INSTALLED and the ACTIVATED steps. After making sure the service worker is currently unregistered, I load https://juliandescottes.github.io/sw-test-delay/ . I would expect calls to ServiceWorkerManager::GetAllRegistrations to return an array containing ServiceWorkerRegistrationInfo with an "installing" worker. But it just returns an empty array. It looks like we never go into the following for loop: > for (auto it2 = it1.UserData()->mInfos.Iter(); !it2.Done(); it2.Next()) which seems weird since loading the https://juliandescottes.github.io/sw-test-delay/ triggered a call to ServiceWorkerManager::AddScopeAndRegistration, which successfully made its way to "data->mInfos.Put(aScope, aInfo);" Only after the serviceworker reaches the ACTIVATED state will getAllRegistrations return a registration. @Ben: Could you help me investigate what is wrong here? I don't know enough with C++ to efficiently make progress on this.

I'm fairly certain this is due to an issue :asuth pointed out to me recently. We only propagate activated service workers across process boundaries. We don't propagate other state changes. I think you will probably need to wait to implement this until we move the SWM to the parent process. We could try to propagate state changes around processes, but it would be very racy and likely introduce bugs. I'd like to avoid it if possible. Andrew, do you concur?

Or if we want to try propagating them right now, we should make sure its only from child-to-parent. If we avoid trying to propagate from parent-to-child then it would probably be ok.

(In reply to Ben Kelly [:bkelly] from comment #28) > I'm fairly certain this is due to an issue :asuth pointed out to me > recently. We only propagate activated service workers across process > boundaries. We don't propagate other state changes. > > I think you will probably need to wait to implement this until we move the > SWM to the parent process. > > We could try to propagate state changes around processes, but it would be > very racy and likely introduce bugs. I'd like to avoid it if possible. > Thanks for the info, I don't think there's any reason to rush this. Any bug we should follow here to know when this will be ready for implementation? For the time being, we will simply display if a ServiceWorker is RUNNING or STOPPED.

As mentioned in my previous comment, for now we are only aiming at describing if a service worker is in one of the 3 following states: - activated & running - activated & stopped - not activated Detecting the not activated state can be done in e10s because we get a service worker for which no registration can be found. In non e10s though, registrations are retrieved before they reach the activated state. For this case we need to start implementing what I had in my first patches uploaded here. Cleaned up version currently on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=588f3f5118eb. Will submit for review if try is green.

Don't we already basically expose this information? If the Debug button has replaced the Start button, then its running?

(In reply to Ben Kelly [:bkelly] from comment #32) > Don't we already basically expose this information? If the Debug button has > replaced the Start button, then its running? As you describe it, it is exposed but indirectly. The goal here is to make it explicit. First of all, I think it's easier to understand for the user. And for us this is a way to start adding the UI we will reuse to display the other states (when we can do so cf your previous comment). However the "not activated" state is currently not exposed. At the moment: - [e10s] registration is simply not displayed - [non-e10s] registration is displayed as if it was running We discussed with Jan and thought that if we could bring this extra bit of information right now (without waiting for the changes you described) that would already be a nice feature to have.

Comment on attachment 8780620 [details] Bug 1153292 - part3: aboutdebugging: expose sw state from registration, notify listeners on statechange; https://reviewboard.mozilla.org/r/71282/#review68796 ::: dom/interfaces/base/nsIServiceWorkerManager.idl:32 (Diff revision 1) > interface nsIWorkerDebugger; > > [scriptable, builtinclass, uuid(76e357ed-208d-4e4c-9165-1c4059707879)] > interface nsIServiceWorkerInfo : nsISupports > { > + const unsigned short STATE_INSTALLING = 1; We should make these exactly equivalent to the existing enum: https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/ServiceWorkerBinding.h#32 Your "unknown" would map to ServiceWorkerState::EndGuard_. ::: dom/workers/ServiceWorkerInfo.cpp:36 (Diff revision 1) > NS_IMETHODIMP > +ServiceWorkerInfo::GetState(uint16_t* aState) > +{ > + MOZ_ASSERT(aState); > + AssertIsOnMainThread(); > + if (mState == ServiceWorkerState::Redundant) { Once the type definitions are aligned you can just assign from one to the other here. Please add static_assert() statements in a .cpp to ensure that the enum and IDL constant values match, though. For example: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#102 ::: dom/workers/ServiceWorkerManager.cpp:3530 (Diff revision 1) > +ServiceWorkerManager::NotifyListenersOnChange( > + nsIServiceWorkerRegistrationInfo* aInfo) > +{ > + nsTArray<nsCOMPtr<nsIServiceWorkerManagerListener>> listeners(mListeners); > + for (size_t index = 0; index < listeners.Length(); ++index) { > + listeners[index]->OnStateChanged(aInfo); Why isn't this just on the existing listener attached to the registration info object? Why do we need another set of listeners?

Comment on attachment 8780616 [details] Bug 1153292 - part0: aboutdebugging: titles on target-name components; https://reviewboard.mozilla.org/r/71274/#review69386 Thanks Julian! R+, but please prefer alphabetical order where possible. Nit: Please move all 4 new `title` attributes *after* their `className` neighbour.

Comment on attachment 8780616 [details] Bug 1153292 - part0: aboutdebugging: titles on target-name components; https://reviewboard.mozilla.org/r/71274/#review69388 ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:130 (Diff revision 1) > + }, pushSubscription.endpoint)) : > null > ), > dom.li({ className: "target-detail" }, > dom.strong(null, Strings.GetStringFromName("scope")), > dom.span({ className: "service-worker-scope" }, target.scope), Nit: Why add a `title` to "service-worker-push-url", but not to "service-worker-scope"? Please make it consistent.

Comment on attachment 8780617 [details] Bug 1153292 - part1: display serviceworker status RUNNING/STOPPPED in aboutdebugging; https://reviewboard.mozilla.org/r/71276/#review69390 I really like the new labels, but I feel like they take up a lot of valuable horizontal space. R+ (with nits) because it's already an improvement, but I hope that we'll find ways to improve Service Worker UX even more in the near future (e.g. by removing the scope from the scriptURL? I remember Helen pointing out the visual duplication of Target Name and Scope). ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:106 (Diff revision 1) > + getServiceWorkerStatus() { > + return this.isRunning() ? "running" : "stopped"; > + }, Nit: Please use real getters (e.g. `get serviceWorkerStatus`) instead of method names that start with "get". ::: devtools/client/aboutdebugging/components/workers/target.js:34 (Diff revision 1) > + (status ? > + dom.span({ > + className: `target-status target-status-${status}`, > + }, Strings.GetStringFromName(status)) : null > + ), This code looks dead, but I imagine you consider adding status information to Dedicated/Shared Workers too?

Comment on attachment 8780618 [details] Bug 1153292 - part4: aboutdebugging: display registering status for service workers; https://reviewboard.mozilla.org/r/71278/#review69404 Thanks for adding Service Workers that are not yet active! I know this feature will be useful to a number of developers. R- because of a duplicate code block, and a few issues to be resolved. ::: devtools/client/aboutdebugging/components/workers/panel.js:119 (Diff revision 1) > + _updateServiceWorker(form) { > + let workers = this.getInitialState().workers; > + let registrationFound = false; > + for (let registration of workers.service) { > + if (registration.scope === form.scope) { > + // XXX: Race, sometimes a ServiceWorkerRegistrationInfo doesn't > + // have a scriptSpec, but its associated WorkerDebugger does. > + if (!registration.url) { > + registration.name = registration.url = form.url; > + } > + registration.workerActor = form.actor; > + registrationFound = true; > + break; > + } > + } > + if (!registrationFound) { > + let serviceWorker = { > + icon: WorkerIcon, > + name: form.url, > + url: form.url, > + scope: form.scope, > + registrationActor: null, > + workerActor: form.actor > + }; > + workers.service.push(serviceWorker); > + } > + }, > + This code looks duplicated, you were probably refactoring it. Please rework these changes into a coherent patch. Nit: While I'm at it, React components don't really expose public or private methods. Please don't prefix methods with underscores. ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:34 (Diff revision 1) > + componentDidUpdate(oldProps, oldState) { > + let oldRegistrationActor = oldProps.target && oldProps.target.registrationActor; > + let registrationActor = this.props.target && this.props.target.registrationActor; > + > + if (!oldRegistrationActor && registrationActor) { > + // Component just reached registration stage and should refresh push information. > + this.updatePushSubscription(); > + } > + }, I don't understand why we need to do this. Wouldn't a "push-subscription-modified" event be sent anyway if a newly registered service worker also subscribed to the Push service? ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:50 (Diff revision 1) > - client.removeListener("push-subscription-modified", > + client.removeListener("push-subscription-modified", this.onPushSubscriptionModified); > - this.onPushSubscriptionModified); > }, > > debug() { > - if (!this.isRunning()) { > + if (!this.isActivated() || !this.isRunning()) { Why forbid debugging of an installing worker? You already have a `workerActor` so it should work. It's interesting to debug a service worker prior to it being active, e.g. if you're troubleshooting update issues. ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:60 (Diff revision 1) > let { client, target } = this.props; > debugWorker(client, target.workerActor); > }, > > push() { > - if (!this.isRunning()) { > + if (!this.isActivated() || !this.isRunning()) { Forbidding pushing to a service worker before it's active makes more sense, because I believe the "push" event would be lost anyway. ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:120 (Diff revision 1) > // We know the target is running if it has a worker actor. > return !!this.props.target.workerActor; > }, > > + isActivated() { > + return !!this.props.target.registrationActor; Nit: Please add a small comment (as in `isRunning`) to explain why `registrationActor` can be falsy. ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:129 (Diff revision 1) > - return this.isRunning() ? "running" : "stopped"; > + if (this.isActivated() && this.isRunning()) { > + return "running"; > + } else if (this.isActivated()) { > + return "stopped"; > + } > + return "inactive"; Nit: The word "inactive" might be a bit confusing here, because it's supposed to mean "installing or waiting", but users might think "it's not doing anything". I'm OK with disambiguating "installing" and "waiting" stages in a follow-up, but in the meantime, maybe we could find a non-idle word like "setting up", "registering", or similar? Or we could lie slightly by just saying "installing", and fix this soon after. ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:157 (Diff revision 1) > + onClick: this.start > + }, Strings.GetStringFromName("start")) > + ); > + }, > + > + renderUnregisterLink() { Does this really need a separate render function?

Comment on attachment 8780622 [details] Bug 1153292 - part6: add tests for pending state service workers and fix existing tests; Removing review requests for the two last parts as they need to be reworked a bit after applying review comments on part 4.

Comment on attachment 8780620 [details] Bug 1153292 - part3: aboutdebugging: expose sw state from registration, notify listeners on statechange; https://reviewboard.mozilla.org/r/71282/#review68796 Thanks for the review! Applied your comments (as much as I could), and removed the new listeners on ServiceWorkerManager. > Why isn't this just on the existing listener attached to the registration info object? Why do we need another set of listeners? Indeed I can use the events from the registration info here, removed the new listeners.

Comment on attachment 8780618 [details] Bug 1153292 - part4: aboutdebugging: display registering status for service workers; https://reviewboard.mozilla.org/r/71278/#review69404 > This code looks duplicated, you were probably refactoring it. Please rework these changes into a coherent patch. > > Nit: While I'm at it, React components don't really expose public or private methods. Please don't prefix methods with underscores. Done! > I don't understand why we need to do this. Wouldn't a "push-subscription-modified" event be sent anyway if a newly registered service worker also subscribed to the Push service? From the tests I performed, I think this is needed here. 1- service worker is installing 2- component is created for the service worker, no registrationActor is available -> updatePushSubscription cannot get the push information 3- service worker is activated 4- registration is retrieved by about:debugging, it already has its subscription 5- the component already exists so we don't go in componentDidMount, and the subscription is already there so no event will be received either This is only needed because we are displaying service workers for which the registration is not available yet (which should only be a temporary measure). I updated the comment to describe more or less what I said above. Let me know if I'm missing something. > Why forbid debugging of an installing worker? You already have a `workerActor` so it should work. It's interesting to debug a service worker prior to it being active, e.g. if you're troubleshooting update issues. The reason I wanted to disable everything for "inactive/registering" service workers is because we are using a temporary workaround to display such workers (no registration etc...). I wanted to wait until we had a proper implementation to decide which actions were available for which state. That being said, I followed your advice and enabled the button & action here. > Nit: The word "inactive" might be a bit confusing here, because it's supposed to mean "installing or waiting", but users might think "it's not doing anything". > > I'm OK with disambiguating "installing" and "waiting" stages in a follow-up, but in the meantime, maybe we could find a non-idle word like "setting up", "registering", or similar? Or we could lie slightly by just saying "installing", and fix this soon after. Good point about using a name that hints at something being "in progress". Used "registering" for now. I wouldn't use an actual state name, because I'm not sure when we will be able to have the real implementation and I would prefer to avoid releasing a version of the UI displaying incorrect information. > Does this really need a separate render function? I find ternary operators in render() methods hard to read and confusing. If you feel strongly about this I can move it back in the main render().

In a component, I tried using a getter in a class created via React.createClass(). > createClass({ > get serviceWorkerStatus() { > return this.props.target.workerActor ? "running" : "stopped"; > } > }); I had two issues with this. 1- the getter is read when React creates the class (in mixSpecIntoComponent). At this stage this.props is not set which means I have to guard this. 2- even after guarding it, React reads the getter a first time, caches the value and the getter is never called afterwards. The class is just using the value it had when createClass was called. Lin: I could not find a clear answer about this on the web. Do you know if we are we not supposed to use getters/setters with React.createClass()?

I haven't seen anyone try to use getters with React.createClass before, so I can't say for sure. But I would expect that there is no intention to support them.

Comment on attachment 8780620 [details] Bug 1153292 - part3: aboutdebugging: expose sw state from registration, notify listeners on statechange; https://reviewboard.mozilla.org/r/71282/#review69956 Thanks! ::: dom/interfaces/base/nsIServiceWorkerManager.idl:32 (Diff revision 4) > interface nsIWorkerDebugger; > > [scriptable, builtinclass, uuid(76e357ed-208d-4e4c-9165-1c4059707879)] > interface nsIServiceWorkerInfo : nsISupports > { > + const unsigned short STATE_INSTALLING = 0; nit: Maybe add a comment here saying the values correspond to the ServiceWorkerState enumeration.

Comment on attachment 8780618 [details] Bug 1153292 - part4: aboutdebugging: display registering status for service workers; https://reviewboard.mozilla.org/r/71278/#review72754 Looks good to me! Thanks, R+ with a few nits and questions. (In reply to Julian Descottes [:jdescottes] from comment #53) > > I don't understand why we need to do this. Wouldn't a "push-subscription-modified" event be sent anyway if a newly registered service worker also subscribed to the Push service? > > From the tests I performed, I think this is needed here. > 1- service worker is installing > 2- component is created for the service worker, no registrationActor is > available -> updatePushSubscription cannot get the push information > 3- service worker is activated > 4- registration is retrieved by about:debugging, it already has its > subscription > 5- the component already exists so we don't go in componentDidMount, and the > subscription is already there so no event will be received either > > This is only needed because we are displaying service workers for which the > registration is not available yet (which should only be a temporary > measure). > I updated the comment to describe more or less what I said above. Let me > know if I'm missing something. Ok, thanks for the reasoning. As explained below, the call to `updatePushSubscription()` from `componentDidMount()` is there just in case there was a pre-existing Push Subscription before we loaded about:debugging, which means we missed to original "push-subscription-modified" event. In the case you mention, the call to `updatePushSubscription()` has no effect in the `componentDidMount()` of a still-installing service worker, but I believe that there can't be a pre-existing Push Subscription here, because a service worker needs to be fully registered before it can even ask to subscribe to Push events. So, once the registration completes, the component will discover about the registrationActor, it won't call `componentDidMount()` again obviously, but any web app code that attempts to subscribe to Push will happen after that point, so the "push-subscription-modified" event should still trigger after you received the registrationActor, and `updatePushSubscription()` will be called after all (except if the client never subscribes, in which case there is no subscription and we don't need to ever call that method). Now that's the theory of how I think about:debugging / Service Worker / Push all work, but I might be missing something or be wrong. Have you seen what happens in practice? Is it really possible that a Push subscription arrives before a registration is complete? > > Why forbid debugging of an installing worker? You already have a `workerActor` so it should work. It's interesting to debug a service worker prior to it being active, e.g. if you're troubleshooting update issues. > > The reason I wanted to disable everything for "inactive/registering" service > workers is because we are using a temporary workaround to display such > workers (no registration etc...). > I wanted to wait until we had a proper implementation to decide which > actions were available for which state. > That being said, I followed your advice and enabled the button & action here. Thanks for the explanation, I understand your position, but thanks for enabling the button anyway. I believe the functionality should just work, so there is no point in blocking access to it. > > Nit: The word "inactive" might be a bit confusing here, because it's supposed to mean "installing or waiting", but users might think "it's not doing anything". > > > > I'm OK with disambiguating "installing" and "waiting" stages in a follow-up, but in the meantime, maybe we could find a non-idle word like "setting up", "registering", or similar? Or we could lie slightly by just saying "installing", and fix this soon after. > > Good point about using a name that hints at something being "in progress". > Used "registering" for now. > I wouldn't use an actual state name, because I'm not sure when we will be > able to have the real implementation and I would prefer to avoid releasing a > version of the UI displaying incorrect information. Makes sense. Thanks! > > Does this really need a separate render function? > > I find ternary operators in render() methods hard to read and confusing. If > you feel strongly about this I can move it back in the main render(). If you really prefer a separate render function, that's fine by me, you're the owner after all :) I just wanted to make sure it wasn't a small function accidentally left over from a bigger refactor. ::: devtools/client/aboutdebugging/components/workers/panel.js:87 (Diff revision 5) > - if (!registration.url) { > + if (!registration.url) { > - registration.name = registration.url = form.url; > + registration.name = registration.url = form.url; > - } > + } > - registration.workerActor = form.actor; > + registration.workerActor = form.actor; > - break; > - } > + } else { > + workers.service.push({ Nit: Please briefly explain in a comment why we still add a "registration" even when there is none. ::: devtools/client/aboutdebugging/components/workers/panel.js:113 (Diff revision 5) > > this.setState({ workers }); > }); > }, > > + getRegistrationForWorker(form, registrations) { Quick question: are you passing `form` instead of `scope` here because you expect there might be more to a `form` needed to identify a service worker in the future? ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:38 (Diff revision 5) > + let oldRegistrationActor = oldProps.target && oldProps.target.registrationActor; > + let registrationActor = this.props.target && this.props.target.registrationActor; > + > + if (!oldRegistrationActor && registrationActor) { > + // If the component was created without a registrationActor, updatePushSubscription > + // was not called in componentDidMount, and if the subscription is already attached Well, technically updatePushSubscription *was* called in componentDidMount, but it bailed out due to lacking registrationActor. And that call is there only because about:debugging might be loaded after pre-existing service workers subscribed already (so they have a subscription, but no further event might be triggered). I'm still not convinced that a not-yet-fully-registered service worker, even pre-existing before about:debugging is loaded, could have a pushSubscription we won't hear about again. ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:40 (Diff revision 5) > + > + if (!oldRegistrationActor && registrationActor) { > + // If the component was created without a registrationActor, updatePushSubscription > + // was not called in componentDidMount, and if the subscription is already attached > + // to the registration when the registration becomes accessible to about:debugging > + // the "push-subscription-modified" event will not be triggered. If the "push-subscription-modified" event can happen before the registration becomes available, fair enough. But I thought that a registration would always be complete and available in about:debugging before the push subscription even happens, and so by the time `registration.pushManager.subscribe()` is called the component would already have a `registrationActor`. Has it happened to you that a registration was able to subscribe to Push before about:debugging even heard about the registration? ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:124 (Diff revision 5) > // We know the target is running if it has a worker actor. > return !!this.props.target.workerActor; > }, > > - get serviceWorkerStatus() { > - return this.isRunning() ? "running" : "stopped"; > + isActive() { > + return !!this.props.target.registrationActor; Nit: Comment 45 asked for a small comment here. Please either add one, or briefly explain why you won't.

Comment on attachment 8780619 [details] Bug 1153292 - part2: aboutdebugging: fix service worker layout if buttons are not displayed; https://reviewboard.mozilla.org/r/71280/#review72782 LGTM!

Comment on attachment 8783590 [details] Bug 1153292 - part5: aboutdebugging: rely on activeWorker to decide if registration is activated; https://reviewboard.mozilla.org/r/73348/#review72800 R+ with nits: ::: devtools/client/aboutdebugging/components/workers/panel.js:45 (Diff revision 1) > componentDidMount() { > let client = this.props.client; > client.addListener("workerListChanged", this.update); > client.addListener("serviceWorkerRegistrationListChanged", this.update); > client.addListener("processListChanged", this.update); > + client.addListener("registration-changed", this.update); Nit: Why update the whole WorkerPanel when a ServiceWorkerRegistration changes? Why not just each ServiceWorkerTarget component, like for "push-subscription-changed" events? (That way in the future, both registration-specific events could be prevented from unnecessarily updating all the other unrelated registrations). ::: devtools/client/aboutdebugging/components/workers/panel.js:70 (Diff revision 1) > + // no worker are actually registrations with a hidden activating worker. > + // - In non-e10s: registrations always have at least one worker, if it is an > + // active worker, the registration is active. > + let hasWorker = form.activeWorker || form.waitingWorker || form.installingWorker; > + let isE10s = Services.appinfo.browserTabsRemoteAutostart; > + let active = (isE10s && !hasWorker) || form.activeWorker; Nit: Checking for `form.activeWorker` seems to me like the general case, with the e10s weirdness a temporary special-case workaround. Please place the general case first in this composite condition. Also, you're betting on the fact that in e10s, the platform won't ever give you not-yet-active registrations, without any attached worker (this code would wrongly flag such a registration as "active"). Ideally one day, even e10s will send not-yet-active registrations that have an installingWorker or waitingWorker, but is there a safeguard to prevent the edge case where a not-yet-active registration has no available worker? ::: devtools/server/actors/worker.js:375 (Diff revision 1) > + > + get waitingWorkerForm() { > + return this._getWorkerForm(this._registration.waitingWorker); > + }, > + > + _getWorkerForm: function (worker) { Note: Eventually, you might want to create a separate ServiceWorkerActor taking a ServiceWorkerInfo parameter, create and attach these actors to their ServiceWorkerRegistrationActor, and move `ServiceWorkerRegistrationActor._getWorkerForm(worker)` to `ServiceWorkerActor.form()`. It won't replace `WorkerActor` and the associated debug functionality yet (because they won't live in the right process for that), but at first it will simply be a cleaner location for this separate `_getWorkerForm` method, and maybe eventually we'll figure out how to associate a `WorkerActor` to its `ServiceWorkerActor` here instead of in about:debugging.

Comment on attachment 8783591 [details] Bug 1153292 - part6: add tests for pending state service workers and fix existing tests; https://reviewboard.mozilla.org/r/73350/#review72806 R+ (with nits) provided everything works fine on try. Note: It felt a bit weird to review all the previous patches without tests/fixes. In future patch series, please include feature-specific test changes along with their feature changes. Thanks! ::: devtools/client/aboutdebugging/test/browser_service_workers_status.js:20 (Diff revision 1) > + ["dom.serviceWorkers.enabled", true], > + ["dom.serviceWorkers.testing.enabled", true], > + ["dom.serviceWorkers.idle_timeout", SW_TIMEOUT], > + ["dom.serviceWorkers.idle_extended_timeout", SW_TIMEOUT], > + ]}; > + SpecialPowers.pushPrefEnv(options, done); Nit: A few small things have changed in the way we write tests for about:debugging. Please refer to browser_service_workers_push_service.js for an idea about current best practices, e.g. yielding directly on `SpecialPowers.pushPrefEnv` instead of creating an ad-hoc `Promise`. ::: devtools/client/aboutdebugging/test/browser_service_workers_status.js:57 (Diff revision 1) > + let aboutDebuggingUpdate = waitForMutation(serviceWorkersElement, > + { childList: true }); > + > + try { > + yield unregisterServiceWorker(swTab); > + ok(true, "Service worker registration unregistered"); Nit: Unnecessary word "registration". ::: devtools/client/aboutdebugging/test/browser_service_workers_unregister.js:41 (Diff revision 1) > // Wait for the service workers-list to update. > yield onMutation; > > + // Wait for the unregister link to be available, when the service worker reaches the > + // ACTIVATED state. > + yield waitForMutation(serviceWorkersElement, { childList: true, subtree: true }); Nit: Wouldn't it be cleaner here to just add `subtree: true` in `onMutation`? Or maybe you expect exactly two mutations?

Again, thank you Julian for all this good work! At this point, please ensure the follow-up work of de-duplicating "installing" and "waiting" won't be forgotten about (e.g. by filing a follow-up bug). Also, once this has landed, I think it will be a good time to have a more involved UX discussion with Helen about our Service Worker tools. It might also be a good idea to involve Sole, who is great at rooting out design bugs or anything that can might confusing to a user.

Thanks for the reviews Jan. Replies to your main question below > re: `updatePushSubscription()` in `componentDidUpdate()` Thanks for pushing back about this issue, made me investigate more. The initial reason I added this was because browser_service_workers_push_service.js was failing with the new code. I didn't realize that the subscription was only occurring after the registration. I still think that we need to call updatePushSubscription on componentDidUpdate (sorry :) ) but only in the temporary e10s case. If the push-subscription-modified event fired before the component got updated with the registrationActor, the information will be missed. There is no way to ensure the component will be updated before the subscription event is received. This means I can rework the condition to run the updatePushSubscription to revolve around "is registrationActor already available or not", which makes more sense technically. But after reading your comment, I thought that this should work in non-e10s, since the component has a registrationActor from the beginning. And it still doesn't work, but this time it looks like it is coming from a bug in DebuggerClient: - sw registers - push subscription modified fired from the sw-registration - service worker target calls getPushSubscription for its sw-registration - debugger client registers an active request for this sw-registration actor - sw-registration actor emits "registration-changed" event - debugger client receives the packet for "registration-changed" and matches it with the current active request for this sw-registration actor, which is coming from getPushSubscription (feel free to test by yourself by removing the call to updatePushSubscription in componentDidUpdate and running browser_service_workers_push_service) So we do receive the push-subscription-modified event, but the bug/limitation described above makes the subsequent call to getPushSubscription useless. I wonder if I should not keep my current implementation with a comment explaining the two issues mentioned above (e10s + race condition in debuggerClient) so that we can go forward with this.

Updated the patches addressing some of your comments, details below. (In reply to Jan Keromnes [:janx] from comment #73) > Comment on attachment 8780618 [details] > Bug 1153292 - part2: aboutdebugging: display registering status for service > workers; > > https://reviewboard.mozilla.org/r/71278/#review72754 > > Looks good to me! Thanks, R+ with a few nits and questions. > > (In reply to Julian Descottes [:jdescottes] from comment #53) > > > I don't understand why we need to do this. Wouldn't a "push-subscription-modified" event be sent anyway if a newly registered service worker also subscribed to the Push service? > > > > From the tests I performed, I think this is needed here. > > 1- service worker is installing > > 2- component is created for the service worker, no registrationActor is > > available -> updatePushSubscription cannot get the push information > > 3- service worker is activated > > 4- registration is retrieved by about:debugging, it already has its > > subscription > > 5- the component already exists so we don't go in componentDidMount, and the > > subscription is already there so no event will be received either > > > > This is only needed because we are displaying service workers for which the > > registration is not available yet (which should only be a temporary > > measure). > > I updated the comment to describe more or less what I said above. Let me > > know if I'm missing something. > > Ok, thanks for the reasoning. As explained below, the call to > `updatePushSubscription()` from `componentDidMount()` is there just in case > there was a pre-existing Push Subscription before we loaded about:debugging, > which means we missed to original "push-subscription-modified" event. > > [...] > > Now that's the theory of how I think about:debugging / Service Worker / Push > all work, but I might be missing something or be wrong. Have you seen what > happens in practice? Is it really possible that a Push subscription arrives > before a registration is complete? > Still investigating, see my previous comment. > > ::: devtools/client/aboutdebugging/components/workers/panel.js:113 > (Diff revision 5) > > > > this.setState({ workers }); > > }); > > }, > > > > + getRegistrationForWorker(form, registrations) { > > Quick question: are you passing `form` instead of `scope` here because you > expect there might be more to a `form` needed to identify a service worker > in the future? > Yes. > > ::: > devtools/client/aboutdebugging/components/workers/service-worker-target.js: > 124 > (Diff revision 5) > > // We know the target is running if it has a worker actor. > > return !!this.props.target.workerActor; > > }, > > > > - get serviceWorkerStatus() { > > - return this.isRunning() ? "running" : "stopped"; > > + isActive() { > > + return !!this.props.target.registrationActor; > > Nit: Comment 45 asked for a small comment here. Please either add one, or > briefly explain why you won't. I should have folded my patches. In part5, we have the active property directly computed by the panel (because it knows if the registration was a fake registration added for e10s or not). There is no need for a comment after part 5 so I didn't feel like amending this previous changeset was bringing anything. I will fold parts 2, 5 and 6 before landing. (In reply to Jan Keromnes [:janx] from comment #75) > Comment on attachment 8783590 [details] > Bug 1153292 - part5: aboutdebugging: rely on activeWorker to decide if > registration is activated; > > https://reviewboard.mozilla.org/r/73348/#review72800 > > R+ with nits: > > ::: devtools/client/aboutdebugging/components/workers/panel.js:45 > (Diff revision 1) > > componentDidMount() { > > let client = this.props.client; > > client.addListener("workerListChanged", this.update); > > client.addListener("serviceWorkerRegistrationListChanged", this.update); > > client.addListener("processListChanged", this.update); > > + client.addListener("registration-changed", this.update); > > Nit: Why update the whole WorkerPanel when a ServiceWorkerRegistration > changes? Why not just each ServiceWorkerTarget component, like for > "push-subscription-changed" events? (That way in the future, both > registration-specific events could be prevented from unnecessarily updating > all the other unrelated registrations). As discussed on IRC, it's not easy to implement right now as it would mean updating the props for the service-worker-target. But that's an improvement we should keep in mind for the future. > > ::: devtools/client/aboutdebugging/components/workers/panel.js:70 > (Diff revision 1) > > + // no worker are actually registrations with a hidden activating worker. > > + // - In non-e10s: registrations always have at least one worker, if it is an > > + // active worker, the registration is active. > > + let hasWorker = form.activeWorker || form.waitingWorker || form.installingWorker; > > + let isE10s = Services.appinfo.browserTabsRemoteAutostart; > > + let active = (isE10s && !hasWorker) || form.activeWorker; > > Nit: Checking for `form.activeWorker` seems to me like the general case, > with the e10s weirdness a temporary special-case workaround. Please place > the general case first in this composite condition. > > Also, you're betting on the fact that in e10s, the platform won't ever give > you not-yet-active registrations, without any attached worker (this code > would wrongly flag such a registration as "active"). Ideally one day, even > e10s will send not-yet-active registrations that have an installingWorker or > waitingWorker, but is there a safeguard to prevent the edge case where a > not-yet-active registration has no available worker? > Not sure how to create a safeguard here, other than updating this code as soon as the implementation gets fixed in e10s. We rely on this in order to detect service worker registrations in activating state, but afaik there is no way to differentiate them from a buggy registration with no available worker. The alternative was to only rely on active registrations with a worker in activated state (not activating). The issue is that this does not represent a state change for the sw registration, so right now I don't have any event to listen to to be notified about this. We might have to additionally listen to service worker state changes (on top of service worker registration state changes) in the future. I was thinking we could revisit this when the e10s implementation gets sorted out. > ::: devtools/server/actors/worker.js:375 > (Diff revision 1) > > + > > + get waitingWorkerForm() { > > + return this._getWorkerForm(this._registration.waitingWorker); > > + }, > > + > > + _getWorkerForm: function (worker) { > > Note: Eventually, you might want to create a separate ServiceWorkerActor > taking a ServiceWorkerInfo parameter, create and attach these actors to > their ServiceWorkerRegistrationActor, and move > `ServiceWorkerRegistrationActor._getWorkerForm(worker)` to > `ServiceWorkerActor.form()`. > > It won't replace `WorkerActor` and the associated debug functionality yet > (because they won't live in the right process for that), but at first it > will simply be a cleaner location for this separate `_getWorkerForm` method, > and maybe eventually we'll figure out how to associate a `WorkerActor` to > its `ServiceWorkerActor` here instead of in about:debugging. Took a shot at this in part 7. Let me know what you think. (In reply to Jan Keromnes [:janx] from comment #76) > Comment on attachment 8783591 [details] > Bug 1153292 - part6: add tests for pending state service workers and fix > existing tests; > > https://reviewboard.mozilla.org/r/73350/#review72806 > > R+ (with nits) provided everything works fine on try. > > Note: It felt a bit weird to review all the previous patches without > tests/fixes. In future patch series, please include feature-specific test > changes along with their feature changes. Thanks! You're right, will fold before landing. > > ::: devtools/client/aboutdebugging/test/browser_service_workers_status.js:20 > (Diff revision 1) > > + ["dom.serviceWorkers.enabled", true], > > + ["dom.serviceWorkers.testing.enabled", true], > > + ["dom.serviceWorkers.idle_timeout", SW_TIMEOUT], > > + ["dom.serviceWorkers.idle_extended_timeout", SW_TIMEOUT], > > + ]}; > > + SpecialPowers.pushPrefEnv(options, done); > > Nit: A few small things have changed in the way we write tests for > about:debugging. Please refer to browser_service_workers_push_service.js for > an idea about current best practices, e.g. yielding directly on > `SpecialPowers.pushPrefEnv` instead of creating an ad-hoc `Promise`. > Done normally. > ::: devtools/client/aboutdebugging/test/browser_service_workers_status.js:57 > (Diff revision 1) > > + let aboutDebuggingUpdate = waitForMutation(serviceWorkersElement, > > + { childList: true }); > > + > > + try { > > + yield unregisterServiceWorker(swTab); > > + ok(true, "Service worker registration unregistered"); > > Nit: Unnecessary word "registration". > Removed. > ::: > devtools/client/aboutdebugging/test/browser_service_workers_unregister.js:41 > (Diff revision 1) > > // Wait for the service workers-list to update. > > yield onMutation; > > > > + // Wait for the unregister link to be available, when the service worker reaches the > > + // ACTIVATED state. > > + yield waitForMutation(serviceWorkersElement, { childList: true, subtree: true }); > > Nit: Wouldn't it be cleaner here to just add `subtree: true` in > `onMutation`? Or maybe you expect exactly two mutations? I am exactly expecting two mutations. I decided to extract a utility method "waitForServiceWorkerActivation" and move it to head.js. It's non trivial code and it might get updated regularly, so having it in a single place is better. As the name suggests, it simply waits until the service worker leaves the "Registering" status.

Currently investigating a leak issue on debug builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db19bbc149ac712f96e6c59a48d9567b187fc88c

Comment on attachment 8788503 [details] Bug 1153292 - part5: create ServiceWorkerActor; https://reviewboard.mozilla.org/r/76982/#review76304 Thanks for adding a dedicated ServiceWorkerActor. I mostly have a few nits, but I wasn't able to get the `onChange` function to be called, so I'd like to figure this out first. And once we've done that, please direct future reviews of this to someone with more experience with actor life cycles and potential leaks, e.g. jryans or ochameau. ::: devtools/server/actors/worker.js:344 (Diff revision 3) > this._subscription = null; > }, > }); > > +let ServiceWorkerActor = > +protocol.ActorClassWithSpec(serviceWorkerSpec, { Nit: No need for the link break here. ::: devtools/server/actors/worker.js:394 (Diff revision 3) > - } > > - return { url: worker.scriptSpec, state: worker.state }; > + Services.obs.addObserver(this, PushService.subscriptionModifiedTopic, false); > }, > > onChange: function () { Nit: Local style is `onChange() {` instead of `onChange: function () {`. Also, is this actually ever called? I can't get it to be called, with or without e10s, even though I register, update and unregister several service workers from serviceworke.rs. Am I doing something wrong? ::: devtools/server/actors/worker.js:399 (Diff revision 3) > onChange: function () { > + this._installingWorker.destroy(); > + this._activeWorker.destroy(); > + this._waitingWorker.destroy(); > + > + let {installingWorker, waitingWorker, activeWorker} = this._registration; Nit: Not very important, but please keep a consistent order among the three workers. Chronological (installing, waiting, active) seems to be the standard everywhere, but you seem to keep a strange order (installing, active, waiting) in many places here.

(In reply to Jan Keromnes [:janx] from comment #104) > I wasn't able to get the `onChange` function to be called, so I'd like > to figure this out first. > > > onChange: function () { > > Also, is this actually ever called? I can't get it to be called, with or > without e10s, even though I register, update and unregister several service > workers from serviceworke.rs. Am I doing something wrong? I take that back, rebuilding from a clean repo fixed the problem. Sorry for the noise.

Comment on attachment 8780618 [details] Bug 1153292 - part4: aboutdebugging: display registering status for service workers; https://reviewboard.mozilla.org/r/71278/#review76580 Thanks for folding relevant patches together. Still R+, but a few additional nits/questions. ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js (Diff revision 9) > > /* eslint-env browser */ > > "use strict"; > > -const { Ci } = require("chrome"); Nit: You added this unnecessary line in a prior patch of this series, but no need to rework all your patches just to fix this (it's not worth it). ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:34 (Diff revision 9) > this.updatePushSubscription(); > }, > > + componentDidUpdate(oldProps, oldState) { > + let wasActive = oldProps.target.active; > + let isActive = this.props.target.active; Nit: Why not call `this.isActive()` here? ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:37 (Diff revision 9) > + // If the component was created without a registrationActor, updatePushSubscription > + // was not called in componentDidMount, and if the subscription is already attached > + // to the registration when the registration becomes accessible to about:debugging > + // the "push-subscription-modified" event will not be triggered. For components without a `registrationActor` / that aren't active, `updatePushSubscription` *is* actually called in `componentDidMount`, but it doesn't succeed. It might even be called again on `"push-subscription-modified"` events, and still not succeed. Maybe this comment can be simplified to something like "While the service worker isn't active, any calls to `updatePushSubscription` won't succeed. If we just became active, make sure we didn't miss a push subscription change by updating it now."? ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:77 (Diff revision 9) > }); > }, > > start() { > - if (this.isRunning()) { > - // If the worker is already running, we can't start it. > + if (!this.isActive() || this.isRunning()) { > + // If the worker is not active or already running, we can't start it. Nit: Ambiguous not/or, please repeat the "if" e.g. "or if it's already running,". ::: devtools/client/aboutdebugging/components/workers/service-worker-target.js:104 (Diff revision 9) > this.updatePushSubscription(); > } > }, > > updatePushSubscription() { > + if (!this.isActive()) { Nit: Why not check directly for `target.registrationActor` here? Is it bad to call `"getPushSubscription"` on a registration actor that's not `active` yet? ::: devtools/client/aboutdebugging/test/browser_service_workers_status.js:23 (Diff revision 9) > + yield new Promise(done => { > + let options = {"set": [ > + ]}; > + SpecialPowers.pushPrefEnv(options, done); > + }); Nit: This code doesn't seem useful. ::: devtools/client/aboutdebugging/test/service-workers/delay-sw.js:14 (Diff revision 9) > + return new Promise(resolve => { > + setTimeout(resolve, ms); > + }); > +} > + > +// Don't wait for the next page load to become the active service worker. Nit: Strange to see "Don't wait" versus "wait(1000)". Maybe add "but do that after a short delay" to your comment?

Comment on attachment 8780618 [details] Bug 1153292 - part4: aboutdebugging: display registering status for service workers; Alex, could you please double-check the changes made to `devtools/server/actors/worker.js` and `devtools/shared/specs/worker.js` (bottom of the review page), particularly with respect to the listener and potential leaks?

Comment on attachment 8788503 [details] Bug 1153292 - part5: create ServiceWorkerActor; https://reviewboard.mozilla.org/r/76982/#review76594 R+ with nits (from comment 104). ::: devtools/server/actors/worker.js:343 (Diff revision 4) > +let ServiceWorkerActor = > +protocol.ActorClassWithSpec(serviceWorkerSpec, { Nit (from comment 104): No need for the link break here. ::: devtools/server/actors/worker.js:387 (Diff revision 4) > - return this._getWorkerForm(this._registration.waitingWorker); > - }, > - > + this._installingWorker = new ServiceWorkerActor(conn, installingWorker); > + this._activeWorker = new ServiceWorkerActor(conn, activeWorker); > + this._waitingWorker = new ServiceWorkerActor(conn, waitingWorker); Nit (from comment 104): Not very important, but please keep a consistent order among the three workers. Chronological (installing, waiting, active) seems to be the standard everywhere, but you seem to keep a strange order (installing, active, waiting) in many places here. ::: devtools/server/actors/worker.js:394 (Diff revision 4) > - } > > - return { url: worker.scriptSpec, state: worker.state }; > + Services.obs.addObserver(this, PushService.subscriptionModifiedTopic, false); > }, > > onChange: function () { Nit (from comment 104): Local style is `onChange() {` instead of `onChange: function () {`.

Do you still have issues like the one reported here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db19bbc149ac712f96e6c59a48d9567b187fc88c 17:27:17 INFO - ERROR: GC found live Cell 0x7f7f61e81040 of kind FUNCTION at shutdown ? I'm not sure it has much to do with your code, that's the first time I see some message like this. It sounds more like a lower level, c++ leak. Looking at the logs, it might be related to delay-sw.html as it seems to throw these errors after this document destruction. I imagine you ensured that both ServiceWorkerActor.destroy() and ServiceWorkerRegistrationActor.destroy() were called for each instance being created? Otherwise a random guess would be about this code: self.addEventListener("install", function (event) { event.waitUntil(wait(1000)); } May be service workers leak with such code...

Another random guess, you could try to bump the timeout in the test, browser_service_workers_status.js: const SW_TIMEOUT = 2000; To something much higher as the test itself uses a 1s timeout. On debug build it can easily mess up with such timeouts...

(In reply to Alexandre Poirot [:ochameau] from comment #115) > Do you still have issues like the one reported here: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=db19bbc149ac712f96e6c59a48d9567b187fc88c > > 17:27:17 INFO - ERROR: GC found live Cell 0x7f7f61e81040 of kind > FUNCTION at shutdown > > ? > I'm not sure it has much to do with your code, that's the first time I see > some message like this. > It sounds more like a lower level, c++ leak. > Looking at the logs, it might be related to delay-sw.html as it seems to > throw these errors after this document destruction. > > I imagine you ensured that both ServiceWorkerActor.destroy() and > ServiceWorkerRegistrationActor.destroy() were called for each instance being > created? > > Otherwise a random guess would be about this code: > self.addEventListener("install", function (event) { > event.waitUntil(wait(1000)); > } > May be service workers leak with such code... It was a listener in the ServiceWorkerRegistrationActor which was not cleaned after unregistering a serviceworker, because the list did not have time to update and the actor was not destroyed. Waiting for the aboutdebugging UI to update after calling unregister in the content page fixes it. Thinking more about it, I guess the actor should be destroyed when closing the tab anyway. There might be more to this. I had the following try push with a dedicated commit to fix the leak (which was later folded) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=968f4d1f0ed8 New try push after rebasing and folding : https://treeherder.mozilla.org/#/jobs?repo=try&revision=360409b8721e

Applied the review comments so far (sorry for not catching comment 104, I did not see it before uploading the last patches). Looks like there is still the same issue we discussed last week: clicking on unregister on a service worker in non-e10s does not update the UI immediately. Not really related to this patch as it also occurs on the current mc. I will open a follow up for this.

Some updates here. # Non e10s testing Using the "New non e10s window" feature to test the non-e10s mode is not compatible with the code I use to differentiate e10s/non-e10s. Need to start firefox with --disable-e10s in order to propertly test this mode. # Unregister issue TLDR: Clicking unregister while the service-worker is between activating and activated states is buggy (with or without this patch) I use https://juliandescottes.github.io/sw-test-delay/ to test this : waits 5 seconds between installing and installed states, and then 5 seconds between activating and activated states. Here I am trying to unregister, as soon as the unregister link comes up. - in e10s: the service worker is visually removed right away, but if you reload the page that installs this serviceworker, it will appear again, and will be in "Running" state directly. - in non-e10s: the service worker remains until it reaches the activated state, at which point it is removed from the UI. But if you reload the page that installs this service-worker, the service worker will remain in "Running" state and won't disappear. So it looks like requesting an unregistration while the active worker is in activating state will wait until the worker reaches the activated state, but can be canceled if we register the same worker again. For now, I display a service worker registration as active as soon as it has an active worker. The active worker however can be in "activating" or "activated" states. I would prefer to wait until the e10s sw refactor is done, otherwise I'm afraid we will just pile up temporary hacks.

(In reply to Jan Keromnes [:janx] from comment #113) > Comment on attachment 8780618 [details] > Bug 1153292 - part4: aboutdebugging: display registering status for service > workers; > > Alex, could you please double-check the changes made to > `devtools/server/actors/worker.js` and `devtools/shared/specs/worker.js` > (bottom of the review page), particularly with respect to the listener and > potential leaks? (friendly reminder :) ) Alex, other than the suggestions you mentioned, do you have other review feedback on this patch ?

FWIW, try is green at https://treeherder.mozilla.org/#/jobs?repo=try&revision=360409b8721e&selectedJob=27520395

Comment on attachment 8780618 [details] Bug 1153292 - part4: aboutdebugging: display registering status for service workers; https://reviewboard.mozilla.org/r/71278/#review78786 My main concern is that `active` should be computed in the actor and not in the UI, otherwise I don't have strong opinion on how we should workaround current state of service workers... ::: devtools/client/aboutdebugging/components/workers/panel.js:64 (Diff revision 10) > let workers = this.getInitialState().workers; > > getWorkerForms(this.props.client).then(forms => { > forms.registrations.forEach(form => { > + // - In e10s: only active registrations are available, but if the worker is in > + // activating state it won't be available as the activeWorker. Registrations with This sentence is unclear to me. Is this one similar to what you meant? "In e10s: only active registrations are available. And even if the worker is in activating state, activeWorker will be null." ::: devtools/client/aboutdebugging/components/workers/panel.js:67 (Diff revision 10) > forms.registrations.forEach(form => { > + // - In e10s: only active registrations are available, but if the worker is in > + // activating state it won't be available as the activeWorker. Registrations with > + // no worker are actually registrations with a hidden activating worker. > + // - In non-e10s: registrations always have at least one worker, if it is an > + // active worker, the registration is active. if it *is* an active worker => if it *has* an active worker? ::: devtools/client/aboutdebugging/components/workers/panel.js:70 (Diff revision 10) > + // no worker are actually registrations with a hidden activating worker. > + // - In non-e10s: registrations always have at least one worker, if it is an > + // active worker, the registration is active. > + let hasWorker = form.activeWorker || form.waitingWorker || form.installingWorker; > + let isE10s = Services.appinfo.browserTabsRemoteAutostart; > + let active = form.activeWorker || (isE10s && !hasWorker); `active` looks like something that should be computed in the actor as it checks for Services.appinfo.browserTabsRemoteAutostart, which is a state relevant to the server, not the client. (imagine debugging workers on android from firefox desktop) Also regarding (isE10s && !hasWorker), so, it means that form.activeWorker will always be null in e10s? If not, it looks weird! ::: devtools/client/aboutdebugging/components/workers/panel.js:112 (Diff revision 10) > + url: form.url, > + scope: form.scope, > + registrationActor: null, > + workerActor: form.actor, > + active: false > + }); We already have a worker object defined on line 83, we could augment this one instead of creating a new one.

Applied review feedback from Alex, sorry about the delay! Pushed to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d23ef7c0c32

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c3cbf93bd07 part0: aboutdebugging: titles on target-name components;r=janx https://hg.mozilla.org/integration/autoland/rev/f2b131557c0a part1: display serviceworker status RUNNING/STOPPPED in aboutdebugging;r=janx https://hg.mozilla.org/integration/autoland/rev/6f119f302f36 part2: aboutdebugging: fix service worker layout if buttons are not displayed;r=janx https://hg.mozilla.org/integration/autoland/rev/ff027e2ec0ea part3: aboutdebugging: expose sw state from registration, notify listeners on statechange;r=bkelly https://hg.mozilla.org/integration/autoland/rev/edbcf59c1278 part4: aboutdebugging: display registering status for service workers;r=janx https://hg.mozilla.org/integration/autoland/rev/104c497c4c44 part5: create ServiceWorkerActor;r=janx

Logged the follow up Bug 1307105 in order to finish the implementation once the e10s implementation matches the non-e10s one.

https://hg.mozilla.org/mozilla-central/rev/0c3cbf93bd07 https://hg.mozilla.org/mozilla-central/rev/f2b131557c0a https://hg.mozilla.org/mozilla-central/rev/6f119f302f36 https://hg.mozilla.org/mozilla-central/rev/ff027e2ec0ea https://hg.mozilla.org/mozilla-central/rev/edbcf59c1278 https://hg.mozilla.org/mozilla-central/rev/104c497c4c44

I've added a bit on this. Please let me know if it's OK. https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Service_worker_state

Looks good, thanks Will (and sorry about the late answer :()! Updated slightly the description for the Running and Stopped states, hope it's ok. (ni? so you can verify my changes on MDN)

Thanks for looking Julian! Your changes look good to me :).