Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Service worker requestSubscription() freezes and doesn't show native popup if serviceworker.controller is null #29067

Open
Serginho opened this issue Mar 2, 2019 · 14 comments

Comments

@Serginho
Copy link
Contributor

@Serginho Serginho commented Mar 2, 2019

馃悶 bug report

Affected Package

@angular/service-worker

Is this a regression?

No

Description

The problem is here

const controllerChangeEvents = fromEvent(serviceWorker, 'controllerchange');
const controllerChanges = controllerChangeEvents.pipe(map(() => serviceWorker.controller));
const currentController = defer(() => of (serviceWorker.controller));
const controllerWithChanges = concat(currentController, controllerChanges);
this.worker = controllerWithChanges.pipe(filter<ServiceWorker>(c => !!c));
this.registration = <Observable<ServiceWorkerRegistration>>(
this.worker.pipe(switchMap(() => serviceWorker.getRegistration())));

When controllerchange is triggered, the code access to serviceworker.controller and filters it if is null. After that, the registration is created from the previous pipes.

What happens if the serviceworker.controller is null in all controllerchange events? (At the moment you are thinking that couldn't happen, but yes 馃帀it happens!!!馃帀.
Then if the app is started with serviceworker.controller = null and requestSubscription is called a few minutes later, the promise won't never be resolved.

return this.pushManager.pipe(switchMap(pm => pm.subscribe(pushOptions)), take(1))
.toPromise()
.then(sub => {
this.subscriptionChanges.next(sub);
return sub;
});
}

Possible solution
Merging registrationobservable with serviceworker.readybut I'm not sure if breaks something.

this.registration = <Observable<ServiceWorkerRegistration>>(
this.worker.pipe(switchMap(() => serviceWorker.getRegistration())));

馃敩 Minimal Reproduction

The problem reproduction is explained in the previous section. But if you want to get crazy to Chrome to test the bug, you can do the following:

You can test this on Chrome 72 Windows, the steps are not going to work on Chrome Mac. But I have logs that prove the bug happens in other Chrome version and platforms, maybe, doing different steps, but the state is the same, serviceworker.controller is null.

Steps Chrome Windows.

  1. Create a app with a checkbox or switch button to activate/descativate notifications.
  2. Check the switch in your app will call requestSubscription()
  3. When the popup is shown, press Block.
  4. Go to de addressbar, click in the lock 馃敀. Change the notifications state to Allow. Chrome will say you that the page should be reloaded.
  5. Reload the page.
  6. If you call requestSubscription()again, you won't get the popup. Or if you write in the console serviceworker.controller you will get null.

Here is an example image. The app is started, serviceworker.controller is null, but I can get the service worker registration and the push subscription, so serviceworker.controller is not needed to get it. (Keys are hidden in the image).
captura de pantalla 2019-03-01 a las 19 49 21

馃實 Your Environment

Angular Version:
Tested in angular 7.1.7 but I think all version are affected.

Anything else relevant?

I use this code with a timeout before I run into the issue to avoid freezing the function. I'm getting logs Can't subscribe: Timeout ocurred from Chrome Windows but specially lots of entries for Chrome android.

      this.swPush.requestSubscription({serverPublicKey: this.VAPID_PUBLIC_KEY}))
      .pipe(
        timeout(8000),
        tap({
          next: () => this.measuringDao.sendBackendEvent('activated_notifications').subscribe(),
          error: err => this.logger.warn('Can\'t subscribe: ' + err.toString(), err)
        }),
        switchMap(sub => this.notificationsDao.registerPushSubscriber(sub))
      );
@ngbot ngbot bot added this to the needsTriage milestone Mar 2, 2019
@gkalpak

This comment has been minimized.

Copy link
Member

@gkalpak gkalpak commented Mar 3, 2019

Interesting. Generally, according to the spec, navigator.serviceWorker.controller does always reflect the worker controlling the page (which is what we want). One strange corner case seems to be that, if a client is force refreshed (Shift + reflesh), then the activated SW will not take it over once activated (even with clients.claim()) and thus navigator.serviceWorker.controller will indeed remain null (until the page is reloaded).

It seems strange that clients.claim() would not be respected (I can't find anything relevant in the spec) 馃槙
That was incorrect. clients.claim() is respected.

In any case, this does not explain the behavior (since there was no force refreshing involved) and I could not reproduce it (with Chrome 72 on Windows).

I wouldn't be opposed to taking navigator.serviceWorker.ready or navigator.serviceWorker.getRegistration() into account, but I would like to understand the error first. @Serginho, could you create a minimal reproduction (e.g. a repo I could checkout and reproduce the issue)?

@Serginho

This comment has been minimized.

Copy link
Contributor Author

@Serginho Serginho commented Mar 3, 2019

We can't discard that could be a Chrome bug. I'll try to give you an example an how to reproduce it, but... The important thing is:

Even if the user does a forced reload, the code should works anyway.

I use a forked version of the service worker (don't worry if you are thinking that could be a problem of my forked code, I've only implemented the pushsubscriptionchange event)... Do you think this could be a hotfix?

this.registration = <Observable<ServiceWorkerRegistration>>( 
     merge(this.worker, fromEvent(serviceWorker, 'ready')).pipe(switchMap(() => serviceWorker.getRegistration()))); 

We are losing lots of requestSubscriptions. Like I said, this code logs we the popup isn't shown.

      this.swPush.requestSubscription({serverPublicKey: this.VAPID_PUBLIC_KEY}))
      .pipe(
        timeout(8000),
        tap({
          next: () => this.measuringDao.sendBackendEvent('activated_notifications').subscribe(),
          error: err => this.logger.warn('Can\'t subscribe: ' + err.toString(), err)
        }),
        switchMap(sub => this.notificationsDao.registerPushSubscriber(sub))
      );

Noticed that we are NOT showing the popup in the main page when the app boots. We have a page explaining why you should activate notifications, and if you click our button that says: "Activate notifications" then we will call requestSubscription.

These are the logs in the last 30 minutes. As you can see, This is not the tipical weird bug that happens by chance.

March 3rd 2019, 20:02:02.514 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (72.0.3626.105) running over Android (5.1.1)
March 3rd 2019, 19:54:03.964 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (71.0.3578.99) running over Android (8.1.0)
March 3rd 2019, 19:53:18.459 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (72.0.3626.105) running over Android (7.1.1)
March 3rd 2019, 19:53:08.646 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (71.0.3578.99) running over Android (4.4.2)
March 3rd 2019, 19:42:17.368 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (72.0.3626.105) running over Android (9)
March 3rd 2019, 19:41:16.824 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (72.0.3626.105) running over Android (8.1.0)
March 3rd 2019, 19:40:31.476 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (72.0.3626.105) running over Android (6.0.1)
March 3rd 2019, 19:39:33.304 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (72.0.3626.105) running over Android (6.0)
March 3rd 2019, 19:38:26.182 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (72.0.3626.105) running over Android (7.0)
March 3rd 2019, 19:36:15.625 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (70.0.3538.110) running over Android (6.0.1)
March 3rd 2019, 19:35:21.772 | Can't subscribe: TimeoutError: Timeout has occurred | Chrome (70.0.3538.110) running over Android (6.0.1)

@gkalpak

This comment has been minimized.

Copy link
Member

@gkalpak gkalpak commented Mar 3, 2019

Even if the user does a forced reload, the code should works anyway.

Well, it is debatable what "works" should mean. If the page is not controlled by a SW (as seems to be the case after a force refresh), then should it be possible to request a subscription? Technically, it is possible to use the matching SW registration to do that, but the page would not be notified of notification clicks, for example (since the SW in not yet controlling the page).
That could be equally confusing imo.

But, as you said, this could be a browser issue. It is hard to decide on the right "fix", without understanding the root of the problem (hence I want to reproduce it first).

Do you think this could be a hotfix?

this.registration = <Observable<ServiceWorkerRegistration>>(
   merge(this.worker, fromEvent(serviceWorker, 'ready')).pipe(switchMap(() => serviceWorker.getRegistration())));

I don't think navigator.serviceWorker has a ready event, thus fromEvent(serviceWorker, 'ready') wouldn't do anything.
I think something like the following should do:

this.registration = <Observable<ServiceWorkerRegistration>>(
    merge(
        from(serviceWorker.ready),
        this.worker.pipe(switchMap(() => serviceWorker.getRegistration())),
    ));

But notice that other APIs depending on this.worker (e.g. checking/activating updates) might not work.

@gkalpak

This comment has been minimized.

Copy link
Member

@gkalpak gkalpak commented Mar 3, 2019

Interestingly, after a force refresh, the page is in a weird, intermediate state 馃榿
It can interact with the SW (e.g. via (await navigator.serviceWorker.getRegistration()).postMessage(...)) and the SW can respond to messages, but the page is not controlled by the SW.

We could probably detect this state and force the page to be controlled by the SW by trigerring clients.claim(), but there are certain drawbacks:

  1. It has the side effect of claiming all clients. (AFAICT, there is no way for the SW to claim a specific client only.)
  2. It breaks the purpose of force refresh, which seems to be running the page without a SW.

One could argue that (1) would generally not be a problem, since all pages would be claimed. And, also, (2) may not make sense in the context of a SPA, where force refresh should be expected to run without the SW for the initial load, but after that it would be fine for the Sw to take over.

Of course, this is not something we should do lightly, but maybe worth considering.

@Serginho

This comment has been minimized.

Copy link
Contributor Author

@Serginho Serginho commented Mar 3, 2019

I don't think navigator.serviceWorker has a ready event, thus fromEvent(serviceWorker, 'ready') wouldn't do anything.

Yes, navigator.serviceWorker.ready is available.

  1. It breaks the purpose of force refresh, which seems to be running the page without a SW.

Hmm... Could it be when I change the notifications state from Block to Allow and Chrome shows an alert to reload the page, this reload will be a forced reload?. But just look at all android devices...

Another thing we do and didn't mention in the previous comments is to call requestSubscription when the user enters on the site only if the notifications are enabled, this behaviour is cause Chrome don't implement pushsubscriptionchange event in order to minimize lost subscriptions. For this reason I know if the user has notifications enabled, and we request the subscription, the timeout could happen. 馃く

I will try to give you a reproduction example, or at least a video showing the problem.

@Serginho

This comment has been minimized.

Copy link
Contributor Author

@Serginho Serginho commented Mar 12, 2019

@gkalpak This is crazy. I got it!
I could reproduce it by chance. It's a Chrome bug. When chrome gets in a weird state and subscribe() freezes and doesn't show the prompt and the promise won't never resolved.

See that image:
bug

subscribe() don't print any result.

getSubscription() prints null

permissionStatus() prints prompt

Notification.permission prints default

To confirm the bug I debugged SwPush. When this happens if you put a breakpoint in this line pm is defined.

return this.pushManager.pipe(switchMap(pm => pm.subscribe(pushOptions)), take(1))

But never reaches this line:
this.subscriptionChanges.next(sub);

@gkalpak

This comment has been minimized.

Copy link
Member

@gkalpak gkalpak commented Mar 12, 2019

Nice, @Serginho! Have you checked whether .subscribe() rejects?

@Serginho

This comment has been minimized.

Copy link
Contributor Author

@Serginho Serginho commented Mar 12, 2019

@gkalpak
Captura de pantalla 2019-03-12 a las 17 56 55

Update, no rejects. It seems a UI problem. You call subscribe(), the prompt doesn't show, and the code if waiting you to answer something in the prompt.

@gkalpak

This comment has been minimized.

Copy link
Member

@gkalpak gkalpak commented Mar 12, 2019

Nice sleuthing. Could you report it to Chromium (if not already reported)?

@Serginho

This comment has been minimized.

Copy link
Contributor Author

@Serginho Serginho commented Mar 12, 2019

That's I'm trying to do. That is the last straw! I getting a 400 Bad request when I submit the issue to Chromium issue tracker, all fields are filled with no errors 馃槶. I'll link the issue here.

@Moeriki

This comment has been minimized.

Copy link

@Moeriki Moeriki commented Mar 13, 2019

Not sure if same issue. I make the same call

await this.swPush.requestSubscription({ serverPublicKey })
  • I get no response, error or otherwise.
  • navigator.serviceWorker.controller is null.
  • Reproducible in Chrome and Firefox.

Can I help investigate? Should I create a separate issue?

@Serginho

This comment has been minimized.

Copy link
Contributor Author

@Serginho Serginho commented Mar 13, 2019

You are doing a forced reload (Shift + F5). When you do this, the tab isn't going to be controlled by service worker.

No issue if you see controller = null with forced reload. I've been investigating this for days and I only got controller null when I did a forced reload.

@ElectroluxV2

This comment has been minimized.

Copy link

@ElectroluxV2 ElectroluxV2 commented Dec 19, 2019

Not sure if same issue. I make the same call

await this.swPush.requestSubscription({ serverPublicKey })
  • I get no response, error or otherwise.
  • navigator.serviceWorker.controller is null.
  • Reproducible in Chrome and Firefox.

Can I help investigate? Should I create a separate issue?

I have same issue except that for me it works in Chrome unless user already gave permission to show notification... (works only if permission state is to ask for permission) and doesn't work at all in latest Brave.

@omatrot

This comment has been minimized.

Copy link

@omatrot omatrot commented Jan 3, 2020

I have the same issue with Chrome 79.0.3945.88 / Edge 18363 on Windows when using Angular 8.X.
This seems to be an angular issue because with another web site that is using Angular 7.2 it works !
Unsupported on Firefox/Edge Beta (Chromium)

EDIT:
Not an angular issue, it still works an a 7.2 sample upgraded to 8.2.14
Hi @Serginho. Any news on this issue ?

EDIT2:
This is an angular-cli issue.
Solved the problem with the help of these comments 1 and 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.