Attepted fix of NPE in #1783 by noah-livio · Pull Request #1786 · smartdevicelink/sdl_java_suite

noah-livio · 2022-01-25T19:11:21Z

This is not ready for review.

Risk

None

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

testDisposeAfterRetryChangeRegistration() - Creates a sample SdlManager, calls retryChangeRegistration() and then calls dispose(). This test will pass if no exceptions are thrown.

testRetryChangeRegistrationAfterDispose() - Creates a sample SdlManager, calls dispose() and then calls retryChangeRegistration(). This test will pass if no exceptions are thrown.

Core Tests

Both Core tests were performed in a modified version of Hello Sdl Android, while locally making retryChangeRegistration() public so it can be directly called in the app.

At the end of SdlService.performWelcomeShow() I called sdlManager.retryChangeRegistration() then sdlManager.dispose(). As a result, the app did not crash and SdlManager did not throw a visible exception.
At the end of SdlService.performWelcomeShow() I called sdlManager.dispose() then sdlManager.retryChangeRegistration(). As a result, the app did not crash and SdlManager did not throw a visible exception.

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

Converts local variable handler in SdlManager.retryChangeRegistration() to a private member of the class
Creates private member changeRegistrationRunnable to replace anonymous Runnable in SdlManager.retryChangeRegistration()

Add DebugTool warning in the default constructor of SdlManager to let developers know not to use the constructor directly.

In SdlManager.dismiss(), remove changeRegistrationRunnable from handler if both are not null

In BaseSdlManager.checkLifecycleConfiguration() add a null check for managerListener

Changelog

Breaking Changes

None

Enhancements

None

Bug Fixes

Fixes NPE crash in BaseLifecycleManager on checkLifecycleConfiguration #1783

CLA

I have signed the CLA

codecov · 2022-01-25T19:18:44Z

Codecov Report

Merging #1786 (04ec0e3) into develop (039503d) will increase coverage by 0.05%.
The diff coverage is 30.59%.

❗ Current head 04ec0e3 differs from pull request most recent head e816eec. Consider uploading reports for the commit e816eec to get more accurate results

@@              Coverage Diff              @@
##             develop    #1786      +/-   ##
=============================================
+ Coverage      54.36%   54.42%   +0.05%     
- Complexity      5513     5522       +9     
=============================================
  Files            562      562              
  Lines          25516    25543      +27     
  Branches        3319     3328       +9     
=============================================
+ Hits           13872    13901      +29     
  Misses         10382    10382              
+ Partials        1262     1260       -2

Impacted Files	Coverage Δ
...icelink/managers/lockscreen/LockScreenManager.java	`46.74% <0.00%> (ø)`
...tdevicelink/managers/video/VideoStreamManager.java	`22.40% <0.00%> (ø)`
...managers/video/resolution/VideoStreamingRange.java	`52.72% <0.00%> (ø)`
...icelink/transport/MultiplexBluetoothTransport.java	`4.23% <ø> (ø)`
...martdevicelink/transport/SdlBroadcastReceiver.java	`3.04% <0.00%> (ø)`
...om/smartdevicelink/transport/SdlRouterService.java	`11.09% <ø> (ø)`
...artdevicelink/transport/utl/SdlDeviceListener.java	`7.93% <0.00%> (ø)`
...in/java/com/smartdevicelink/util/AndroidTools.java	`20.89% <0.00%> (ø)`
...a/com/smartdevicelink/managers/BaseSdlManager.java	`49.24% <0.00%> (-0.57%)`	⬇️
...tdevicelink/managers/file/UploadFileOperation.java	`67.83% <0.00%> (ø)`
... and 31 more

Add another check in case the manager listener is never instantiated

Also make SdlManager default constructor package private

Resolve merge conflicts

Also add debug warning about using the constructor outside the builder

RHenigan · 2022-02-03T20:18:51Z

base/src/main/java/com/smartdevicelink/managers/BaseSdlManager.java

-                            }
-                        } else {
-                            DebugTool.logError(TAG, "Change Registration onError: " + response.getResultCode() + " | Info: " + response.getInfo());
-                            retryChangeRegistration();


Looks like this code was missed, this should not be removed

RHenigan · 2022-02-03T20:22:43Z

android/sdl_android/src/androidTest/java/com/smartdevicelink/managers/SdlManagerTests.java

+        SdlManager sdlManager = createSampleManager("Test App", "test.test.test", lockScreenConfig);
+        sdlManager.dispose();
+        sdlManager.retryChangeRegistration();
+    }


I dont think these unit tests actually will catch what we are trying to prevent. If the checkLifecycleConfiguration method is updated to manually throw an exception these tests will still pass. It does not look like the code is waiting for the runnable to complete so I believe the tests are completing before the runnable even executes.

Think I should just remove them then?

Yeah, I dont believe these tests are needed

Okay, will do

RHenigan · 2022-02-03T20:23:07Z

android/sdl_android/src/main/java/com/smartdevicelink/managers/SdlManager.java

+    };
+
+    public SdlManager(){
+        DebugTool.logWarning(TAG, "If this SdlManager was created without using SdlManager.Builder, most of its members are not initialized");


Suggested change

DebugTool.logWarning(TAG, "If this SdlManager was created without using SdlManager.Builder, most of its members are not initialized");

DebugTool.logWarning(TAG, "SdlManager must be created with SdlManager.Builder");

noah-livio · 2022-02-04T18:06:36Z

Fixes #1783

This is not ready for review.

Risk

None

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 - None
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

None

Core Tests

Both Core tests were performed in a modified version of Hello Sdl Android, while locally making retryChangeRegistration() public so it can be directly called in the app.

At the end of SdlService.performWelcomeShow() I called sdlManager.retryChangeRegistration() then sdlManager.dispose(). As a result, the app did not crash and SdlManager did not throw a visible exception.
At the end of SdlService.performWelcomeShow() I called sdlManager.dispose() then sdlManager.retryChangeRegistration(). As a result, the app did not crash and SdlManager did not throw a visible exception.

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

Converts local variable handler in SdlManager.retryChangeRegistration() to a private member of the class
Creates private member changeRegistrationRunnable to replace anonymous Runnable in SdlManager.retryChangeRegistration()

Add DebugTool warning in the default constructor of SdlManager to let developers know not to use the constructor directly.

In SdlManager.dismiss(), remove changeRegistrationRunnable from handler if both are not null

In BaseSdlManager.checkLifecycleConfiguration() add a null check for managerListener

Changelog

Breaking Changes

None

Enhancements

None

Bug Fixes

Fixes NPE crash in BaseLifecycleManager on checkLifecycleConfiguration #1783

CLA

I have signed the CLA

Attepted fix of NPE in #1783

c0e5a39

noah-livio added 4 commits January 25, 2022 15:02

Add additional null check

398ae47

Add another check in case the manager listener is never instantiated

Add unit tests

6f1b478

Also make SdlManager default constructor package private

Merge branch 'develop' into bugfix/issue_1783

150f294

Resolve merge conflicts

Revert SdlManager default constructor

8432443

Also add debug warning about using the constructor outside the builder

noah-livio marked this pull request as ready for review January 27, 2022 21:02

RHenigan reviewed Feb 3, 2022

View reviewed changes

noah-livio added 2 commits February 4, 2022 09:57

PR feedback changes

acffcf2

Remove new unit tests

e816eec

RHenigan approved these changes Feb 8, 2022

View reviewed changes

RHenigan merged commit b51cda0 into develop Feb 8, 2022

RHenigan deleted the bugfix/issue_1783 branch February 8, 2022 17:41

theresalech mentioned this pull request Feb 17, 2022

NPE crash in BaseLifecycleManager on checkLifecycleConfiguration #1783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attepted fix of NPE in #1783#1786

Attepted fix of NPE in #1783#1786
RHenigan merged 7 commits intodevelopfrom
bugfix/issue_1783

noah-livio commented Jan 25, 2022 •

edited

Loading

Uh oh!

codecov bot commented Jan 25, 2022 •

edited

Loading

Uh oh!

RHenigan Feb 3, 2022

Uh oh!

noah-livio Feb 4, 2022

Uh oh!

RHenigan Feb 3, 2022

Uh oh!

noah-livio Feb 4, 2022

Uh oh!

RHenigan Feb 4, 2022

Uh oh!

noah-livio Feb 4, 2022

Uh oh!

RHenigan Feb 3, 2022

Uh oh!

noah-livio commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	DebugTool.logWarning(TAG, "If this SdlManager was created without using SdlManager.Builder, most of its members are not initialized");
	DebugTool.logWarning(TAG, "SdlManager must be created with SdlManager.Builder");

Conversation

noah-livio commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

CLA

Uh oh!

codecov bot commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RHenigan Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

noah-livio Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

RHenigan Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

noah-livio Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

RHenigan Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

noah-livio Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

RHenigan Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

noah-livio commented Feb 4, 2022

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

CLA

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

noah-livio commented Jan 25, 2022 •

edited

Loading

codecov bot commented Jan 25, 2022 •

edited

Loading