Closed Bug 1542163 Opened 7 months ago Closed 6 months ago

[Automated review] Coverity suggests defect in a huge file that has nothing to do with the specific patch

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: violet.bugreport, Assigned: andi)

References

(Blocks 1 open bug, )

Details

Phabricator URL: https://phabricator.services.mozilla.com/D26271

My patch has nothing to do with the defect Coverity suggested, the only thing makes them related is that they are in different parts of the same huge file.

(BTW, I don't think the said defect Dereferencing pointer "aFrame" even exists. The pointer is just passed to a constructor, there is no dereference.)

Andi, rings a bell?

Flags: needinfo?(bpostelnicu)

Sorry for the bat experience that you've had. The latest snapshot that was used for the analysis did have that issue in it. This was happening because the code in that region changed a little bit and Coverity didn't have time to build the most up to date snapshot.
This kind of issues will not happen in the future when we will build snapshop more frequently.

When it comes to the actual bug that was triggered by coverity, indeed aFrame is passed to constructor nsDisplayHitTestInfoItem from there is passed to ctor nsDisplayItem and from there is dereferenced, please see [1]

Thank you again for reporting this.

[1] https://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#3188

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → WORKSFORME
Flags: needinfo?(bpostelnicu)

Is this really working as expected? I'm seeing similar spurious reports, e.g. https://phabricator.services.mozilla.com/D30260#inline-176891 where the statement that coverity dislikes is about 1000 lines away from anything my patch touched, and https://phabricator.services.mozilla.com/D30260#inline-176890 is 5000 lines away.

The "noise" of irrelevant reports like this makes it much less likely that as a developer I'll notice when there's a genuine issue with my patch, which is sad -- in theory, the automated reviews are a valuable aid, but the more "false positives" they include, the more they lose their value.

Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Jonathan thank you for the feedback, right now we are relying solely on what Coverity server is providing us, and it seems it fails from time to time. There are several issues why this might happen:

  • The codebase that the patch is used is newer than the last snapshot captured by the coverity server and hence it doesn't know about other issues in the code.
  • From time to time the analysis becomes divergent, and what we analyze on a per patch basis may differ a little bit than what we actually have on the latest snapshot that is created on the entire code base.

I will apply the same reasoning for the Coverity analyzer like we do for clang-tidy and thus we will eliminate this inconvenient.

Assignee: nobody → bpostelnicu

FWIW, this is also another example where Coverity suggest fixes that are irrelevant to my patch https://phabricator.services.mozilla.com/D30581#899191

Sorry again for the bad experience, this will be fixed by the end of next week when we have scheduled our next deployment of releneg-services.

Duplicate of this bug: 1551037

This patch fixes the above issue. As stated earlier in the bug the fix will be available after the next release of the releneg-servie.

No longer regressed by: coverity-analysis
Duplicate of this bug: 1551715
Duplicate of this bug: 1552721

Closing this as production has been deployed.

Status: REOPENED → RESOLVED
Closed: 7 months ago6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.