Closed Bug 1230133 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED INVALID
Tracking Status
firefox45 --- affected

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1338373)

Attachments

(1 file)

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));
Whiteboard: CID 1338373
Attached patch Bug 1230133.diffSplinter Review
Hello David,

Could you please take a look other this patch?

THX
Attachment #8695261 - Flags: review?(dbaron)
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.
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
Flags: needinfo?(dbaron)
So does this replace one error report with a different one, or does it remove the error entirely?
Flags: needinfo?(dbaron)
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.
So what if there were a similar case, but where comment 2 wasn't true?  Wouldn't this then just hide a real error?
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.
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?
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.
(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.
Attachment #8695261 - Flags: review?(dbaron) → review-
Thank you David, great to clear this out. I was not aware of the logic for this component.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.