[Static Analysis][Coverity] layout/base/nsCSSFrameConstructor.cpp: 8543 Null pointer dereferences (FORWARD_NULL)

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kanru, Assigned: emilio)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla55
coverity
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: CID 1405541)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
In bug 1355351 a for loop is replaced by a while loop

-    for (; ancestor; ancestor = ancestor->GetParent()) {
-      ancestorFrame = ancestor->GetPrimaryFrame();
-      if (ancestorFrame) {
-        break;
-      }
+    while (!ancestor->GetPrimaryFrame()) {
+      // FIXME(emilio): Should this use the flattened tree parent instead?
+      ancestor = ancestor->GetParent();
+      MOZ_ASSERT(ancestor, "we can't have a display: contents subtree root!");

It also removed the null check embedded in the for loop. Coverity caught this:

*** CID 1405541:  Null pointer dereferences  (FORWARD_NULL)
/layout/base/nsCSSFrameConstructor.cpp: 8543 in nsCSSFrameConstructor::ContentRemoved(nsIContent *, nsIContent *, nsIContent *, nsCSSFrameConstructor::RemoveFlags, bool *, nsIContent **)()
8537         // Remove it once that's fixed.
8538         ClearUndisplayedContentIn(aChild, aContainer);
8539       }
8540       MOZ_ASSERT(!childFrame || !GetDisplayContentsStyleFor(aChild),
8541                  "display:contents nodes shouldn't have a frame");
8542       if (!childFrame && GetDisplayContentsStyleFor(aChild)) {
>>>     CID 1405541:  Null pointer dereferences  (FORWARD_NULL)
>>>     Assigning: "ancestor" = "aContainer".
8543         nsIContent* ancestor = aContainer;
8544         while (!ancestor->GetPrimaryFrame()) {
8545           // FIXME(emilio): Should this use the flattened tree parent instead?
8546           ancestor = ancestor->GetParent();
8547           MOZ_ASSERT(ancestor, "we can't have a display: contents subtree root!");
8548         }

If aContainer can be null, we should add the null check back.
Flags: needinfo?(emilio+bugs)
(Reporter)

Updated

2 years ago
Blocks: 1230156
Whiteboard: CID 1405541
(Assignee)

Comment 1

2 years ago
My idea is that that null check isn't needed. A display contents child always has an ancestor, given we blockify the root.

It's the same that allow us to assert ancestor in the bottom of the loop.

Let me assert that.
Flags: needinfo?(emilio+bugs)
Comment hidden (mozreview-request)
Assignee: nobody → emilio+bugs
status-firefox55: --- → affected
status-firefox57: affected → ---

Comment 3

2 years ago
mozreview-review
Comment on attachment 8862361 [details]
Bug 1360157: Assert that a display: contents child always has a parent.

https://reviewboard.mozilla.org/r/134290/#review137438
Attachment #8862361 - Flags: review?(mats) → review+

Comment 4

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c6537f15d2ea
Assert that a display: contents child always has a parent. r=mats

Comment 5

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