Closed Bug 1360157 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kanru, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1405541)

Attachments

(1 file)

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)
Whiteboard: CID 1405541
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)
Assignee: nobody → emilio+bugs
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+
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
https://hg.mozilla.org/mozilla-central/rev/c6537f15d2ea
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.