Skip to content
This repository has been archived by the owner on Feb 9, 2023. It is now read-only.

Fix NPE resulting from HoverViewState's callback after it already lost control #3

Merged
merged 4 commits into from
Dec 3, 2018

Conversation

SeoJungHong
Copy link

@SeoJungHong SeoJungHong commented Nov 21, 2018

#1 이 PR에서 NPE fix 만 분리했습니다.

  • If each of the HoverViewState's callback is fired after it has lost control (by changeState()), the NPE can occur because mHoverView has been set to null.
  • Added checking whether the HoverViewState instance still has control (by checking mHasControl) in each of the callbacks.

기존 마스터 상태에서 샘플을 빌드해서 쓸때, State( = HoverViewStateClosed, HoverViewStateCollapsed, HoverViewStateExpanded 중 하나) 들을 빠르게 변경하면 (ex. Expanded 상태로 변하는 중에 백버튼 눌러서 Collapsed 로 만들기) 바로 NPE 가 발생하는 등 불안정한 상태였습니다.
원인을 분석해 보니 State 가 다른 State 로 전환될 때 이전 State 에서의 HoverView 에 대한 레퍼런스를 null 처리로 끊어버리는데, 몇가지 콜백들이 다른 State 로 전환된 이후에 발생할 수 있어서 모든 콜백에 대해 현재 컨트롤을 가지고 있는지 (기존에 정의되어 있던 변수, mHasControl) 를 확인하는 식으로 NPE 를 방지했습니다.

if (!mHasControl) {
throw new RuntimeException("Cannot give control to another HoverMenuController when we don't have the HoverTab.");
}
mHasControl = false;

Choose a reason for hiding this comment

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

mHoverView 가 null 인지 체크하면 문제가 생길까요? PR 상으로는 mHoverView 의 존재 여부가 mHasControl 의 값하고 일치하는 것 같아서요ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

네 말씀하신대로 현재 mHoverView 존재 여부가 mHasControl 과 일치합니다. 하지만 이렇게 따로 만든건 hover 쪽에 PR을 날리기 위해서 이전 로직을 최대한 활용해서 NPE 만 해결하기 위함이었어요. 별건 아니지만 조금 더 Descriptive 한것 같기도 하네요..

Copy link

Choose a reason for hiding this comment

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

그렇군요ㅎㅎ 공감되긴 합니다만 아직 저는 추후 개발하면서 mHasControl 과 mHoverView 의 값이 consistency 가 깨질 위험이 있어 보여서, 실제 문제가 되는 mHoverView 값이 null 임을 확인하는게 더 직관적인 것 같은 생각이 들긴 하네요.

그래도 이 부분 판단은 헤일리 결정에 따르겠습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

@ssowonny 네! 아직은 로직이 복잡하지 않으니 일단 이대로 NPE 만 수정하고, 추후 mHasControl 관련 로직이 복잡해 진다면 실제로 문제가 되는 HoverView 를 체크하는 방향으로 전반적으로 수정하도록 하겠습니다.

@SeoJungHong SeoJungHong merged commit ccf9508 into master Dec 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants