[Accepted] SDL 0345 - Android 12 Issues

Question

[Accepted] SDL 0345 - Android 12 Issues

theresalech opened this issue 3 years ago · 26 comments

Hello SDL community,

The review of the revised proposal "SDL 0345 - Android 12 Issues" begins now and runs through February 22, 2022. The previous review of this proposal took place December 8, 2021 - February 8, 2022. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0345-android-12-issues.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#1181

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

Is the problem being addressed significant enough to warrant a change to SDL?
Does this proposal fit well with the feel and direction of SDL?
If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
theresa@livio.io

Answer 1 · 2021-12-13T10:20:59.000Z

I have a couple of questions and/or comments:

In Library change of AndroidTools.java, the code snippet shows

    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S && intent.getBooleanExtra(TransportConstants.PENDING_BOOLEAN_EXTRA, false)) {
        Intent pending = new Intent();
        PendingIntent pendingIntent = PendingIntent.getForegroundService(context, (int) System.currentTimeMillis(), pending, PendingIntent.FLAG_MUTABLE | Intent.FILL_IN_COMPONENT);
        intent.putExtra(TransportConstants.PENDING_INTENT_EXTRA, pendingIntent);
    }

Looking at SdlRouterService.java, PENDING_BOOLEAN_EXTRA is simply set TRUE if Build.VERSION.SDK_INT is greater than or equal to 31. So I am wondering if PENDING_BOOLEAN_EXTRA is indeed required. More specifically, doesn't version check for Android 12 suffice the needs?

In Bluetooth Runtime Permissions section, the proposal says SDL apps require both BLUETOOTH_CONNECT and BLUETOOTH_SCAN.

However, I don't think BLUETOOTH_SCAN is required, because most SDL apps specify BLUETOOTH pairing is prerequisite. After user establishes BLUETOOTH pairing, BLUETOOTH_CONNECT permission suffices our needs to connect with head unit.

BLUETOOTH_SCAN is specified in many sections (Motivation, Proposed solution, Impact on existing code, etc), and they need to be updated as well.

Answer 2 · 2021-12-13T16:09:03.000Z

Hello @shiniwat,

You are correct, this logic can be simplified and the check for Android 12 should be enough for this proposed solution.
The BLUETOOTH_SCAN permission is necessary for BluetoothAdapter.cancelDiscovery(). Which is being used in MultiplexBluetoothTransport.java. However we could check for the permission and fail out of that method if the permission is not granted. cancelDiscovery() is only being used in the event we act as the client instead of listening as the server. As this is not a production use case we can make this change remove BLUETOOTH_SCAN as a required permission.

Answer 3 · 2021-12-13T23:42:33.000Z

Hi @RHenigan,

Regarding 2, MuletiplexBluetoothTransport.java has the code as you mentioned, but I don't think it is used in sdl_java_suite library. A ConnectThread in MultiplexBluetoothTransport is invoked only if SdlRouterService gets ROUTER_REQUEST_BT_CLIENT_CONNECT message, but I believe nobody uses that message.
Maybe SdlRouterService needs some cleanup?

Answer 4 · 2021-12-14T15:01:49.000Z

Hello @shiniwat,

You are correct, we do not use the ROUTER_REQUEST_BT_CLIENT_CONNECT in a production setting. We can wrap this code in a permission check to avoid requiring the BLUETOOTH_SCAN permission or we can remove this code from the SdlRouterService

Answer 5 · 2021-12-14T21:49:07.000Z

Hi @RHenigan,
Sounds good. At least, we don't have to check BLUETOOTH_SCAN permission when starting SdlRouterService (at initCheck()). Thanks,

Answer 6 · 2021-12-14T21:53:25.000Z

Hello @shiniwat
Just to be clear we will still need to check the BLUETOOTH_CONNECT permission.

Answer 7 · 2021-12-15T14:12:02.000Z

The Steering Committee voted to keep this proposal in review until our next meeting in order to allow for more discussion on the review issue.

Answer 8 · 2022-01-11T10:04:07.000Z

Hi @RHenigan,
I went through the proposal again, and basically do not find issues other than the already discussed BLUETOOTH_SCAN permission issue. As you mentioned, SdlRouterService still requires BLUETOOTH_CONNECT permission anyway.

It's still tricky if user connects USB with head unit while the app does not have BLUETOOTH_CONNECT permission. In particular, because requesting permission itself is up to application (not the responsibility of the library), I do think SDL library can minimize the permission check and allows SdlRouterService to run with MultiplexUsbTransport only. However, adding notification is still reasonable solution for most users, and Alternatives considered section mentions about that.

Answer 9 · 2022-01-11T21:04:48.000Z

I apologize for my tremendously last minute review.

3. Now that the SdlRouterService is now essentially starting the apps' SdlService do we need to update the exception catcher in the router service to catch the exception that happens if the aforementioned SdlService doesn't enter the foreground in time? Will the router service still run after hitting this exception even if caught?

4. It should probably be a little more called out that using the PendingIntent method of starting their SdlService will require the developers to make that service exported.

5. If users don't choose to make their services exported what kind of alternatives can we allow for them or suggest? We could tell them they could start their own activity maybe or that at least warn them their app will not be seen on the IVI unless a user specifically starts it up.

6. Why are we adding the PendingIntent to the AndroidTools.sendExplictBroadcast method? We only need to include the PI when there is a transport connection right? We should only include it for that use case which would then require the changes to be in the SdlRouterService not AndroidTools utility method.

7.

... and therefore will not know to start its own SdlRouterService when the device connects over Bluetooth.

Should be

and therefore will not know to start its own SdlRouterService when the device connects over Bluetooth in the event it has been chosen to host it.

8. I believe in the SdlBroadcastReceiver modifications we need to check to make sure there are no older versions of the router services installed right? Otherwise, because we're now doing a permission check we are potentially starting up two router services. One that the old library tries to start up and another that the newer library is starting up. While they may overlap that doesn't mean they will.

8b.It also appears that it's checking for BT permissions twice, once when gathering the apps and again while it iterates through them.

9. What is the value of BT_PERMISSIONS_CHECK_FREQUENCY when looping to check for BT permission grant while connected over USB.

10. Also is the init check the best place to be creating the BT permission loop check? Does it make sense to be somewhere after USB transport successfully connects?

Answer 10 · 2022-01-12T00:05:43.000Z

The Steering Committee voted to keep this proposal in review until our next meeting in order to allow for more discussion on the review issue.

Answer 11 · 2022-01-18T16:48:46.000Z

@joeygrover
3. I will have to do further testing to confirm the router service will still run even if the exception is caught, but I agree we will need to modify the SdlRouterService.setRouterServiceExceptionHandler to catch this exception in the even the SdlRouterService is not able to start the apps SdlService.
4. I can highlight this section more and update the language to make it clearer perhaps

As an external app’s SdlRouterService would be starting the developers SdlService, the Developer will be required to export their SdlService in the manifest so it is accessible to be started by the SdlRouterService

If the developers SdlService is not exported then the SdlRouterService will not be able to start the SdlService. In this case the `SdlService would need to be started from a foreground context to be visible on the IVI, Developers could implement to help guide users to manually starting the SdlService through the app.
Correct this should be changed so it is no longer in AndrdoidTools and is only in SdlRouterService
Agreed I can make this change
You are correct, there is a chance the app with these changes would try to start its own router service while an app with an older router service tries to start its own at the same time.  We will need to add a RouterServiceVersion check
8B. This check is only happening for the current SdlRouterService
private final static int BT_PERMISSIONS_CHECK_FREQUENCY = 1000; I will add this to the code snippet where it was missed
I agree, would you recommend checking for USB transport type in the SdlRouterService.onTransportConnected method?

Answer 12 · 2022-01-19T14:53:37.000Z

The Steering Committee voted to keep this proposal in review until our next meeting to allow for more discussion on the review issue.

Answer 13 · 2022-01-24T19:45:07.000Z

@joeygrover
For point 3 we can expand the exception catcher in the SdlBroadcastReceiver to handle this

@Override
public void uncaughtException(Thread t, Throwable e) {
    if (e != null
        && e instanceof AndroidRuntimeException
        && ("android.app.RemoteServiceException".equals(e.getClass().getName()) || "android.app.ForegroundServiceDidNotStartInTimeException".equals(e.getClass().getName())) //android.app.RemoteServiceException is a private class
        && e.getMessage() != null
        && (e.getMessage().contains("SdlRouterService")) || e.getMessage().contains("SdlService")) {
            DebugTool.logInfo(TAG, "Handling failed startForegroundService call");
            Looper.loop();
        } else if (defaultUncaughtExceptionHandler != null) { //No other exception should be handled
            defaultUncaughtExceptionHandler.uncaughtException(t, e);
        }
}

Answer 14 · 2022-01-26T14:12:03.000Z

The Steering Committee voted to keep this proposal in review until our next meeting to allow for more discussion on the review issue.

Answer 15 · 2022-01-31T16:30:38.000Z

3. So it appears that the calling broadcast receiver will actually be responsible for the exception even though the context of the router service is being used, which was not not my initial thinking. This makes things a little easier/better designed for the library as developers/apps will be responsible for their own services. The issue I see here is now we are sort of masking this issue for developers if they do something wrong. For this feature it might be warranted otherwise all developers will essentially have to add this code themselves.

3b. There is a new issue using SdlService as the class name because it is not required that developers use that name for the SDL service, and in fact, they can add that functionality to an existing service. I think we need to add another overridable method in the SdlBroadcastReceiver like String getSdlServiceName() and the default implementation simply returns SdlService but a developer has the ability to change that if they need. This will need explanation in the proposal and documentation added to the guides.

4. Yea that's a step in the right direction. I think we should add language that states we will add this as a specific call out in the guides due to its sensitive nature.

5. I think this also needs to be sure to get added to the guides and clearly spelled out in the proposal.

5b. If a developer doesn't export their SdlService, and chooses not to start their SdlService in the background, does the current code logic allow their SdlService to start when the application is opened on the mobile device by the user? ie does calling SdlReceiver.queryForConnectedService(this) in their "main activity" kick off logic to eventually have their SdlService start up?

6. 👍

7. 👍

8. 👍 to include updated code changes in the proposal

9. 👍

10. That might work. At the end of the method checking if the connected transport was USB and then checking status on BT and permission in a conditional statement.

Answer 16 · 2022-02-02T13:57:38.000Z

The Steering Committee voted to keep this proposal in review until our next meeting to allow for more discussion on the review issue.

Answer 17 · 2022-02-03T18:16:00.000Z

3: 👍

3b: I think this approach sounds appropriate for this situation

4:

As an external app’s SdlRouterService would be starting the developers SdlService, the Developer will be required to export their SdlService in the manifest so it is accessible to be started by the SdlRouterService. This information will be highlighted in the developer guides for the Integration Basics page as well as the appropriate migration guide

5: 👍

5b: If the service is not exported, queryForConnectedService will work if the app developers make the call while the app is in the foreground. However this will require two things. 1st the app developers will be responsible for tracking if the app is in the foreground or not. 2nd the SdlReceiver.onSdlEnabled will need to be implemented in such a way that it tries to start the SdlService in the appropriate way. If the app is in the foreground, the developer can directly call startForegroundService to start the SdlService. If the app is in the background the app developer would need the service to be exported and the to utilize the pendingIntent to start the SdlService.

SdlReceiver.java

private static boolean isForeground;

public static void setIsForeground(boolean status) {
    isForeground = status;
}

@Override
    public void onSdlEnabled(Context context, Intent intent) {
        DebugTool.logInfo(TAG, "SDL Enabled");
        intent.setClass(context, SdlService.class);

        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
            if (isForeground) {
                context.startForegroundService(intent);
            } else {
                if (intent.getParcelableExtra(TransportConstants.PENDING_INTENT_EXTRA) != null) {
                    PendingIntent pendingIntent = (PendingIntent) intent.getParcelableExtra(TransportConstants.PENDING_INTENT_EXTRA);
                    try {
                        pendingIntent.send(context, 0, intent);
                    } catch (PendingIntent.CanceledException e) {
                        e.printStackTrace();
                    }
                }
            }
        } else {
            // SdlService needs to be foregrounded in Android O and above
            // This will prevent apps in the background from crashing when they try to start SdlService
            // Because Android O doesn't allow background apps to start background services
            if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
                context.startForegroundService(intent);
            } else {
                context.startService(intent);
            }
        }
    }

MainActivity or where appropriate

private androidx.lifecycle.LifecycleObserver lifecycleObserver;

 @Override
protected void onCreate(Bundle savedInstanceState) {
try {
            lifecycleObserver = new androidx.lifecycle.LifecycleObserver() {
                @androidx.lifecycle.OnLifecycleEvent(androidx.lifecycle.Lifecycle.Event.ON_START)
                public void onMoveToForeground() {
                    SdlReceiver.setIsForeground(true);
                }

                @androidx.lifecycle.OnLifecycleEvent(androidx.lifecycle.Lifecycle.Event.ON_STOP)
                public void onMoveToBackground() {
                    SdlReceiver.setIsForeground(false);
                }
            };

            if (androidx.lifecycle.ProcessLifecycleOwner.get() != null) {
                androidx.lifecycle.ProcessLifecycleOwner.get().getLifecycle().addObserver(lifecycleObserver);
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
}

@Override
protected void onDestroy() {
    super.onDestroy();
    try {
        if (androidx.lifecycle.ProcessLifecycleOwner.get() != null && lifecycleObserver != null) {
            androidx.lifecycle.ProcessLifecycleOwner.get().getLifecycle().removeObserver(lifecycleObserver);
        }
    } catch (Exception e) {
        e.printStackTrace();
    }

    lifecycleObserver = null;
}

Answer 18 · 2022-02-08T21:07:29.000Z

I agree to all points thus far and believe we should return for revisions listed here.

Answer 19 · 2022-02-08T21:21:30.000Z

Here's a summary of the agreed upon revisions:

1. Simplify logic in AndroidTools.java to remove PENDING_BOOLEAN_EXTRA.

2. In Bluetooth Runtime Permissions section, update to state that we should check for the BLUETOOTH_SCAN permission and fail out the method if the permission is not granted. Additionally, wrap ROUTER_REQUEST_BT_CLIENT_CONNECT in a permission check to avoid requiring the BLUETOOTH_SCAN permission.

3. Modify SdlRouterService.setRouterServiceExceptionHandler to expand the exception catcher in the SdlBroadcastReceiver, as described in this comment.

3b. Add another overridable method in the SdlBroadcastReceiver like String getSdlServiceName() and the default implementation returns SdlService but a developer has the ability to change that if needed. A full explanation of this functionality should be added to the proposal and Android guides.

4. Add the following language to the proposal:

As an external app’s SdlRouterService would be starting the developers SdlService, the Developer will be required to export their SdlService in the manifest so it is accessible to be started by the SdlRouterService. This information will be highlighted in the developer guides for the Integration Basics page as well as the appropriate migration guide.

5. Add the following language to the proposal, and call out that it will be included in the Android guides upon proposal implementation:

If the developers SdlService is not exported then the SdlRouterService will not be able to start the SdlService. In this case the `SdlService would need to be started from a foreground context to be visible on the IVI, Developers could implement to help guide users to manually starting the SdlService through the app.

5b. Add the information and code snippets described in point 5b of this comment: #1181 (comment).

6. Only include PendingIntent in the SdlRouterService not AndroidTools utility method.

7.

... and therefore will not know to start its own SdlRouterService when the device connects over Bluetooth.

should be changed to

and therefore will not know to start its own SdlRouterService when the device connects over Bluetooth in the event it has been chosen to host it.

8. Add a RouterServiceVersion check to the SdlBroadcastReceiver modifications.

9. Update BT_PERMISSIONS_CHECK_FREQUENCY code snippet to include private final static int BT_PERMISSIONS_CHECK_FREQUENCY = 1000;.

10. At the end of the SdlRouterService.onTransportConnected method, check to see if the connected transport was USB and then check status on BT and permission in a conditional statement.

Answer 20 · 2022-02-09T14:13:06.000Z

The Steering Committee voted to return this proposal for the revisions outlined in this comment.

Answer 21 · 2022-02-16T16:27:20.000Z

The author has revised this proposal based on the Steering Committee's requested revisions, and the revised proposal is now in review until February 22, 2022.

The specific items that were updated since the last review can be found in this merged pull request: #1183.

Answer 22 · 2022-02-21T14:20:22.000Z

Went through revised proposal, paid attention to sections that refer to Bluetooth Runtime permissions, and they look good to me.

Just one thing to note:
Bluetooth Runtime permissions and other issues addressed in this proposal are such a big deal, but these issues start happening if the SDL app is built with targetSdkVersion = 31 AND if running on Android 12.
I just want to confirm if the new sdl_java_suite library works fine when running on Android 11 or lower versions, and even if the app is built with targetSdkVersion = 30.

Answer 23 · 2022-02-21T15:42:42.000Z

With the revisions I believe this proposal is in a state to accept.

@shiniwat all the permissions checks will be surrounded with an OS version level check for Android 12. If an app is running on a pre-Android 12 OS device, then the library will not use check for those permissions or requests them.

We intend to still ask for permissions if an app targets sdk <=30. Since Google has implemented requirements to update target versions for app submittal it doesn't not make much sense to try and cater to this need that will have an expiration date. While there isn't a set date for Android 12 requirement, judging on the previously set dates we can anticipate 8/2022 for new apps and 11/2022 for existing apps.

It is also not possible to know target version of other apps so the library can't get insight on that when checking if another app has permissions to start up a router service. I would recommend that apps that do not want to implement these newly required runtime permissions use an older version of the SDL library.

Answer 24 · 2022-02-21T23:54:48.000Z

Hi @joeygrover,
I believe the proposal is in a good shape, too.

It makes sense that apps its targetSdkVersion <= 30 do not have to update SDL library (i.e. use the latest released version = 5.3.1).
I understand this proposal addresses the case where app is built with targetSdkVersion = 31 and if running on Android 12. But the updated SDL library in an app built with targetSdkVersion = 31 still has to work on Android 11 or lower versions. if we find something unexpected when running on Android 11 or lower, we can handle it as a bug. Anyway, it has nothing to do with the proposal itself.

Answer 25 · 2022-02-23T13:58:17.000Z

The Steering Committee fully agreed to accept this proposal.

Answer 26 · 2022-02-23T14:01:39.000Z

Implementation Issue: smartdevicelink/sdl_java_suite#1794