Last Comment Bug 404215 - "ASSERTION: Dead placeholder in placeholder map" with -moz-column, position: absolute
: "ASSERTION: Dead placeholder in placeholder map" with -moz-column, position: ...
Status: RESOLVED FIXED
[sg:critical?]
: assertion, crash, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: P2 critical (vote)
: ---
Assigned To: fantasai
:
Mentors:
Depends on:
Blocks: stirdom 369230 379349 399407 408602 408737 414255 423098
  Show dependency treegraph
 
Reported: 2007-11-17 22:08 PST by Jesse Ruderman
Modified: 2013-03-31 15:54 PDT (History)
12 users (show)
dsicore: blocking1.9-
fantasai.bugs: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (383 bytes, text/html)
2007-11-17 22:08 PST, Jesse Ruderman
no flags Details
testcase 2 (crash) (885 bytes, text/html)
2008-01-10 16:07 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 3 (assert) (1.01 KB, text/html)
2008-01-10 16:12 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 4 (hang) (717 bytes, text/html)
2008-01-10 16:17 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 5 (rendering issue) (353 bytes, text/html)
2008-01-14 14:07 PST, Daniel Holbert [:dholbert]
no flags Details
patch (5.08 KB, patch)
2008-01-17 15:48 PST, fantasai
no flags Details | Diff | Review
patch v2 (10.21 KB, patch)
2008-02-04 14:17 PST, fantasai
no flags Details | Diff | Review
IRC log of discussion (1.89 KB, text/plain)
2008-02-12 15:49 PST, fantasai
no flags Details
work in progress (61.80 KB, patch)
2008-02-26 07:16 PST, fantasai
no flags Details | Diff | Review
patch (95.48 KB, patch)
2008-02-28 19:37 PST, fantasai
no flags Details | Diff | Review
reftest.patch (5.63 KB, patch)
2008-02-29 15:05 PST, fantasai
no flags Details | Diff | Review
patch (11.46 KB, patch)
2008-03-17 23:28 PDT, fantasai
no flags Details | Diff | Review
patch (54.13 KB, patch)
2008-04-10 03:09 PDT, fantasai
no flags Details | Diff | Review
patch (no assertion, more reftests) (59.64 KB, patch)
2008-04-16 00:16 PDT, fantasai
roc: review+
roc: superreview+
mbeltzner: approval1.9+
Details | Diff | Review

Description Jesse Ruderman 2007-11-17 22:08:09 PST
Created attachment 289197 [details]
testcase (crashes Firefox when loaded)

Loading the testcase triggers three assertions and a crash.

###!!! ASSERTION: Placeholder relationship should have been torn down; see comments in nsPlaceholderFrame.h: '!shell->FrameManager()->GetPlaceholderFrameFor(mOutOfFlowFrame)', file /Users/jruderman/trunk/mozilla/layout/generic/nsPlaceholderFrame.cpp, line 132

###!!! ASSERTION: Dead placeholder in placeholder map: 'entry->placeholderFrame->GetOutOfFlowFrame() != (void*)0xdddddddd', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 136

###!!! ASSERTION: no placeholder frame: 'nsnull != placeholderFrame', file /Users/jruderman/trunk/mozilla/layout/generic/nsHTMLReflowState.cpp, line 1098

Null deref crash [@ nsHTMLReflowState::GetNearestContainingBlock].

Marking security-sensitive because of the second assertion.
Comment 1 Boris Zbarsky [:bz] 2007-11-18 12:49:41 PST
The first assert I see here is:

###!!! ASSERTION: overflow containers out of order or bad parent: '!(aOverflowCont->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER)', file ../../../mozilla/layout/generic/nsContainerFrame.cpp, line 1379

This happens a number of times, then I get:

###!!! ASSERTION: loop in frame list.  This will probably hang soon.: 'Error', file ../../../mozilla/layout/generic/nsFrameList.cpp, line 587

Then I in fact seem to hang.

fantasai, want to check this out?  Is this a regression from something it should be blocking?
Comment 2 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-11-19 07:44:41 PST
Related to bug 398332, perhaps?
Comment 3 Daniel Holbert [:dholbert] 2007-12-14 14:38:17 PST
I see the same asserts & crash that Jesse reported in Comment 0 -- not the behavior that bz described in comment 1.

Using fresh trunk checkout from today:
  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2007121412 Minefield/3.0b3pre
Comment 4 Jesse Ruderman 2008-01-10 15:30:28 PST
Increasing to P1 because this interferes with fuzzing.
Comment 5 Daniel Holbert [:dholbert] 2008-01-10 15:51:13 PST
Regression range is between these nightlies:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072504 Minefield/3.0a7pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072604 Minefield/3.0a7pre
Comment 6 Daniel Holbert [:dholbert] 2008-01-10 15:53:36 PST
regression range points to Bug 379349 (Add support for 'overflow containers')
Comment 7 Daniel Holbert [:dholbert] 2008-01-10 16:07:52 PST
Created attachment 296439 [details]
testcase 2 (crash)

Posting a set of three related testcases for this bug that I made a little while back.

This one (testcase 2) triggers the same assertions & crash as in comment 0 (like the original testcase.)
Comment 8 Daniel Holbert [:dholbert] 2008-01-10 16:12:43 PST
Created attachment 296442 [details]
testcase 3 (assert)

This one is effectively the same as testcase 2 without the "position: absolute".  (I also added a -moz-column-gap and background colors, for easier visualization)

This doesn't trigger the crash. Instead, we hit this assertion:
###!!! ASSERTION: frame was not removed from primary frame map before destruction or was readded to map after being removed: 'Not Reached', file /mozilla/layout/base/nsFrameManager.cpp, line 711
Comment 9 Daniel Holbert [:dholbert] 2008-01-10 16:17:16 PST
Created attachment 296445 [details]
testcase 4 (hang)

This testcase is like testcase 2, but...
 - with no "position: absolute"
 - with different height values
 - we don't call the "boom" function  (because we don't have to)

It triggers a hang, during which a ridiculously large frame tree is constructed.  I'll post a partial frame dump of this at some point.

I think this hang is caused by the same underlying issue, because it has the same regression range.
Comment 10 Daniel Holbert [:dholbert] 2008-01-11 15:36:32 PST
Actually,  it looks like backing out the patch for bug 399407 fixes all of this bug's testcases.
Comment 11 Daniel Holbert [:dholbert] 2008-01-11 17:11:17 PST
Also -- AFAICT, testcases 1 and 2 *don't* crash using latest-nightly

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011104 Minefield/3.0b3pre

but they *do* crash using a debug build from a checkout today.

I imagine this is probably because of some pointer that gets cleared in debug builds but not in optimized builds, so we hit a null deref in the debug build but not in the optimized build.
Comment 12 fantasai 2008-01-14 12:36:26 PST
Maybe instead of ignoring overflow containers when balancing we want to look at the overflow rect instead?
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-14 12:47:41 PST
What is the desired rendering of testcase 4?
Comment 14 Daniel Holbert [:dholbert] 2008-01-14 14:01:21 PST
(In reply to comment #13)
> What is the desired rendering of testcase 4?

Just a blank page, loaded in a reasonable amount of time.
(There are no borders or backgrounds or text to display)
Comment 15 Daniel Holbert [:dholbert] 2008-01-14 14:07:19 PST
Created attachment 297065 [details]
testcase 5 (rendering issue)

Testcase 5 shows part of what's going wrong and causing the hang in testcase 4: we're letting the content spill beyond the supposedly-limited number of columns.

 Desired Testcase 5 rendering: 3 columns
  Actual Testcase 5 rendering: 4 columns

If you make div c taller, you can get arbitrarily many columns in buggy builds.  (but it sticks to 3 columns if you back out bug 399407)
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-14 14:57:09 PST
Well yeah, I mean what is the desired CSS box geometry. It's not clear to me what it should be.
Comment 17 fantasai 2008-01-17 15:48:23 PST
Created attachment 297641 [details] [diff] [review]
patch

Here you go. This reverts the patch for bug 399407 and replaces it with a switch from GetSize() to GetOverflowRect() in the balancing code. AFAICT it passes all testcases here and in 399407 -- no crashes, hangs, or asserts. Awesome job with the investigation, dholbert -- writing this was easy given all your analysis!
Comment 18 Daniel Holbert [:dholbert] 2008-01-17 16:04:25 PST
(In reply to comment #17)
> Awesome job with the investigation, dholbert -- writing this was easy given all
> your analysis!

Great, I'm glad it helped and that you could patch it! I was getting a bit overwhelmed trying to wrap my mind around the inner workings of -moz-col. :)

> AFAICT it
> passes all testcases here and in 399407 -- no crashes, hangs, or asserts.

It'd be good to check if it fixes bug 408737, as well -- I think the testcases there are similar to testcase 4 (hang) on this bug.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-20 14:57:06 PST
I don't think using GetOverflowRect is the right thing here. What if the last child of a block is position:relative with top:100px?
Comment 20 fantasai 2008-01-22 23:48:29 PST
If it's an onmousedown dynamic switch between top: 100px; and something else, then I can see why we wouldn't want to rebalance the columns. But in most cases I think we do want to take overflow into account, and I don't think it makes sense to make an exception for this case.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-23 03:04:59 PST
If you have content where the last line always causes overflow (due to relative positioning or something as simple as an unusually large descender glyph), then our balancing algorithm is going to always see overflow, conclude that the content never fits, and therefore try to reduce the column height to approximately zero. That doesn't sound good.
Comment 22 fantasai 2008-01-23 16:35:00 PST
So.. I guess the question is, does layout make sure overflow always fits in the availableHeight? If not, then what we really want is PR_MIN(availableHeight for column, column's overflow rect).
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-23 18:42:36 PST
That's no good because then we're not detecting when the in-flow content doesn't fit in the available height.

What we really need here is some new reflow metric, the max of the YMosts of all blocks whose block formatting context is the column, or something like that. 
Comment 24 fantasai 2008-01-25 11:38:37 PST
Ok, so, how about
  PR_MAX(mRect.height, PR_MIN(availableHeight, overflow.height));
? I *think* that should solve this bug without breaking the overflowing content detection insofar as it already exists.

It won't handle detecting in-flow content that doesn't fit if that content is in the overflow: we'll need to add a ymost metric to the frames for that.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-25 22:52:28 PST
Maybe we should just implement a function that computes that by scanning through the frames in the column, and use it.
Comment 26 fantasai 2008-01-26 10:54:07 PST
Because it will have to recurse into descendants to check floats and overflow containers, that's not going to be a simple computation. You could maybe run it after a tentative balancing height has been computed.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-01-26 12:26:28 PST
It's not complex I think, but it might be slow-ish. We could run it only when the column's overflow-YMost is greater than its border-box height, because we know the "max of the YMosts of all blocks whose block formatting context is the column" must be less than or equal to the overflow-YMost.
Comment 28 fantasai 2008-02-04 14:17:21 PST
Created attachment 301366 [details] [diff] [review]
patch v2

Patch implementing YMost scanning function. Let me know if this is the right approach.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-04 14:49:58 PST
+    return rectHeight;
+  }
+  else {

Don't do else after return.

+struct YMostAnalysisData {
+  nscoord mOriginOffset;
+  nscoord mYMost;

Document what invariants govern these fields

+    return PR_FALSE;
+  }
+  else {

No else after return

+  data->mYMost = PR_MAX(data->mOriginOffset + aFrame->GetRect().height,

This is wrong right? It's not including GetRect().y but it should?

+  nscoord rectHeight = aFrame->GetRect().height;
+  nscoord overflowHeight = aFrame->GetOverflowRect().height;
+  if (rectHeight >= overflowHeight) {
+    return rectHeight;
+  }

Probably not even worth doing this optimization here, might as well do it only during the recursive analysis.

MapSubtree's not really needed, I'd just do it all directly.
Comment 30 fantasai 2008-02-12 15:49:56 PST
Created attachment 302926 [details]
IRC log of discussion

roc and I talked about some of the more complicated issues we need to work around here (like needing to undo relative positioning) and came to the conclusion that we're best off storing ymost data on the frames. The idea is to make it part of the overflowRect struct and only store it when it doesn't match mRect.height.
Comment 31 fantasai 2008-02-26 07:16:02 PST
Created attachment 305764 [details] [diff] [review]
work in progress
Comment 32 fantasai 2008-02-27 12:55:16 PST
roc, storing the overflowRect and contentBottom in one datastruct is a mess. I'm going to keep them separate and just share the flag. Since contentBottom usually exists only when the overflowRect != mRect, GetOverflowRect will get very few false positives on whether it needs to retrieve data from the proptable; GetContentBottom will have more, but it's only checked about once per frame during reflow.

The patch for this seems to fix this bug, the crash in bug 408602, bug 408737, and bug 414255. It'll need a slew of reftests, though.
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-27 12:57:14 PST
Is it still work in progress or do you want me to review it?
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-02-27 12:58:12 PST
Wait, you're adding mContentBottom to all frames in nsIFrame.h? Is that intentional?
Comment 35 fantasai 2008-02-27 13:33:24 PST
Still work in progress. The patch is a mess; I'm cleaning up the mess. :)

mContentBottom was intentional at that point: I used it so I had reliable storage while I fixed the calculations. The final patch will optimize the storage by using the proptable.
Comment 36 fantasai 2008-02-28 19:37:48 PST
Created attachment 306433 [details] [diff] [review]
patch
Comment 37 fantasai 2008-02-29 15:05:24 PST
Created attachment 306607 [details] [diff] [review]
reftest.patch

nsTableCellFrame.cpp
   GetSelfOverflow(desiredSize.mOverflowArea);
-  ConsiderChildOverflow(desiredSize.mOverflowArea, firstKid);
+  ConsiderChildOverflow(desiredSize, firstKid);
   FinishAndStoreOverflow(&desiredSize);

There's a missing
  SetContentBottom(desiredSize.GetContentBottom());
call there. With that, these reftests all pass.
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-03-02 15:06:49 PST
+                  nsHTMLReflowMetrics*     aMetrics = nsnull);

This needs documentation --- and a better name --- to make it clear this is the *container's* metrics into which child overflow should be accumulated.

+      if (line->IsBlock())
+        aMetrics.MergeContentBottom(line->mFirstChild->GetContentBottom());
+      else
+        aMetrics.MergeContentBottom(line->mBounds.YMost());
+    }

Slightly better if you set "nscoord bottom = line->IsBlock() ? ... : ...; aMetrics.MergeContentBottom(...);"

+  mFrame->SetContentBottom(mFrame->GetContentBottom() - y);

What is going on here? We should avoid getting block's y-offset into its bottom-value in the first place. Seems like we should be adding the frame-y in a lot of MergeContentBottom calls. I'm confused.

I'm not sure how you're handling relatively positioned blocks, but it's tied up with the previous issue.

+   * Returns a y coordinate relative to this frame's origin that represents
+   * the logical bottom of the frame or its visible content, whichever is lower.
+   * Relative positioning is ignored and margins and glyph bounds are not
+   * considered.

I think you need to be more precise here about what "visible content" means.

Do we really have to include abs-pos frames in the bottom calculation? That seems to lead to problems where the abs-pos frame is positioned using 'bottom' to be overflowing the bottom of the container, no matter what the container's height is.

+  void SetContentBottom(nscoord aYMost)
+  {
+    if (&height == mContentBottom)
+      mContentBottom = new nscoord;
+    *mContentBottom = aYMost;
+  }

Avoid the dynamic allocation, either use a spare nscoord allocated in the nsHTMLReflowMetrics itself, or better still just use a flag to tell whether the value is the height or not. Although actually I don't understand the need for this, why not just set mContentBottom to zero initially and have GetContentBottom return PR_MAX(height, mContentBottom)?

BIG QUESTION:

We store the before-relative-positioning offset of blocks in nsGkAtoms::computedOffsetProperty for the frame. If we did the same for relatively positioned inlines (pretty easy to do), would the original "compute bottom value by scanning the frame tree" work reasonably simply? Because this approach is adding quite a lot of code.
Comment 39 fantasai 2008-03-02 23:15:57 PST
> What is going on here?

"Undoing" relative positioning. You're right, a lot of MergeContentBottom calls are missing the y value.

> I think you need to be more precise here about what "visible content" means.

Not clipped. I'll clarify that.

> Do we really have to include abs-pos frames in the bottom calculation?

Yes, because abspos content generates overflow columns if it doesn't fit.

> problems where the abs-pos frame is positioned using 'bottom' to be
> overflowing the bottom of the container, no matter what the container's
> height is.

We shouldn't be including abspos frames for which the colset is the containing block. Looks like that's not something I'm paying enough attention to...

> why not just set mContentBottom to zero initially and have
> GetContentBottom return PR_MAX(height, mContentBottom)?

Undoing relative positioning can cause it to be less than the height. I could have GetContentBottom() check nsGkAtoms::computedOffsetProperty if you want to avoid the height/mContentBottom gymnastics.

> If ... would the original "compute bottom value by scanning the frame tree"
> work reasonably simply?

Dunno. I'll try it and we'll see. BTW, computedOffsetProperty stores the relpos offsets, not the original position, right?
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-03-03 00:29:03 PST
It stores the offsets, right.

You'd want to create a helper method that gives you the frame origin ignoring relative positioning, that you can call from your code and from nsBlockReflowState::FlowAndPlaceFloat.
Comment 41 fantasai 2008-03-17 23:28:16 PDT
Created attachment 310178 [details] [diff] [review]
patch
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-03-21 21:08:19 PDT
+  const nsStyleDisplay* displayStyle = GetStyleDisplay();
+  if (NS_STYLE_POSITION_RELATIVE == displayStyle->mPosition) {
+    nsPoint *offsets = static_cast<nsPoint*>
+                         (GetProperty(nsGkAtoms::computedOffsetProperty));
+    if (offsets) {
+      return *offsets;
+    }
+  }

Why not just unconditionally look for the property? Or at least, if you want nsBlockReflowState to not suffer, take nsStyleDisplay as a parameter.

+      if (childList == nsnull && aFrame->GetType() == nsGkAtoms::blockFrame) {

Use GetAsBlock, there are areaFrames and table captions that fail this check

I don't see any code that is setting computedOffsetProperty for rel-pos inline frames?

Also you forgot to include your tests in the patch.

+      else if (childList != nsGkAtoms::overflowList &&
+          childList != nsGkAtoms::excessOverflowContainersList)

You need to exclude the overflowOutOfFlows list here too

+  nscoord contentBottom = aFrame->GetRect().height + aOffset;
+
+  if (aFrame->GetOverflowRect().height > contentBottom) {

This can't be right, the first line includes aOffset but the overflow rect doesn't.

Why are we taking offset as a parameter here and always adding it, anyway?

+      if (nsLayoutUtils::CalculateContentBottom(child) > aConfig.mColMaxHeight) {
+        allFit = PR_FALSE;
+      }
     }
 
     contentRect.UnionRect(contentRect, child->GetRect());
 
     ConsiderChildOverflow(overflowRect, child);
+    contentBottom = PR_MAX(contentBottom,
+                           nsLayoutUtils::CalculateContentBottom(child));

Avoid calling CalculateContentBottom(child) twice?
Comment 43 Mike Schroepfer 2008-04-07 17:05:58 PDT
Given the lack of motion I'm going to move this to the wanted-next list - please re-nom/yell if you think this positively must block
Comment 44 Mike Schroepfer 2008-04-07 17:06:49 PDT
Er - just noticed the dependency on other blockers - so I'm keeping this on the list..
Comment 45 Damon Sicore (:damons) 2008-04-09 15:14:30 PDT
This is a fuzz test issue, there many like it, and after discussing with roc and dbaron, we won't block on it.  Request approval if the patch gets reviewed in time.
Comment 46 fantasai 2008-04-10 03:09:31 PDT
Created attachment 314819 [details] [diff] [review]
patch

> Why not just unconditionally look for the property? Or at least, if you want
> nsBlockReflowState to not suffer, take nsStyleDisplay as a parameter.

Done.

> Use GetAsBlock, there are areaFrames and table captions that fail this check

Done.

> I don't see any code that is setting computedOffsetProperty for rel-pos
> inline frames?

They wouldn't be used for this patch, but I shifted the rel-pos storing code
to nsHTMLReflowState anyway.

> Also you forgot to include your tests in the patch.

Added.

> You need to exclude the overflowOutOfFlows list here too

Done.

> This can't be right, the first line includes aOffset but the overflow rect
> doesn't. Why are we taking offset as a parameter here and always adding it,
> anyway?

Because I'm confused. :/ Removed.

> Avoid calling CalculateContentBottom(child) twice?

Added caching within ReflowChildren and between ReflowChildren and Reflow.

I need to do some more testing (reftests pass), I'll flag you when I'm done. Do we have good reftests for column balancing?
Comment 47 Boris Zbarsky [:bz] 2008-04-10 09:27:27 PDT
> +  NS_PRECONDITION(aFrame->GetType() == nsGkAtoms::blockFrame,
                     "aFrame must be block frame");

Why is this assertable?  How do you know it's not a caption or an areaFrame or something like that?

Comment 48 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-04-10 15:09:44 PDT
Yeah, we should use GetAsBlock there too. fantasai, did you mean to mark that patch for review?
Comment 49 fantasai 2008-04-10 17:06:18 PDT
Not yet, I need to do some more testing (and remove that assert). Do we have good reftests for column balancing, or do I need to write some?
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-04-10 17:25:07 PDT
You probably need to write some, sorry :-(
Comment 51 fantasai 2008-04-16 00:16:42 PDT
Created attachment 315919 [details] [diff] [review]
patch (no assertion, more reftests)

(Still need to convert the testcases in this and associated bugs to reftests/crashtests, though.)
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-04-16 17:56:58 PDT
Comment on attachment 315919 [details] [diff] [review]
patch (no assertion, more reftests)

+          // mColMaxHeight is feasible.

colData.mMaxHeight

This looks great!
Comment 53 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-17 11:56:20 PDT
Comment on attachment 315919 [details] [diff] [review]
patch (no assertion, more reftests)

a1.9=beltzner

Note You need to log in before you can comment on or make changes to this bug.