Last Comment Bug 341858 - Crash [@ GetNifOrSpecialSibling] with display: table-caption and display: table-row-group;
: Crash [@ GetNifOrSpecialSibling] with display: table-caption and display: tab...
Status: VERIFIED FIXED
[sg:critical?] regression from bug 30...
: crash, regression, testcase, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: 309322 330981 337476 343270 347898
  Show dependency treegraph
 
Reported: 2006-06-17 02:36 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2011-06-13 10:01 PDT (History)
12 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
caillon: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes on load or second load) (232 bytes, text/html)
2006-06-17 02:37 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
original file from which testcase is derived from (19.71 KB, text/html)
2006-06-17 02:41 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Inbetween original file and testcase (9.96 KB, text/html)
2006-06-17 02:44 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Patch rev. 1 (7.23 KB, patch)
2006-07-08 06:50 PDT, Mats Palmgren (:mats)
bernd_mozilla: review+
bzbarsky: superreview-
Details | Diff | Review
Content+frame dumps with Patch rev. 1 (10.74 KB, text/html)
2006-07-08 06:51 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev2 (7.52 KB, patch)
2006-10-19 07:56 PDT, Bernd
no flags Details | Diff | Review
rev2 that even compiles (6.52 KB, patch)
2006-10-19 10:04 PDT, Bernd
bzbarsky: superreview+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-06-17 02:36:48 PDT
See upcoming testcase, which crashes current trunk Mozilla builds on load.
It could be that it's only crashing after second load.

It consists of this:
<table style="display: table-caption;">
<keygen style="display: table-caption;">
<span style="display: table-caption;">
<span style="display: table-row-group;">
<body style="display: table-row-group;">
<input>

Talkback ID: TB19965177Z
GetNifOrSpecialSibling   nsCSSFrameConstructor::FindFrameWithContent   nsCSSFrameConstructor::FindPrimaryFrameFor   nsFrameManager::GetPrimaryFrameFor   nsCSSFrameConstructor::FindPrimaryFrameFor   nsFrameManager::GetPrimaryFrameFor   nsCSSFrameConstructor::FindPrimaryFrameFor   nsFrameManager::GetPrimaryFrameFor   nsGenericElement::GetPrimaryFrameFor   nsGenericHTMLElement::GetFormControlFrame   nsHTMLInputElement::SaveState   0xed6ccfb8

This regressed between 2005-11-30 and 2005-12-15. Ria, you have builds inbetween to look for a narrower regression range?
Comment 1 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-06-17 02:37:51 PDT
Created attachment 225977 [details]
testcase (crashes on load or second load)
Comment 2 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-06-17 02:41:02 PDT
Created attachment 225979 [details]
original file from which testcase is derived from

This is the original file from which the testcase is derived from.

Talkback ID: TB19965429M
nsGenericHTMLElement::SetScrollLeft   nsGenericHTMLElementTearoff::SetScrollLeft   XPCWrappedNative::CallMethod  
As you can see, it's crashing in quite some different area. Not sure what that means.
Comment 3 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-06-17 02:44:23 PDT
Created attachment 225980 [details]
Inbetween original file and testcase

With this file I crash also in a different part of the code.
Talkback ID: TB19965484Z
nsCachedStyleData::GetStyleData   nsGenericHTMLElement::GetOffsetRect   nsGenericHTMLElement::GetOffsetHeight   nsGenericHTMLElementTearoff::GetOffsetHeight   XPCWrappedNative::CallMethod
Comment 4 Ria Klaassen (not reading all bugmail) 2006-06-17 03:19:27 PDT
Regression between 1.9a1_2005120305 and 1.9a1_2005120315.
Can't reproduce the crash in the first build at least.
Comment 5 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-06-17 03:24:13 PDT
Thanks, Ria! I guess bug 309322 is the 'guilty' one. 
Comment 6 Mats Palmgren (:mats) 2006-07-01 02:30:30 PDT
I'm seeing some weird looking stacks here and in bug 343270 so I'm
marking this Security-Sensitive for now...

I have a fix for this bug and bug 343270 (which is similar) and it also
fixes the assertions in bug 337476.
I need sleep before attempting to explain it... (table frame construction)
Comment 7 Bernd 2006-07-01 05:20:27 PDT
> I need sleep before attempting to explain it... (table frame construction)

this is a well known problem ;-)
Comment 8 Bernd 2006-07-08 00:07:54 PDT
Mats, could you please attach the patch even if it is half baked. I think I hit the issue with a less severe case at bug 343946. I really need the patch to advance with  bug 339128.
Comment 9 Mats Palmgren (:mats) 2006-07-08 06:50:21 PDT
Created attachment 228528 [details] [diff] [review]
Patch rev. 1

We're trying to insert a SELECT. In ContentInserted() we find an insertion
point which happens to have a CaptionFrame and thus |parentFrame| is the
outer table frame when we call ConstructFrame() for the child.
I guess this is reasonable since we haven't resolved the child's style yet
so we can't say much about the final frame for the child.

Fix: make AdjustParentFrame() deal with the outer table case, adjusting
to the inner table as needed and then look at the result in
ContentInserted() adjusting the insertion point data accordingly.
I'll attach a content dump + frame dump before and after ContentInserted()
so you can see the result with this patch. I think the frame tree is the
correct one.
(I also removed |tempItems| because it's not needed)
Comment 10 Mats Palmgren (:mats) 2006-07-08 06:51:26 PDT
Created attachment 228529 [details]
Content+frame dumps with Patch rev. 1
Comment 11 Bernd 2006-07-11 12:17:54 PDT
I understand the first chunk and think it is correct. You have however to null check the new parent frame. I have seen several occasions where the outer table was reflown without an inner. I've put there a 0 deref stop and I would like to see here one too. I would like to avoid the next fire drill. You might add an assert however. 

I don't get the 

+  // If the final parent frame (decided by AdjustParentFrame()) is different
+  // from the parent of the insertion point we calculated above then
+  // parentFrame/prevSibling/appendAfterFrame are now invalid (bug 341858).
+  if (frameItems.childList &&
+      frameItems.childList->GetParent() != parentFrame) {
+    prevSibling = nsnull;
+    isAppend = PR_TRUE;
+    parentFrame =
+      ::AdjustAppendParentForAfterContent(mPresShell->GetPresContext(),
+                                          aContainer,
+                                          frameItems.childList->GetParent(),
+                                          &appendAfterFrame);
+  }
+
part. Wouldn't we hit this thing with normal  (pseudo) frame construction also 
when we execute in nsCSSFrameConstructor::AdjustParentFrame(nsFrameConstructorState&     aState,

aParentFrame = aState.mPseudoFrames.mCellInner.mFrame; ? 

If so, why did this not cause issues before?

The problem here is that in former times we did forbid the creation of a second caption, however this causes crashes with fixed pos etc. Now that a caption can have a sibling content we need to put them sometimes on a different place.
Comment 12 Mats Palmgren (:mats) 2006-07-11 16:02:58 PDT
(In reply to comment #11)
> I understand the first chunk and think it is correct.
> You have however to null check the new parent frame.

I'll add an assertion. I think we should maintain the
invariant that an outer table frame always has an inner.


> I have seen several occasions where the outer table
> was reflown without an inner.

Please file a bug and CC me when that occurs.


> + // If the final parent frame (decided by AdjustParentFrame()) is different
> + // from the parent of the insertion point we calculated above then
>
> Wouldn't we hit this thing with normal (pseudo) frame construction also ...
> If so, why did this not cause issues before?

In case we create pseudo frames |frameItems.childList| is the top pseudo,
otherwise it's just the primary frame for the child.
In both cases |frameItems.childList->GetParent()| should be |parentFrame|.
This is the case uptil now. This patch adds the possibility that the new
frames (pseudo or not) is actually created in the inner table frame when the
original |parentFrame| is an outer table frame (which would only occur
in the specific case we have in this bug: trying to add a sibling caption,
in all other cases ConstructFrame() will be given the inner table frame and
we will "adjust" captions after the fact by inserting them in the outer).

I could have missed something though, so please elaborate if you think
there is a case where this is not true.

I can add the following assertion in the if-block if you want:

NS_ASSERTION(parentFrame->GetType() == nsLayoutAtoms::tableOuterFrame &&
  frameItems.childList->GetParent()->GetType() == nsLayoutAtoms::tableFrame,
  "bad parent frame change");
Comment 13 Boris Zbarsky [:bz] 2006-07-16 16:28:24 PDT
So wait.  Isn't the real issue that we end up with the caption as a "sane" prevsibling or nextsibling for the select?  In my opinion this is wrong.  Maybe we should just special-case this and not use the prevsibling to get the parent if the type of the prevsibling is tableCaptionFrame?  Would that fix things too?  Seems like it'd be simpler.

That said, it seems like nsCSSFrameConstructor::IsValidSibling should handle that already.  How is it failing?
Comment 14 Boris Zbarsky [:bz] 2006-08-13 13:39:03 PDT
Wait.  Is this trunk-only?  On trunk, IsValidSibling is semi-busted.  See bug 347898 comment 7.  So maybe this patch is indeed the right way to go.
Comment 15 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-14 05:54:59 PDT
(In reply to comment #14)
> Wait.  Is this trunk-only?

The testcases don't crash on the latest 1.8.1 build or 1.8.0.6 build, only trunk builds.

Comment 16 Bernd 2006-08-20 22:42:28 PDT
Only trunk accepts multiple captions, all branches have the double caption supression which has its own merrits (aka crash bugs)
Comment 17 Boris Zbarsky [:bz] 2006-08-21 08:03:50 PDT
Bernd, what do you think of this patch?  And my comments in bug 347898 comment 7?
Comment 18 Bernd 2006-08-21 09:42:12 PDT
Sorry for the delay but I need to debug this first to understand whats going on. However this will go after bug 347725 which is regression crash on the 1.8.1 branch.
Comment 19 Bernd 2006-08-22 12:32:42 PDT
>bz: On trunk, IsValidSibling is semi-busted.

Hmm regardless how many beer I have, I can't understand what is not busted in this function. Its plain wrong to me. It uses display instead of frame types, it does some fancy caching of things it shouldn't cache. Reading back bug 136848 where the whole was introduced is pretty entertaining. My bet is that the function is not fixable. It uses style before xbl resolution. Its completely special content agnostic. This was good enough to get NS6 out of the door but I guess it has done its duties. 

So I guess, the way Mats is heading is correct. Adjust the parent once we know (after xbl) where we are.
Comment 20 Boris Zbarsky [:bz] 2006-08-22 21:22:55 PDT
> Its plain wrong to me.

OK, I buy that too.  The problem is that we're determining the parentFrame before we resolve XBL stuff, and determining the parentFrame involves doing this IsValidSibling stuff, right?  Or some equivalent?
Comment 21 Bernd 2006-08-23 23:08:36 PDT
Comment on attachment 228528 [details] [diff] [review]
Patch rev. 1

Boris just head over to bug 136848 and look if you would r/sr the original patch there. (All my experience shows that the patch would be faster r-'ed than written).
IsValidSibling (
http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#8548)
is a misnomer, its just Chris'HackToGetTheTableParentRight stuffed later with the fieldset legend, which is  very similiar to tables as Chris wrote it. My prediction is that Mats approach makes the table part in IsValidSibling obsolete if not we should move the logic at the place where Mats suggested to adjust the parent. With that in mind I would prefer if we split the first part of the patch into a seperate function that would later harbor the functionality of IsvalidSibling.
Comment 22 Boris Zbarsky [:bz] 2006-08-23 23:26:12 PDT
OK.  I'll try to review this in detail ASAP.  Thanks for looking into it!
Comment 23 Boris Zbarsky [:bz] 2006-09-18 23:26:13 PDT
Comment on attachment 228528 [details] [diff] [review]
Patch rev. 1

>Index: layout/base/nsCSSFrameConstructor.cpp
>+  PRBool childIsSpecialContent =
>+    IsSpecialContent(aChildContent, aTag, aNameSpaceID, aChildStyle);

So... IsSpecialContent() is not exactly cheap.  I'd like to avoid calling it if we can.  Could we just call it lazily (and keep a boolean that indicates we don't need to call it again or something)?  In particular, I'd like to not have IsSpecialContent() slowing down all the non-table codepaths that go through here.

>+    aParentFrame = aParentFrame->GetFirstChild(nsnull);

I'd prefer aParentFrame->GetContentInsertionFrame(), I think....

>@@ -9457,25 +9467,24 @@ nsCSSFrameConstructor::ContentInserted(n
>+  ConstructFrame(state, aChild, parentFrame, frameItems);

We should probably assert here that frameItems.firstChild == frameItems.lastChild, since we assume it in this code.

>+      frameItems = nsFrameItems();

Or frameItems.RemoveChild(frameItems.firstChild), though that may actually be slower...

>+    prevSibling = nsnull;
>+    isAppend = PR_TRUE;

So... why do we suddenly decide this is an append?  That doesn't make much sense to me.  I guess it's an easy fallback if we feel that all such cases are error cases anyway or something... But it feels like with this code it's easy to have the same DOM have two very different frame structures depending on the order of DOM operations to get to it, which is a little odd to me....
Comment 24 Bernd 2006-10-03 09:31:12 PDT
Mats any news on this? Or shall I take and fix bz's comments?
Comment 25 Bernd 2006-10-19 07:55:22 PDT
>>Index: layout/base/nsCSSFrameConstructor.cpp
>>+  PRBool childIsSpecialContent =
>>+    IsSpecialContent(aChildContent, aTag, aNameSpaceID, aChildStyle);
> 
> So... IsSpecialContent() is not exactly cheap.  I'd like to avoid calling it if
> we can.  Could we just call it lazily (and keep a boolean that indicates we
> don't need to call it again or something)?  In particular, I'd like to not have
> IsSpecialContent() slowing down all the non-table codepaths that go through
> here.

fixed, now we call IsSpecialContent() only if the parent is outer table frame, which shall be very seldom and if so we reuse the flag.

>>+    aParentFrame = aParentFrame->GetFirstChild(nsnull);
> 
> I'd prefer aParentFrame->GetContentInsertionFrame(), I think....
> 
You are the boss, :-)

fixed
>>@@ -9457,25 +9467,24 @@ nsCSSFrameConstructor::ContentInserted(n
>>+  ConstructFrame(state, aChild, parentFrame, frameItems);
> 
> We should probably assert here that frameItems.firstChild ==
> frameItems.lastChild, since we assume it in this code.
> 
Assertion added
>>+      frameItems = nsFrameItems();
> 
> Or frameItems.RemoveChild(frameItems.firstChild), though that may actually be
> slower...
> 
>>+    prevSibling = nsnull;
>>+    isAppend = PR_TRUE;
> 
> So... why do we suddenly decide this is an append?  That doesn't make much
> sense to me.  I guess it's an easy fallback if we feel that all such cases are
> error cases anyway or something... But it feels like with this code it's easy
> to have the same DOM have two very different frame structures depending on the
> order of DOM operations to get to it, which is a little odd to me....
In principle you are right but in pseudo reality we handle non of the pseudo DOM operations correctly. So there is no hope at this place to recover the correct insertion point.
Comment 26 Bernd 2006-10-19 07:56:49 PDT
Created attachment 242765 [details] [diff] [review]
Patch rev2

Patch with review comments addressed (I hope so).
Comment 27 Bernd 2006-10-19 08:12:13 PDT
https://bugzilla.mozilla.org/attachment.cgi?id=242765&action=diff#mozilla/layout/base/nsCSSFrameConstructor.cpp_sec2
is a reminder in my tree not wrong in general but not part of the issue here.
Comment 28 Bernd 2006-10-19 10:04:37 PDT
Created attachment 242775 [details] [diff] [review]
rev2 that even compiles

IIUIC there is no firstChild for frameitems.
Comment 29 Boris Zbarsky [:bz] 2006-10-20 11:43:29 PDT
Comment on attachment 242775 [details] [diff] [review]
rev2 that even compiles

> So there is no hope at this place to recover the correct insertion point.

OK.  File a followup bug to fix that....  It should probably depend on some of the frame construction arch bugs we have.
Comment 30 Bernd 2006-10-20 13:10:31 PDT
Mats, I am not certain what is going on your side but I need this thing moving. I would prefer if you would check it in as it is your patch. I have enough cvs blame that I prefer not to watch tinderbox ;-) and don't wan't to steal patches. If you could just comment whether there is a issue with the patch or you will check in and when, I would feel much better.

Thanks
Comment 31 Bernd 2006-10-28 11:18:03 PDT
fixed on trunk
Comment 32 Stephen Donner [:stephend] - PTO; back on 5/28 2006-11-13 20:39:35 PST
None of the three testcases crashed for me using build 2006-11-13-09 of SeaMonkey trunk under Windows XP.

Verified FIXED.
Comment 33 Daniel Veditz [:dveditz] 2007-03-20 23:59:57 PDT
If bug 309322 lands on the branches then this will be needed too.
Comment 34 Daniel Veditz [:dveditz] 2007-03-23 08:49:35 PDT
And if we do land bug 309322 on the branch we need to make sure bug 343270 come back or is still fixed by this patch.
Comment 35 Daniel Veditz [:dveditz] 2007-04-25 13:56:27 PDT
Moving to 1.8.1.5 following bug 309322
Comment 36 Daniel Veditz [:dveditz] 2007-07-09 15:17:08 PDT
Moving to 1.8.1.6 following bug 309322
Comment 37 Daniel Veditz [:dveditz] 2007-10-04 14:45:53 PDT
fix checked into branch with bug 309322
Comment 38 Carsten Book [:Tomcat] 2007-10-12 08:39:22 PDT
verified fixed 1.8.1.8 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.8.1.8) Gecko/2007100816 Firefox/2.0.0.8 and Mozilla/5.0 (Macintosh; U;
Intel Mac OS X; ja-JP-mac; rv:1.8.1.8) Gecko/2007100816 Firefox/2.0.0.8 and the
testcase from this bug.

-> no crash on testcase -> adding verified keyword
Comment 39 Boris Zbarsky [:bz] 2009-02-27 17:26:14 PST
Added crashtest: http://hg.mozilla.org/mozilla-central/rev/4d4e400e1301

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