Closed Bug 1234554 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1324682 )

Attachments

(1 file, 2 obsolete files)

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.
(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();
Attached patch Bug 1234554.diff (obsolete) — Splinter Review
Attachment #8701067 - Flags: review?(jst)
Attached patch Bug 1234554.diff (obsolete) — Splinter Review
Attachment #8701067 - Attachment is obsolete: true
Attachment #8701067 - Flags: review?(jst)
Attachment #8701068 - Flags: review?(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!
Attachment #8701068 - Flags: review?(jst) → review-
Attached patch Bug 1234554.diffSplinter Review
Yes completely backwards! I've attached the right one.
Attachment #8701068 - Attachment is obsolete: true
Attachment #8701363 - Flags: review?(jst)
Comment on attachment 8701363 [details] [diff] [review]
Bug 1234554.diff

That makes much more sense! :) r=jst
Attachment #8701363 - Flags: review?(jst) → review+
Thanks for pointing the obvious :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88a2ffb5920e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.