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.

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

Prevent NPE in SdlDeviceListener #1789

Merged
merged 2 commits into from
Feb 16, 2022
Merged

Prevent NPE in SdlDeviceListener #1789

merged 2 commits into from
Feb 16, 2022

Conversation

RHenigan
Copy link
Contributor

@RHenigan RHenigan commented Feb 7, 2022

Fixes #1780

This PR is [not 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 Core and verified behavior (if applicable, if not applicable, explain why below).
  • I have tested Android, Java SE, and Java EE

Unit Tests

N/A

Core Tests

[List of tests performed against Core and behaviors verified]

Core version / branch / commit hash / module tested against: [INSERT]
HMI name / version / branch / commit hash / module tested against: [INSERT]

Summary

In SdlDeviceListener the bluetoothTransport and bluetoothHandler are only initialized if the device has not connected previously. In the event that the device has connected before the bluetoothTransport is not initialized because we get the relevant data from the shared preferences. If this is the case bluetoothTransport will not be initialized and will be null. If this is the case the vehicleInfo will be retrieved from the shared preferences instead and the logic to retrieve the vehicleData from the startServiceACK is not needed.

in SdlDeviceListener the onPacketRead method will be called when the StartServiceACK is received. This will later call notifyConnection which will try to stop the bluetoothTransport if the RFCOMM channel should not remain open. If this is the case after stoping the bluetoothTransport it will then be set to null. When this happens we should also set the bluetoothHandler to null as we no longer want its handleMessages method to be called. This will prevent the case where notifyConnection is called while the bluetoothTransport is null

In onPacketRead we are already performing a null check before writing to the bluetoohTransport. Since it is possible that bluetoothTransport is null when notifyConnection is called we should also check for null before calling bluetoothTransport.stop().

CLA

@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #1789 (0702630) into develop (3d1e54e) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1789   +/-   ##
==========================================
  Coverage      54.42%   54.43%           
- Complexity      5519     5523    +4     
==========================================
  Files            562      562           
  Lines          25540    25542    +2     
  Branches        3327     3328    +1     
==========================================
+ Hits           13900    13903    +3     
+ Misses         10381    10380    -1     
  Partials        1259     1259           
Impacted Files Coverage Δ
...artdevicelink/transport/utl/SdlDeviceListener.java 7.81% <0.00%> (-0.13%) ⬇️
...ink/managers/screen/BaseTextAndGraphicManager.java 64.58% <0.00%> (+0.83%) ⬆️
...rtdevicelink/streaming/video/SdlRemoteDisplay.java 51.21% <0.00%> (+1.21%) ⬆️

@noah-livio
Copy link
Contributor

Fixes #1780

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 - N/A
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).
  • I have tested Android, Java SE, and Java EE

Unit Tests

N/A

Core Tests

Using a modified version of the SDL library without Robert's changes, I was able to force the NPE by setting bluetoothTransport to null before notifyConnection() is called.

After including Robert's changes, the NPE no longer occurs and the crash is prevented.

Core version / branch / commit hash / module tested against: SDL Core v8.0.0 master 68f082169e0a40fccd9eb0db3c83911c28870f07
HMI name / version / branch / commit hash / module tested against: Generic HMI v0.11.0 master 47e0ad42f05b27adff61befd864e79c2ab4b8cec

Summary

This PR surrounds sdlListener.bluetoothTransport.stop() in SdlDeviceListener.notifyConnection() with a null check and sets sdlListener.bluetoothHandler to null in the last line of the null checked block.

I have verified Robert's assessment regarding possible causes of bluetoothTransport entering a null state and have also verified his changes are sufficient to prevent the reported NPE.

CLA

@noah-livio noah-livio marked this pull request as ready for review February 16, 2022 14:31
@JulianKast JulianKast merged commit e5398b4 into develop Feb 16, 2022
@JulianKast JulianKast deleted the bugfix/issue_1780 branch February 16, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants