1230133 - [Static Analysis][Dereference Null Return Value] Function nsLayoutUtils::GetReferenceFrame from nsLayoutUtils.cpp

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that aFrame from GetReferenceFrame can be bassed null from nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer on line:

      nsIFrame* referenceFrame =
        nsLayoutUtils::GetReferenceFrame(nsLayoutUtils::GetCrossDocParentFrame(aFrame));

Comment 1

[Andi [:andi]]

Attachment: Bug 1230133.diff

Hello David,

Could you please take a look other this patch?

THX

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this is only safe because we know that it's not possible to animate a transform on the ViewportFrame.

It's not clear to me how adding an assertion would help fix a coverity warning, though.

Comment 3

[Andi [:andi]]

having that assertion there tell coverity that if assertion it should stop execution of the code, so the expected behavior would be the assertion to be valid. 
The current assertion model implemented for coverity is:

void MOZ_ASSERT(bool b) { 
	if (!b)
		__coverity_panic__();
}

__coverity_panic__() - indicates function ends the execution path

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So does this replace one error report with a different one, or does it remove the error entirely?

Comment 5

[Andi [:andi]]

it removes the error generated by Coverity, since it know by running into that assert that the normal behavior it would be to have a valid pointer, so no null dereference would be expected.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what if there were a similar case, but where comment 2 wasn't true?  Wouldn't this then just hide a real error?

Comment 7

[Andi [:andi]]

No it will trigger an assertion, the implementation moz_assert that i've copied here is valid only for coverity, it will not affect the actual implementation from the trunk.
To recap:
1. Coverity detected a pointer
2. Coverity assumed that the pointer is null
3. Coverity signaled a null pointer dereference

by adding the moz_assert we check at runtime the validity of that pointer, at least that it's different than null, and we instruct coverity that the null pointer case is treated by our assert function, at least on debug. 

If i wasn't clear enough let me know.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But isn't the point of coverity to detect things statically that might not be covered in our test suite?  If comment 2 weren't true, wouldn't we want coverity to warn in that case?

Comment 9

[Andi [:andi]]

Yes you are right, this is the goal of static analysis, as i see it there are two options:
1. Silent the warning and mark it as false-positive, i don't consider this to be the best approach.
2. Check the value of pointer if it's null, return null, and also check the returned value in the function where it gets called such as:
nsDisplayList.cpp

>>      // is also reference frame too, so the parent's reference frame
>>      // are used.
>>      nsIFrame* referenceFrame =
>>        nsLayoutUtils::GetReferenceFrame(nsLayoutUtils::GetCrossDocParentFrame(aFrame));
>>      origin = aFrame->GetOffsetToCrossDoc(referenceFrame);

nsImageFrame.cpp

>>  // ...and this frame's content box...
>>  const nsPoint offset =
>>    GetOffsetToCrossDoc(nsLayoutUtils::GetReferenceFrame(this));
>>  const nsRect frameContentBox = GetInnerArea() + offset;

If you agree with the 2nd option i will submit a patch for review.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Bogdan Postelnicu from comment #9)
> nsDisplayList.cpp
> 
> >>      // is also reference frame too, so the parent's reference frame
> >>      // are used.
> >>      nsIFrame* referenceFrame =
> >>        nsLayoutUtils::GetReferenceFrame(nsLayoutUtils::GetCrossDocParentFrame(aFrame));
> >>      origin = aFrame->GetOffsetToCrossDoc(referenceFrame);

I don't think there's any risk of aFrame being null, since aFrame is a frame with animations or transitions, which must have a viewport ancestor.

> nsImageFrame.cpp
> 
> >>  // ...and this frame's content box...
> >>  const nsPoint offset =
> >>    GetOffsetToCrossDoc(nsLayoutUtils::GetReferenceFrame(this));
> >>  const nsRect frameContentBox = GetInnerArea() + offset;

And certainly no risk of this being null.

So I don't think we should add null checks here.

Comment 11

[Andi [:andi]]

Thank you David, great to clear this out. I was not aware of the logic for this component.