[Static Analysis][Dereference null return value] In function nsView::DidCompositeWindow from nsView.cpp

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla46
coverity
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: CID 1346071 )

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
The Static Analysis tool Coverity added that context->GetDisplayRootPresContext() can be null thus having later on a null pointer dereference. 
The path that the analyzer takes in GetDisplayPresContext is:

>>nsPresContext::GetDisplayRootPresContext()
>>{
>>  nsPresContext* pc = this;
    	1. Condition true, taking true branch
>>  for (;;) {
>>    nsPresContext* parent = pc->GetParentPresContext();
    	2. Condition !parent, taking true branch
>>    if (!parent) {
>>      // Not sure if this is always strictly the parent, but it works for GetRootPresContext
>>      // where the current pres context has no frames.
>>      nsIDocument *doc = pc->Document();
    	3. Condition doc, taking true branch
>>      if (doc) {
>>        doc = doc->GetParentDocument();
    	4. Condition doc, taking true branch
>>        if (doc) {
>>          nsIPresShell* shell = doc->GetShell();
    	5. Condition shell, taking false branch
>>          if (shell) {
>>            parent = shell->GetPresContext();
>>          }
>>        }
>>      }
>>    }
    	6. Condition !parent, taking true branch
>>    if (!parent || parent == pc)
    	7. Breaking from loop
>>      break;
>>    pc = parent;
>>  }
    	8. Condition pc->IsRoot(), taking false branch
    	9. return_null: Explicitly returning null.
>>  return pc->IsRoot() ? static_cast<nsRootPresContext*>(pc) : nullptr;
>>}

And the dereference will occur:
>> context->GetDisplayRootPresContext()->GetRootPresContext()->NotifyDidPaintForSubtree(nsIPresShell::PAINT_COMPOSITE);

If we are 100% sure that this scenario is not possible we could wave the issue on Coverity side and mark it as a false positive, otherwise we could assert the result of this function, that will later trigger when automation tests are ran.
Looking on crash-stats i don't find any issues related with the above one but still, an assert doesn't hurt.
(Assignee)

Comment 1

3 years ago
Created attachment 8704593 [details] [diff] [review]
Bug 1237227.diff
Attachment #8704593 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 3

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/577396a6287e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.