1232635 - [Static Analysis][Dereference before null check] In function FrameLayerBuilder::DrawPaintedLayer from FrameLayerBuilder.cpp

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that presContext is dereferenced before being null checked.

dereference:

>>    // PaintedLayers. If aDirtyRegion has not changed since the previous call
>>    // then we can skip this.
>>    int32_t appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel();
>>    RecomputeVisibilityForItems(entry->mItems, builder, aDirtyRegion,
>>                                offset, appUnitsPerDevPixel,
>>                                userData->mXScale, userData->mYScale);
>>    userData->mVisibilityComputedRegion = aDirtyRegion; 

null check:

>>  if (presContext && presContext->GetDocShell() && isActiveLayerManager) {
>>    nsDocShell* docShell = static_cast<nsDocShell*>(presContext->GetDocShell());
>>    RefPtr<TimelineConsumers> timelines = TimelineConsumers::Get();

The null check is done after the dereference was made.

if we expect that the normal behavior of this functios is to always have a valid presContext we should add an assert checking it's validity thus letting know Coverity that this is the intended behavior.

Comment 1

[Andi [:andi]]

Attachment: Bug 1232635.diff

Comment 2

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

Comment on attachment 8698411 [details] [diff] [review]
Bug 1232635.diff

Coverity should know that PresContext() never returns a null pointer without this.  We should fix that problem rather than adding this assertion.

Comment 3

[Andi [:andi]]

What actually confused Coveirty was this check:

>>  if (presContext && presContext->GetDocShell() && isActiveLayerManager) {
>>    nsDocShell* docShell = static_cast<nsDocShell*>(presContext->GetDocShell());
>>    RefPtr<TimelineConsumers> timelines = TimelineConsumers::Get();

Testing the validity of presContext determined Coverity to assume that it could also be NULL so that's why it triggered dereference before null check.

So should this be treated as not a bug?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I think the correct fix here is to remove the useless null check here:
> if (presContext && presContext->GetDocShell() && isActiveLayerManager) {
We always promise the return value of PresContext() is not null, but Coverity deduces that presContext could be null from this line.

Comment 5

[Andi [:andi]]

yes this is what i was thinking as well.

Comment 6

[Andi [:andi]]

Attachment: Bug 1232635.diff

Comment 7

[Andi [:andi]]

Comment on attachment 8699376 [details] [diff] [review]
Bug 1232635.diff

Review of attachment 8699376 [details] [diff] [review]:
-----------------------------------------------------------------

As David is probably very busy

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8699376 [details] [diff] [review]
Bug 1232635.diff

r=me, but Xidorn could totally have reviewed this.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

But I'm totally not a peer :/

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, but I am, and I just totally delegated you to review patches like that if you're comfortable with them.  ;)

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

OK, hopefully I won't ruin the code :)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9e7c8584256

Comment 13

[Carsten Book [:Tomcat]]

backout for test failures like https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ac829c36bb

Comment 14

[Andi [:andi]]

This is only a null check and should't this be the issue.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> backout for test failures like https://hg.mozilla.org/integration/mozilla-inbound/rev/b6ac829c36bb

That's not a link to a test failure... what were the actual failures?  I'm not seeing them on https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=88e44bfba483 but maybe they only showed up later?

Comment 16

[Carsten Book [:Tomcat]]

Hi Boris, it was for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=21413336&repo=mozilla-inbound

see https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Linux%20opt%20Reftest%20e10s%20Crashtest%20e10s%20R-e10s%28C%29&fromchange=99bef2f7a333&tochange=88e44bfba483&selectedJob=21413336

Comment 17

[Andi [:andi]]

I think the crash is associated with this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1247122

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5336b4eb939

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5336b4eb939