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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Missing push notifications #1696

Open
manuroe opened this issue Dec 19, 2017 · 8 comments
Open

Missing push notifications #1696

manuroe opened this issue Dec 19, 2017 · 8 comments

Comments

@manuroe
Copy link
Member

manuroe commented Dec 19, 2017 •
edited
Loading

There are several reports of people complaining Riot sometimes misses to notify them like in the Riot iOS room.

I reproduced a possible similar bug at https://github.com/matrix-org/riot-ios-rageshakes/issues/748 where the notification happened with a 3 min delay.
Looking at system logs, they show that the app tried to start at the time of the notification but it fired a 30s watchdog because it was infinitely looping at https://github.com/vector-im/riot-ios/blob/898353e47d0b70486885e322a2bd3620176e2383/Riot/AppDelegate.m#L315-L320

Note: this piece of code was for fixing spontaneous logout (#1653)
manuroe added a commit that referenced this issue Dec 19, 2017
@manuroe
Missing Push Notifications (#1696): Try to delay `[application isProt…
9f2fe58 
…ectedDataAvailable]`. At least, logging to file will be enabled
@manuroe
Copy link
Member Author

manuroe commented Dec 19, 2017

A way to reproduce missing notifications:

  • Kill the app
  • Enable Flight mode
  • From another account, send a message that should generate a notification to that device
  • Disable Flight mode

-> The device will never show a notification for the message.

Looking at OS logs, Riot fired the watchdog described above.

@manuroe
Copy link
Member Author

manuroe commented Dec 19, 2017

After testing, the bug described in the previous comment is fixed by 9f2fe58.

manuroe added a commit that referenced this issue Dec 19, 2017
@manuroe
Missing Push Notifications (#1696): Try to delay `[application isProt…
00aa51f 
…ectedDataAvailable]`. At least, logging to file will be enabled
@csett86
Copy link
Contributor

csett86 commented Dec 19, 2017

Further push notification crash logs:

Push-notification-crash-logs.zip

At least for the last one, 18.12. 13:32 I know I missed a push notification for a message sent 13:31 to me. The other logs look very similar anyway.

@manuroe manuroe closed this as completed Dec 22, 2017
@manuroe
Copy link
Member Author

manuroe commented Dec 22, 2017

Thanks @csett86 for your logs that confirm the fix I made is the right one.

@manuroe
Copy link
Member Author

manuroe commented Dec 27, 2017

Dave now receives a burst of notifications when he unlocks his phone in the morning.

@manuroe manuroe reopened this Dec 27, 2017
@manuroe
Copy link
Member Author

manuroe commented Dec 27, 2017

The solution seems to move the storage of MXAccount credentials from NSUserDefaults to a simple file with the appropriate read access right like NSFileProtectionCompleteUntilFirstUserAuthentication.

The reason of this move is explained at https://forums.developer.apple.com/thread/15685#45849:
"IMO the best way around this is to avoid NSUserDefaults for stuff that you rely on in code paths that can execute in the background."

@manuroe manuroe mentioned this issue Dec 27, 2017
Merged
manuroe added a commit that referenced this issue Dec 27, 2017
@manuroe
Missing Push Notifications (#1696): We do not need to wait for `[appl…
beb25cf 
…ication isProtectedDataAvailable]` anymore

Thanks to matrix-org/matrix-ios-kit#387
manuroe added a commit that referenced this issue Dec 27, 2017
@manuroe
Merge pull request #1708 from vector-im/MXKAccount_stored_in_file
1938312 
Missing Push Notifications (#1696): We do not need to wait for `[appl…
@manuroe manuroe mentioned this issue Dec 28, 2017
Closed
manuroe added a commit that referenced this issue Dec 29, 2017
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
f734096 
…pp fails to sync with its hs to get all data.

This fixes the case logged at
https://riot.im/bugreports/listing/2017-12-27/163238/console.2.log.gz
manuroe added a commit that referenced this issue Dec 29, 2017
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
536c8cc 
…pp fails to sync with its hs to get all data.

Build the string for the notification
manuroe added a commit that referenced this issue Dec 29, 2017
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
9e612dd 
…pp fails to sync with its hs to get all data.

Renaming
manuroe added a commit that referenced this issue Dec 29, 2017
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
750a6e9 
…pp fails to sync with its hs to get all data.

Show notif on /sync failure callback too
manuroe added a commit that referenced this issue Dec 29, 2017
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
97cb429 
…pp fails to sync with its hs to get all data.

wording
@manuroe manuroe mentioned this issue Dec 29, 2017
Merged
manuroe added a commit that referenced this issue Dec 29, 2017
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
8676024 
…pp fails to sync with its hs to get all data.

Fixed easy Giom's remarks
manuroe added a commit that referenced this issue Dec 29, 2017
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
0cdcff9 
…pp fails to sync with its hs to get all data.

Fix last Giom's remark: Make sure we do not display a "limited" notif for an event with already a "full" notif.
manuroe added a commit that referenced this issue Jan 3, 2018
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
30125ea 
…pp fails to sync with its hs to get all data.

BF: Return a string
manuroe added a commit that referenced this issue Jan 3, 2018
@manuroe
Missing Push Notifications (#1696): Show a notification even if the a…
6f15fb0 
…pp fails to sync with its hs to get all data.

BF: Return a string
(cherry picked from commit 30125ea)
@SB-68846
Copy link

Is there any updates on this issue? Most of our employees on iOS are experiencing it to varying degree. Android works fine.

Happy to help with additional info if needed.

@cwmke
Copy link

cwmke commented May 31, 2018

I have noticed that I don't get push notifications at all on iphone X but they work for the most part on an older ipad.

@r4dh4l r4dh4l mentioned this issue Nov 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@csett86 @cwmke @manuroe @SB-68846