Fix/issue 1829 #1830
Fix/issue 1829 #1830
Conversation
| @@ -265,8 +265,10 @@ void ResumeCtrlImpl::OnSuspend() { | |||
| LOG4CXX_AUTO_TRACE(logger_); | |||
| StopSavePersistentDataTimer(); | |||
| SaveAllApplications(); | |||
| resumption_storage_->OnSuspend(); | |||
| resumption_storage_->Persist(); | |||
| if (resumption_storage_) { | |||
There was a problem hiding this comment.
resumption_storage cant be uninitialized if Init method was called.
It will be initialized either here: https://github.com/fronneburg/sdl_core/blob/58af36f00d9af10d18274251d750cf43b4b9e190/src/components/application_manager/src/resumption/resume_ctrl_impl.cc#L83
Or here https://github.com/fronneburg/sdl_core/blob/58af36f00d9af10d18274251d750cf43b4b9e190/src/components/application_manager/src/resumption/resume_ctrl_impl.cc#L107
But in case if Init method was not called, or other resumption method was called before Init called. this if won't resolve issue of null pointer access. Code will fail in StopSavePersistentDataTimer method in https://github.com/fronneburg/sdl_core/blob/58af36f00d9af10d18274251d750cf43b4b9e190/src/components/application_manager/src/resumption/resume_ctrl_impl.cc#L138
The only way to avoid null pointer access is to guarantee that resumption_storage_ is created before each ResumeCtrl method call. This can be done with DCHECKs or with creating stub of resumption_storage_ in ResumeCtrl constructor. Best solution is to create real resumption_storage_ in constructor, but it requires rework of SDL LifeCycle.
There was a problem hiding this comment.
In ApplicationManagerImpl::Init, if the first call to InitDirectory fails, this causes LifeCycle::StartComponents to fail and the resumption_storage_ object in ResumeCtrlImpl will not be created. And then "main" calls LifeCycle::StopComponents, which calls ApplicationManagerImpl::Stop, which calls UnregisterAllApplications, which calls ResumeCtrlImpl::OnSuspend, with an uninitialized resumption_storage_ object.
Yes, it's true that there is another null pointer access, but I couldn't see that one being called unless proper init had taken place.
|
Can one of the admins verify this patch? |
|
@fronneburg Merge conflicts need to be resolved |
|
There seems to be a problem with protocol_handler_tm_test.cc that is unrelated to these changes. It happened on another, completely separate PR also. |
|
@fronneburg @jacobkeeler issues #1829 and #2428 are fixed in PR #2621. Current PR is not actual since there are not enough changes for current develop: ResumeCtrlImpl::OnIgnitionOff is called instead of ResumeCtrlImpl::OnSuspend in ApplicationManagerImpl::UnregisterAllApplications. |
|
These changes are no longer needed since #2621 |
Handle a crash in ResumeCtrlImpl cleanup if the class hasn't been fully initialized.