My idea to handle blob service query failure is that the notification containing blob_hash and encryption key will contain some very short text (to ensure it is deliverable via FCM) that will be displayed instead if we can't fetch the actual blob at the moment.

An alternative to handle network/blob service downtime would be to turn this query into a WorkRequest with exponential backoff retry policy: https://developer.android.com/guide/background/persistent/getting-started/define-work#retries_backoff

Why to make this call asynchronous?

On one hand FCM docs state that onMessageReceived is executed on separate thread - different than main thread: https://firebase.google.com/docs/reference/android/com/google/firebase/messaging/FirebaseMessagingService. On the other thread, I had done some search through FCM code and found out it is always the same thread - subsequent onMessageReceived calls are executed sequentially on dedicated background thread. Therefore I concluded that we shouldn't block displaying small notifications immediately on some large notification being downloaded.

To avoid background execution abuse I set time limit on each blob request to the value mentioned in the docs: https://firebase.google.com/docs/cloud-messaging/android/receive#backgrounded

But I think using WorkManager and exponential backoff policy for this case a bit of over-engineering.

• According to the docs, 20s is the maximum limit, but can be shorter depending on OS and other random stuff. Maybe we can decrease it to let's say 15s?
• Other than that, can we extract this number as a private static final constant? Along with this comment

My idea to handle blob service query failure is that the notification containing blob_hash and encryption key will contain some very short text (to ensure it is deliverable via FCM) that will be displayed instead if we can't fetch the actual blob at the moment.

This idea makes sense - I think we want notification to be displayed even if it's short instead of delaying its delivery until request succeeds (which may never happen).

As a user, I'd be more frustrated when received notif too late (or totally miss it) than if it was brief/vague but delivered quickly.

Makes sense to me

Do we have to create this object each time this function is called?

You are right @tomek . This if statement should be removed and badgeOnly always set to '0'. Real badge only notifs are handled in updateBadgeCount function in this file.

OkHttp library methods are throwing checked exception so throws clause is necessary.

This is probably out of the scope of this differential.

Are they declaring to throw Exception or a subtype? If only some subtypes, we can make this method declare throwing only the subtypes and not the Exception.

OkHttp methods that we use here [[ https://square.github.io/okhttp/#post-to-a-server | could throw IOException ]]. Out getAuthToken method could throw JSONException. Theoretically we can experience OutOfMemory error since we are downloading potentially large blob into memory. That said if we want to be more specific we would have to bring back the original code and remove the Exception.

We actually introduced a bug here. From my latest understanding updateBadgeCount handles user-initiated badge updates (marking thread manually as unread/read). The code above was supposed to handle the case of visual notification sent to the thread that user might have potentially muted. The badgeOnly here was based on thread subcription.

Will be fixed when we land: https://phab.comm.dev/D12539

Can you clarify what product impact this bug has? How does it affect users?

I spoke to @marcin about this in our 1:1 today. The bug is that when the keyserver should send a badge-only notif to an Android device, it accidentally sets badgeOnly to 0 and includes fields not relevant for badgeOnly notifs (the text of the notif). This results in a visual notif for the user

Since this is a user-facing issue, I've created an urgent task to track: ENG-8620