1551166 - [Automated review] Coverity thinks a pointer is potentially null if after a null-check that breaks a loop

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P2)

Description

[Henri Sivonen (:hsivonen)]

https://phabricator.services.mozilla.com/D30873

Unrelated to the changes in the patch, the file being patched, nsFocusManager.cpp has code like this

  while (1) {
    nsIFrame* frame = iterStartContent->GetPrimaryFrame();
    // if there is no frame, look for another content node that has a frame
    while (!frame) {
      // if the root content doesn't have a frame, just return
      if (iterStartContent == aRootContent) {
        return NS_OK;
      }

      // look for the next or previous content node in tree order
      iterStartContent = aForward ? iterStartContent->GetNextNode()
                                  : iterStartContent->GetPreviousContent();
      if (!iterStartContent) {
        break;
      }

      frame = iterStartContent->GetPrimaryFrame();

Coverity thinks that iterStartContent may be null on the last line quoted above despite the break immediately above in the null case.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Andi, what do you think?

Comment 2

[Andi [:andi]]

Obviously this is a serious problem with the way how the analyzer looks at iterStartContent, it's like it doesn't understand how container nsCOMPtr behaves. Here we can find more details about the issue in order to understand why the analyzer disregard the logic block where we break from the while loop.

From the above mention artifact we can see the last 2 stack events:

19	
line_number	3330
description	"Member function call \"iterStartContent\" returns field \"mRawPtr\"."
file_path	"/builds/worker/checkouts…base/nsFocusManager.cpp"
path_type	"identity_transfer"
20	
line_number	3330
description	"Dereferencing a pointer that might be \"nullptr\" \"iterStartContent\" when calling \"GetPrimaryFrame\"."
file_path	"/builds/worker/checkouts/gecko/dom/base/nsFocusManager.cpp"
path_type	"dereference"

This definitely looks like a bug in Coverity, but I think we have two solutions for this:

• I plan on updating this week to the latest analyzer version 2019.4
• I will pass the modeling file to the review-time analyzer, in order to better understand MOZ_ASSERT since in the above case the nullptr is guarderd as such, https://searchfox.org/mozilla-central/source/xpcom/base/nsCOMPtr.h#842

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.

Comment 4

[Andi [:andi]]

This has been fixed in the latest reviewbot updates for the coverity try job.