Page MenuHomePhabricator

Fix MacOS app loosing device token in undeterministic manner
ClosedPublic

Authored by marcin on Nov 24 2023, 8:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 11:10 AM
Unknown Object (File)
Wed, Oct 30, 9:23 PM
Unknown Object (File)
Wed, Oct 30, 9:23 PM
Unknown Object (File)
Wed, Oct 30, 9:23 PM
Unknown Object (File)
Wed, Oct 30, 9:23 PM
Unknown Object (File)
Wed, Oct 30, 9:23 PM
Unknown Object (File)
Wed, Oct 30, 9:23 PM
Unknown Object (File)
Wed, Oct 30, 9:23 PM
Subscribers

Details

Summary

This differential solves bug that is well described in relevant Linear task:
https://linear.app/comm/issue/ENG-5890/macos-app-looses-devicetoken-in-undeterministic-manner

Test Plan

Build desktop app. Log in and log out multiple time. Ensure that notifications are always working. Additionally close and re-open the window (without closing app!). Ensure that no error appears in console and notifications work correctly.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Rebase to update commit message

desktop/src/main.js
263 ↗(On Diff #33595)

Can you just pass sendDeviceTokenToWebApp directly? If there's a Flow issue, then I suspect the type for ipcMain.on should be updated to accept any return value (mixed) instead of insisting on void

Update types and pass a function directly.

Looks like we'll need another desktop release after landing this one

michal requested changes to this revision.Nov 28 2023, 2:05 AM

There are a few issues with this (but they should be easily fixable):

  1. You are registering a callback on ipcMain everytime a window is loaded. This wouldn't be a problem in Windows but on macOS you can close the window without closing the app. Then if you would open a window again you would register another callback.
  2. The callbacks themselves use the main window variable which will stop being valid after the window is closed. So the previously registered callbacks will probably throw an error after re-opening the window.

We probably want to:

  1. Make the sendDeviceTokenToWebApp function a global function that uses the mainWindow variable which should always point at the current window (or be null)
  2. Call ipcMain.on only once (near the other calls to it)

Could you amend the test plan with closing and re-opening the window (on macOS, without closing the whole app) and checking if there are no errors?

This revision now requires changes to proceed.Nov 28 2023, 2:05 AM

Use mainWindow in sendDeviceTokenToWebApp.

This revision is now accepted and ready to land.Nov 28 2023, 6:38 AM