Skip to content

Commit

Permalink
fix race condition getting wrong sw registration
Browse files Browse the repository at this point in the history
If you attempt to get the service worker registration immediately after
registering it the browser can give you a different registration than
exepected. This is due to the fact the new registeration may not be
ready yet and there migth be sw in scope at a higher level.

The problem noted above cause the OneSignal SDK to register for a push
token on the wrong service worker. This in turn causes the SDK to
attempt to create a user with a push susbcription but without a push
token, which results in a 400 error on POST /users.

To fix, instead of calling navigator.serviceWorker.getRegistration
we are now simply using the ServiceWorkerRegistration give to us by
navigator.serviceWorker.register to avoid this race condition completely
  • Loading branch information
jkasten2 committed Nov 16, 2023
1 parent 3b54786 commit 7930f20
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/shared/helpers/SubscriptionHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export default class SubscriptionHelper {
await PermissionUtils.triggerNotificationPermissionChanged();
await EventHelper.checkAndTriggerSubscriptionChanged();
} catch (e) {
Log.info(e);
Log.error(e);
}
break;
case WindowEnvironmentKind.OneSignalSubscriptionPopup: {
Expand Down
32 changes: 25 additions & 7 deletions src/shared/managers/ServiceWorkerManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,8 @@ export class ServiceWorkerManager {
});
}

// Returns true if a the OneSignal service worker can't be installed
// or is already installed and doesn't need updating.
private async shouldInstallWorker(): Promise<boolean> {
// 1. Does the browser support ServiceWorkers?
if (!Environment.supportsServiceWorkers()) return false;
Expand Down Expand Up @@ -486,9 +488,11 @@ export class ServiceWorkerManager {
* additional query parameters, but this must then stay consistent.
*/

public async installWorker() {
public async installWorker(): Promise<
ServiceWorkerRegistration | undefined | null
> {
if (!(await this.shouldInstallWorker())) {
return;
return this.getRegistration();
}

Log.info('Installing worker...');
Expand All @@ -512,8 +516,12 @@ export class ServiceWorkerManager {
Log.info(
`[Service Worker Installation] Installing service worker ${workerHref} ${scope}.`,
);

let registration: ServiceWorkerRegistration;
try {
await navigator.serviceWorker.register(workerHref, { scope });
registration = await navigator.serviceWorker.register(workerHref, {
scope,
});
} catch (error) {
Log.error(
`[Service Worker Installation] Installing service worker failed ${error}`,
Expand All @@ -527,14 +535,21 @@ export class ServiceWorkerManager {
throw error;
}

await this.fallbackToUserModelBetaWorker();
registration = await this.fallbackToUserModelBetaWorker();
}
Log.debug(`[Service Worker Installation] Service worker installed.`);
Log.debug(
`[Service Worker Installation] Service worker installed. Waiting for activation`,
);

await ServiceWorkerUtilHelper.waitForServiceWorkerToActive(registration);

Log.debug(`[Service Worker Installation] Service worker active`);

await this.establishServiceWorkerChannel();
return registration;
}

async fallbackToUserModelBetaWorker() {
async fallbackToUserModelBetaWorker(): Promise<ServiceWorkerRegistration> {
const BETA_WORKER_NAME = 'OneSignalSDK.sw.js';

const configWithBetaWorkerName: ServiceWorkerManagerConfig = {
Expand All @@ -557,7 +572,9 @@ export class ServiceWorkerManager {
);

try {
await navigator.serviceWorker.register(workerHref, { scope });
const registration = await navigator.serviceWorker.register(workerHref, {
scope,
});

const DEPRECATION_ERROR = `
[Service Worker Installation] Successfully installed v16 Beta Worker.
Expand All @@ -568,6 +585,7 @@ export class ServiceWorkerManager {
`;

Log.error(DEPRECATION_ERROR);
return registration;
} catch (error) {
const response = await fetch(workerHref);
if (response.status === 403 || response.status === 404) {
Expand Down
7 changes: 3 additions & 4 deletions src/shared/managers/SubscriptionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -487,8 +487,10 @@ export class SubscriptionManager {
}

/* Now that permissions have been granted, install the service worker */
let workerRegistration: ServiceWorkerRegistration | undefined | null;
try {
await this.context.serviceWorkerManager.installWorker();
workerRegistration =
await this.context.serviceWorkerManager.installWorker();
} catch (err) {
if (err instanceof ServiceWorkerRegistrationError) {
if (err.status === 403) {
Expand All @@ -506,9 +508,6 @@ export class SubscriptionManager {
throw err;
}

Log.debug('[Subscription Manager] Getting OneSignal service Worker...');
const workerRegistration =
await this.context.serviceWorkerManager.getRegistration();
if (!workerRegistration) {
throw new Error('OneSignal service worker not found!');
}
Expand Down
25 changes: 25 additions & 0 deletions src/sw/helpers/ServiceWorkerUtilHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,29 @@ export default class ServiceWorkerUtilHelper {
}
return availableWorker;
}

// Allows waiting for the service worker registration to become active.
// Some APIs, like registration.pushManager.subscribe, required it be active
// otherwise it throws.
static waitForServiceWorkerToActive(
registration: ServiceWorkerRegistration,
): Promise<void> {
return new Promise((resolver) => {
// IMPORTANT: checking non-active instances first,
// otherwise the 'statechange' event could be missed.
const inactiveWorker = registration.installing || registration.waiting;
if (inactiveWorker) {
inactiveWorker.addEventListener('statechange', () => {
Log.debug(
'OneSignal Service Worker state changed:',
inactiveWorker.state,
);
if (registration.active) {
resolver();
}
});
}
if (registration.active) resolver();
});
}
}

0 comments on commit 7930f20

Please sign in to comment.