Closed
Bug 1248908
Opened 8 years ago
Closed 8 years ago
[Static Analysis][Dereference after null check] In function ToolbarDisplayLayout::updateProgress
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox47 fixed)
RESOLVED
FIXED
Firefox 47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: andi, Assigned: andi)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID 123355)
Attachments
(1 file)
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; >> }
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/35235/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35235/
Attachment #8720220 -
Flags: review?(mark.finkle)
Comment 2•8 years ago
|
||
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"
Assignee | ||
Comment 3•8 years ago
|
||
sure i'll do this and update the patch.
Assignee | ||
Comment 4•8 years ago
|
||
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/
Attachment #8720220 -
Attachment description: MozReview Request: Bug 1248908 - remvoe useless null check for tab in ToolbarDisplayLayout::updateProgress. r?mfinkle → MozReview Request: Bug 1248908 - modified class ToolbarDisplayLayout in order to eliminate useless tab against null checking and pass is as @NonNull since we are sure it's a valid object. r?sebastian
Attachment #8720220 -
Flags: review?(mark.finkle) → review?(s.kaspari)
Updated•8 years ago
|
Attachment #8720220 -
Flags: review?(s.kaspari) → review+
Comment 5•8 years ago
|
||
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•8 years ago
|
||
(I triggered a try run on mozreview)
Comment 7•8 years ago
|
||
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.
Attachment #8720220 -
Flags: review+
Assignee | ||
Comment 8•8 years ago
|
||
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/
Attachment #8720220 -
Attachment description: MozReview Request: Bug 1248908 - modified class ToolbarDisplayLayout in order to eliminate useless tab against null checking and pass is as @NonNull since we are sure it's a valid object. r?sebastian → MozReview Request: Bug 1248908 - ToolbarDisplayLayout: Remove unnecessary null checks. r?sebastian
Attachment #8720220 -
Flags: review?(s.kaspari)
Comment 9•8 years ago
|
||
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.
Attachment #8720220 -
Flags: review?(s.kaspari) → review+
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5658512697c4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•