Closed Bug 1256701 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 121390)

Attachments

(1 file)

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);
>>        }
Attachment #8730763 - Flags: review?(s.kaspari)
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 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/
Attachment #8730763 - Flags: review?(s.kaspari)
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.
Attachment #8730763 - Flags: review?(s.kaspari) → review+
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/
https://hg.mozilla.org/mozilla-central/rev/5371f013cb35
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.