Skip to content

Conversation

@joeygrover
Copy link
Member

Fixes #1351

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested this PR against a TDK and verified behavior (if applicable, if not applicable, explain why below).
  • I have tested Android

Tests

This issue is tricky to test since we have not found anyone who can actually reproduce the issue. Therefore we will have to simulate the issue and test around it. There are essentially two tests we can run: one to ensure the exception handler is properly triggered, and two to make sure the flag is properly saved and retrieved.

Testing the exception handler:

Comment out line ~1514 that is notificationManager.createNotificationChannel(notificationChannel); and add a log into the exception handler. This will prevent the channel from being actually registered with the system and then the exception handler should trigger. Simply turn on the bluetooth adapter and connect to the module. The service will start up and the exception will be thrown within a few seconds.

Testing the flag is set

Change the previous line to

if(getBooleanPref(KEY_AVOID_NOTIFICATION_CHANNEL_DELETE,false)){
	notificationManager.createNotificationChannel(notificationChannel);
    Log.d(TAG, "Created channel");
}else{
	Log.d(TAG, "Did not create channel");
}

On first pass this will throw the exception. The router service will shutdown. Turn off bluetooth or disconnect from the device. Then turn bluetooth back on and reconnect to the device. This time, since the flag was set, the channel will be created.

Summary

  • Added an exception handler for the runtime exception caused by some poor Android implementations around the notification channel. Samsung devices don't seem to properly handle the case where we delete the notification channel after the service exits the foreground. Even though we create the channel on every foreground entrance, the OS doesn't honor that and the crashes. Therefore the handler was added and a flag will be set to prevent the channel deletion in the next run of the router service.
  • Increased the

Changelog

Bug Fixes
  • App will no longer crash. Dynamic behavior is in place to still allow SDL to function.

CLA

@joeygrover joeygrover added android Relating to the android part of the library router-service Relating to the router service labels May 20, 2020
@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #1360 into master will decrease coverage by 0.04%.
The diff coverage is 3.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1360      +/-   ##
============================================
- Coverage     47.67%   47.62%   -0.05%     
  Complexity     4432     4432              
============================================
  Files           489      489              
  Lines         23845    23872      +27     
  Branches       2753     2759       +6     
============================================
+ Hits          11369    11370       +1     
- Misses        11775    11801      +26     
  Partials        701      701              
Impacted Files Coverage Δ Complexity Δ
...om/smartdevicelink/transport/SdlRouterService.java 11.53% <3.57%> (-0.12%) 20.00 <0.00> (ø)


public void startUpSequence(){

setRouterServiceExceptionHandler();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should call this method first thing in onCreate() because currently, the service could enter foreground before adding the exception handler

super.onCreate();
//This must be done regardless of if this service shuts down or not
//Add this first to avoid the runtime exceptions for the entire lifecycle of the service
setRouterServiceExceptionHandler();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing seems to be strange here.

SdlRouterService.this.setSdlRouterServicePrefs(KEY_AVOID_NOTIFICATION_CHANNEL_DELETE, true);
try{
SdlRouterService.this.setSdlRouterServicePrefs(KEY_AVOID_NOTIFICATION_CHANNEL_DELETE, true);
}catch (Exception exception){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing seems to be strange here as well.

@bilal-alsharifi bilal-alsharifi merged commit 9946777 into master May 26, 2020
@bilal-alsharifi bilal-alsharifi deleted the hotfix/issue_1351 branch May 26, 2020 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

android Relating to the android part of the library router-service Relating to the router service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Samsung devices not properly managing notification channels

3 participants