Fix/double notif on iOS Web Push

[rgomezp]

Description

1 Line Summary

Prevents double notifications to iOS Web Push by preventing a race condition that results in the app state being the same for both function invocations of checkAndTriggerSubscriptionChanged

Details

In JavaScript, which is a single-threaded, event-driven language, the concept of a Mutex lock doesn't exactly apply in the same way as in multi-threaded languages like Java or C++. JavaScript code is executed in a single thread, which means it can only do one thing at a time, so you don't generally have to worry about multiple threads accessing a shared resource simultaneously.

However, JavaScript can still perform asynchronous operations, such as AJAX calls, timeouts, and promises, which could potentially lead to race conditions. A race condition is a situation where the behavior of your code could vary depending on the precise timing of these asynchronous operations.

Race condition

There is a race condition in the function checkAndTriggerSubscriptionChanged.

This function is called from two key places which lead it to be called twice in both desktop and mobile environments:

• SubscriptionHelper: when the browser first subscribes
• EventHelper.onNotificationPermissionChange: when the event NATIVE_PROMPT_PERMISSIONCHANGED is emitted.

In the second call, because the call is triggered from the event handler, we cannot await on it to ensure sequential execution of the calls. This leads to the race condition that arises when the variable didStateChange in the function is true for both calls (it should be true only for the first call).

For some reason, the race condition only creates a problem on iOS -- presumably due to the event firing at a different time than on desktop.

Solution

We introduce a mutex lock to the EventHelper class which we use in the checkAndTriggerSubscriptionChanged function to ensure sequential execution.

Systems Affected

• WebSDK
• Backend
• Dashboard

Validation

Tests

Tested manually on an iOS device. Only one notification is received.

Info

We will no longer be adding test coverage on pre-user-model code. All automated tests pass.

Checklist

• All the automated tests pass or I explained why that is not possible
• I have personally tested this on my machine or explained why that is not possible
• I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

• Don't use default export
• New interfaces are in model files

Functions:

• Don't use default export
• All function signatures have return types
• Helpers should not access any data but rather be given the data to operate on.

Typescript:

• No Typescript warnings
• Avoid silencing null/undefined warnings with the exclamation point

Other:

• Iteration: refrain from using elem of array syntax. Prefer forEach or use map
• Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Info

Checklist

• I have included screenshots/recordings of the intended results or explained why they are not needed

---

Related Tickets

---

---

This change is 

[Koji23]

Just curious, why do we still need to call checkAndTriggerSubscriptionChanged when the browser first subscribes if this is already happening again when the NATIVE_PROMPT_PERMISSIONCHANGED event is emitted?

[rgomezp]

Just curious, why do we still need to call checkAndTriggerSubscriptionChanged when the browser first subscribes if this is already happening again when the NATIVE_PROMPT_PERMISSIONCHANGED event is emitted?

Great question. I do believe it is redundant. That said, removing it from the SubscriptionHelper breaks way too many tests and considering we are winding down development on the pre-user-model SDK any sort of fix should happen on the user-model/v1 branch.

[Koji23]

Gotcha I'm all for getting this out then as a quick fix

What would you suggest our plans should be for the user-model/v1 branch? Will it be able to inherit the solution you've provided here or will it need to be fixed again there as well?

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @marshall-onesignal)

[marshall-onesignal]

I'm not very familiar with javascript or typescript runtimes. I agree that we seem to have a race condition, as we could have different outcomes in how this code behaves based on the timing of the async callbacks relative to each other. Maybe it's idiomatic to express this the way you have here, but do we actually need a "mutex" to fix this problem? If there is only one thread of control executing our code at a time, could we instead store some additional state to keep track of whether checkAndTriggerSubscriptionChanged has been called, and rely on that? Would that be simpler/tidier than using a mutex?

[rgomezp]

Gotcha I'm all for getting this out then as a quick fix

What would you suggest our plans should be for the user-model/v1 branch? Will it be able to inherit the solution you've provided here or will it need to be fixed again there as well?

I think on the user-model/v1 branch we would want to come up with a better solution (i.e. clean up the redundant code).

I'm not very familiar with javascript or typescript runtimes. I agree that we seem to have a race condition, as we could have different outcomes in how this code behaves based on the timing of the async callbacks relative to each other. Maybe it's idiomatic to express this the way you have here, but do we actually need a "mutex" to fix this problem? If there is only one thread of control executing our code at a time, could we instead store some additional state to keep track of whether checkAndTriggerSubscriptionChanged has been called, and rely on that? Would that be simpler/tidier than using a mutex?

The mutex might seem very complicated but the mechanism is quite simple. See this link for a better view of the changes (hidden whitespace).

I did think about alternatives like tracking whether the checkAndTriggerSubscriptionChanged has been called or not but this raises the question: when do we reset this value to the non-called state? e.g: if the user subscribes, unsubscribes, then subscribes again, we would need to be careful about how this is handled.

Another alternative would be to assign some sort of idempotent key in the welcome notification payload to prevent duplicates. The key could be a hash of the notification message + the time to the nearest minute. This would de-dupe the notification but is a bit hacky.

[marshall-onesignal]

src/helpers/EventHelper.ts line 30 at r2 (raw file):

    }

    EventHelper._mutexLocked = true;

Looks like this would have a problem if the mutex is locked and two other "threads" tried to acquire it?

Both would await on the current promise, then both would assume after that wait, that they then held the mutex, one would set _mutexPromise = <whatever> and the other would then overwrite that, but both would be running?

Maybe not a problem here as long as we know that's the race is between two callers, but seems a little bit prone to problems if the codebase changes in certain ways?

[rgomezp]

src/helpers/EventHelper.ts line 30 at r2 (raw file):

    }

    EventHelper._mutexLocked = true;

Looks like this would have a problem if the mutex is locked and two other "threads" tried to acquire it?

Both would await on the current promise, then both would assume after that wait, that they then held the mutex, one would set _mutexPromise = <whatever> and the other would then overwrite that, but both would be running?

Maybe not a problem here as long as we know that's the race is between two callers, but seems a little bit prone to problems if the codebase changes in certain ways?

Ah, good catch. I suppose it's possible that there could be more callers in theory. But given there's only two at the moment and we don't plan to invest much more into this codebase, it should be safe.

For context, iOS push support is the last big feature we're investing into here. Everything else should just be bug fixes. Even A2HS will only be added to the user-model version to incentivize migration.

[marshall-onesignal]

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion

[marshall-onesignal]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion

[Koji23]

Sounds good to me, can you add a ticket somewhere clean up the redundant code on the user-model/v1 branch so we don't forget? Not sure if there is already a user model sdk project that makes sense for this or as a last resort and if it's our responsibility then you can throw it in the notif squad tech investments

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @marshall-onesignal)