1248908 - [Static Analysis][Dereference after null check] In function ToolbarDisplayLayout::updateProgress

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable tab is first checked for null:

>>        final boolean shouldShowThrobber = (tab != null &&
>>                                            tab.getState() == Tab.STATE_LOADING);

and afterwards dereferemced:

>>        if (Tab.STATE_SUCCESS == tab.getState() && mTrackingProtectionEnabled) {
>>            mActivity.showTrackingProtectionPromptIfApplicable();

Althow this is a false positive since the stack for  calling updateProgress is:

>> updateProgress
>> updateFromTab
>> updateFromTab
>> onTabChanged
>> onTabChanged

Now in function ContentResolver::onTabChanged we have this code that checks tab to be different than null:

>>        if (tab == null) {
>>            // Only RESTORED is allowed a null tab: it's the only event that
>>            // isn't tied to a specific tab.
>>            if (msg != Tabs.TabEvents.RESTORED) {
>>                throw new IllegalArgumentException("onTabChanged:" + msg + " must specify a tab.");
>>            }
>>            return;
>>        }

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1248908 - ToolbarDisplayLayout: Remove unnecessary null checks. r?sebastian

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

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

This contradicts with the changes done in bug 1248799 / bug 1236431: One tool says the null check is unnecessary because the code before doesn't do one. And the other tool says we need to add more null checks because not everything is covered.

It seems like you already looked at the callstack and the tab can never be null. So this tool here and your change is correct. You'll probably have to rebase the patch though. Can you also add a @NonNull annotation to the tab parameter where appropriate? This way most tools (including Android Studio) will get this right and complain about unnecessary null checks.

NIT: Typo in commit message: "remvoe"

Comment 3

[Andi [:andi]]

sure i'll do this and update the patch.

Comment 4

[Andi [:andi]]

Comment on attachment 8720220 [details]
MozReview Request: Bug 1248908 - ToolbarDisplayLayout: Remove unnecessary null checks. r?sebastian

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

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8720220 [details]
MozReview Request: Bug 1248908 - ToolbarDisplayLayout: Remove unnecessary null checks. r?sebastian

https://reviewboard.mozilla.org/r/35235/#review32069

Thanks! This is looking good. :) Let's push it to try to see if it causes any problems.

NIT: The commit message is pretty long. I guess something like "ToolbarDisplayLayout: Remove unnecessary null checks" is sufficient.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java
(Diff revision 2)
> -

NIT: Keep the separation here.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(I triggered a try run on mozreview)

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8720220 [details]
MozReview Request: Bug 1248908 - ToolbarDisplayLayout: Remove unnecessary null checks. r?sebastian

https://reviewboard.mozilla.org/r/35235/#review32071

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:352
(Diff revision 2)
> -    private void updateProgress(Tab tab) {
> +    private void updateProgress(@NonNull Tab tab) {

It looks like you are not on the latest version of the tree yet (maybe fx-team wasn't merged to mozilla-central yet). This method looks a bit different now:
http://hg.mozilla.org/integration/fx-team/file/tip/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#l345

So you might need to do another rebase (+ Remove the new null check and @Nullable) before we can land this.

Comment 8

[Andi [:andi]]

Comment on attachment 8720220 [details]
MozReview Request: Bug 1248908 - ToolbarDisplayLayout: Remove unnecessary null checks. r?sebastian

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8720220 [details]
MozReview Request: Bug 1248908 - ToolbarDisplayLayout: Remove unnecessary null checks. r?sebastian

https://reviewboard.mozilla.org/r/35235/#review32089

LGTM and all the jobs passed on try.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5658512697c4

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5658512697c4