1234554 - [Static Analysis][Dereference after null check] In function nsDocument::GetViewportInfo from nsDocument.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that a null pointer derefence mai occure before after null check for context, it is checked here:

>>  float fullZoom = context ? context->DeviceContext()->GetFullZoom() : 1.0;
>>  fullZoom = (fullZoom == 0.0) ? 1.0 : fullZoom;

and dereferenced here:

>>  CSSToLayoutDeviceScale layoutDeviceScale = context->CSSToDevPixelScale();

This is not normal because in the case context is null we will haev a null dereference. I suggest removing the line, since variable layoutDeviceScale  is not used in the current context.

Comment 1

[Andi [:andi]]

(In reply to Bogdan Postelnicu from comment #0)
> The Static Analysis tool Coverity added that a null pointer derefence mai
> occure before after null check for context, it is checked here:
> 
> >>  float fullZoom = context ? context->DeviceContext()->GetFullZoom() : 1.0;
> >>  fullZoom = (fullZoom == 0.0) ? 1.0 : fullZoom;
> 
> and dereferenced here:
> 
> >>  CSSToLayoutDeviceScale layoutDeviceScale = context->CSSToDevPixelScale();
> 
> This is not normal because in the case context is null we will haev a null
> dereference. I suggest removing the line, since variable layoutDeviceScale 
> is not used in the current context.

We can modify layoutDeviceScale with:

CSSToLayoutDeviceScale layoutDeviceScale = context ? CSSToLayoutDeviceScale(1) : context->CSSToDevPixelScale();

Comment 2

[Andi [:andi]]

Attachment: Bug 1234554.diff

Comment 3

[Andi [:andi]]

Attachment: Bug 1234554.diff

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 8701068 [details] [diff] [review]
Bug 1234554.diff

>-  CSSToLayoutDeviceScale layoutDeviceScale = context->CSSToDevPixelScale();
>+  CSSToLayoutDeviceScale layoutDeviceScale = context ? CSSToLayoutDeviceScale(1) : context->CSSToDevPixelScale();

That check is backwards, no? Or am I missing something obvious?

r- until we sort that out. Thanks!

Comment 5

[Andi [:andi]]

Attachment: Bug 1234554.diff

Yes completely backwards! I've attached the right one.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 8701363 [details] [diff] [review]
Bug 1234554.diff

That makes much more sense! :) r=jst

Comment 7

[Andi [:andi]]

Thanks for pointing the obvious :)

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88a2ffb5920e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88a2ffb5920e