1256701 - [Static Analysis][Dereference after null check] In function BrowserApp::enterEditingMode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that |selectedTab| because of this check:

>>       if (selectedTab != null) {
>>           mTargetTabForEditingMode = selectedTab.getId();
>>            panelId = selectedTab.getMostRecentHomePanel();
>>        } else {
>>            mTargetTabForEditingMode = null;
>>            panelId = null;
>>        }

because of this null possibility a null pointer dereference can occur:

>>        final boolean isUserSearchTerm = !TextUtils.isEmpty(selectedTab.getUserRequested());
>>        if (isUserSearchTerm && SwitchBoard.isInExperiment(getContext(), Experiments.SEARCH_TERM)) {
>>            showBrowserSearchAfterAnimation(animator);
>>        } else {
>>            showHomePagerWithAnimator(panelId, animator);
>>        }

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1256701 - prevent null pointer dereference |selectedTab| in BrowserApp::enterEditingMode. r?sebastian

Review commit: https://reviewboard.mozilla.org/r/40141/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40141/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730763 [details]
MozReview Request: Bug 1256701 - prevent null pointer dereference |selectedTab| in BrowserApp::enterEditingMode. r?sebastian

https://reviewboard.mozilla.org/r/40141/#review37367

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2327
(Diff revision 1)
> +        if (selectedTab != null ) {
> -        final boolean isUserSearchTerm = !TextUtils.isEmpty(selectedTab.getUserRequested());
> +            final boolean isUserSearchTerm = !TextUtils.isEmpty(selectedTab.getUserRequested());
> -        if (isUserSearchTerm && SwitchBoard.isInExperiment(getContext(), Experiments.SEARCH_TERM)) {
> +            if (isUserSearchTerm && SwitchBoard.isInExperiment(getContext(), Experiments.SEARCH_TERM)) {
> -            showBrowserSearchAfterAnimation(animator);
> +                showBrowserSearchAfterAnimation(animator);
> -        } else {
> +            } else {
> -            showHomePagerWithAnimator(panelId, animator);
> +                showHomePagerWithAnimator(panelId, animator);
> -        }
> +            }
> -
> +        }

My assumption is that we want to show the homepager if there's no selected tab.

So I guess this should be:
final boolean isUserSearchTerm = selectedTab != null &&  !TextUtils.isEmpty(selectedTab.getUserRequested());

Comment 3

[Andi [:andi]]

Comment on attachment 8730763 [details]
MozReview Request: Bug 1256701 - prevent null pointer dereference |selectedTab| in BrowserApp::enterEditingMode. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40141/diff/1-2/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8730763 [details]
MozReview Request: Bug 1256701 - prevent null pointer dereference |selectedTab| in BrowserApp::enterEditingMode. r?sebastian

https://reviewboard.mozilla.org/r/40141/#review37587

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:2326
(Diff revision 2)
>          animator.setUseHardwareLayer(false);
>  
>          TransitionsTracker.track(animator);
>  
>          mBrowserToolbar.startEditing(url, animator);
> -
> +        

NIT: Unnecessary white spaces.

Comment 5

[Andi [:andi]]

Comment on attachment 8730763 [details]
MozReview Request: Bug 1256701 - prevent null pointer dereference |selectedTab| in BrowserApp::enterEditingMode. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40141/diff/2-3/

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5371f013cb35

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5371f013cb35