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

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[violet.bugreport]

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.)

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Andi, rings a bell?

Comment 2

[Andi [:andi]]

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

Comment 3

[Jonathan Kew [:jfkthame]]

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.

Comment 4

[Andi [:andi]]

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.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 6

[Andi [:andi]]

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.

Comment 8

[Andi [:andi]]

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.

Comment 11

[Andi [:andi]]

Closing this as production has been deployed.