Closed Bug 1549614 Opened 6 months ago Closed 5 months ago

[Automated review] Spurious "known to be null" review comment on non-changed code

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cpearce, Assigned: andi)

Details

Phabricator URL: https://phabricator.services.mozilla.com/D26233#inline-174985

Coverity helpfully left an automated review comment on a phabricator revision I pushed, "Dereferencing "bo", which is known to be "nullptr". The "bo" it's referring to is in MediaCache::InsertReadaheadBlock(, plus one more in MediaCache::NoteSeek.

There are two problems with these automated comments:

  1. "bo" is not known to be null here. It is asserted that it's not null on the line above, though I'd not judge the machine too harsely on this, as it's by no means a guarantee. Does coverity expect every single pointer deref to be preceeded by a null check?
  2. My commit didn't change this line of code. This is the more important point I think. Why is my commit having problems pre-existing in the code assigned to my commit? Only problems which my commits are introducing should invoke lint comments.

Another couple of occurences for my patch : https://phabricator.services.mozilla.com/D27893#inline-174615

Assignee: nobody → bpostelnicu

More here:
https://phabricator.services.mozilla.com/D30322#890265
https://phabricator.services.mozilla.com/D30324#890286
https://phabricator.services.mozilla.com/D30321#890298

As with the original report, not only are most of these lines untouched by these patches, the errors are false positives since there are assertions earlier in these functions that assert that the mentioned pointers are not nullptr.

It just keeps on going:
https://phabricator.services.mozilla.com/D30327#890381
https://phabricator.services.mozilla.com/D30321#890387
https://phabricator.services.mozilla.com/D30322#890408

Six bogus reports for a single bug so far. At this point I'm likely to overlook any genuinely useful message. Any chance we can turn this warning off?

Sorry for the bad experience that you are having with this checker. I will try to 'make it smarter' by modeling it based on our code or this doesn't succeeds I will disable it.
For the other problem where there are posted issues that are not part of the patch we have (this)[https://bugzilla.mozilla.org/show_bug.cgi?id=1542163] issue.

In case it's of any use, bug 1253476 might be useful example of the issues we need to fix. Between all the patches these, there would be at least 50 bogus comments by now. On one patch alone I see 3 bogus comments each duplicated 7 times. If we can't disable the check or make it smarter soon, perhaps we can at least stop it from producing the same comment over and over?

The priority flag is not set for this bug.
:sylvestre, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(sledru)

I think we fixed it, closing.

Status: NEW → RESOLVED
Closed: 5 months ago
Flags: needinfo?(sledru)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.