Closed Bug 497519 Opened 15 years ago Closed 15 years ago

Crash [@ nsBlockFrame::CheckFloats] with legend in XBL fieldset

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: tnikkel)

References

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?])

Crash Data

Attachments

(4 files, 4 obsolete files)

testcase (crashes Firefox when loaded)
604 bytes, application/xhtml+xml
Details
testcase for the undisplayed assert not using fieldset
544 bytes, text/html
Details
patch for the undisplayed asserts v3
2.50 KB, patch
dbaron
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
patch for the fieldset asserts v2
2.92 KB, patch
Details | Diff | Splinter Review 
###!!! ASSERTION: Legends should not be positioned and should not float: '!disp->IsAbsolutelyPositioned() && !disp->IsFloating()', file layout/forms/nsLegendFrame.cpp, line 53

###!!! ASSERTION: inserting after sibling frame with different parent: '!aPrevFrame || aPrevFrame->GetParent() == this', file layout/generic/nsBlockFrame.cpp, line 4869

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file layout/generic/nsBlockFrame.cpp, line 4960

###!!! ASSERTION: not in child list: 'found', file layout/base/nsCSSFrameConstructor.cpp, line 1558

Crash [@ nsBlockFrame::CheckFloats] touching 0xddddddfd.
Whiteboard: [sg:critical?]
Since this testcase uses XBL, it won't work from Bugzilla as long as this bug is hidden.
So the key here is that when we insert the legend we RecreateFramesForContent the fieldset... but apparently that doesn't remove the undisplayed entry for the <head>.  And then things go bad.

The thing that would normally remove the undisplayed content mappings is DoCleanupFrameReferences, but that clears undisplayed content under the frame's content (in this case the fieldset) while the undisplayed content map keeps track of things by content tree parent.

It should be pretty doable to create a testcase here that doesn't use fieldsets; just need a display:none kid of a node X, with a binding attached to X, and then reframe the insertion point that contains that kid.

Note that I don't see a crash here, nor any asserts other than the undisplayed content assert and the "legends should not be positioned" assert.  This last happens because the rule in forms.css is using "fieldset > legend" but the frame constructor is using the parent frame type (precisely to avoid XBL issues in the opposite direction).

We could change frame constructor to only do legend frames if both the parent frame _and_ the parent content are fieldset.  That would be a good idea anyway.  But it wouldn't fix the undisplayed content asserts...  Maybe those should go in a different bug.

That said, Jesse, do you still crash here?
Flags: blocking1.9.2?
Keywords: helpwanted
Doesn't crash any more for me.
Hmm.  A June 10 nightly does crash for me:
http://crash-stats.mozilla.com/report/index/f5461444-2836-48b7-8d3b-fe9e02090612?p=1

Did something fix it since then?
Changeset http://hg.mozilla.org/mozilla-central/rev/860a9acc39b1 doesn't crash, changeset http://hg.mozilla.org/mozilla-central/rev/4430cae50dad does crash. From that range I would guess bug 496840 fixed it.
Latest official 1.9.0 and latest 1.9.1 nightly do not crash on the testcase. So something on trunk caused the original crash.
http://hg.mozilla.org/mozilla-central/rev/0f4de606acd7 doesn't crash.
http://hg.mozilla.org/mozilla-central/rev/d0ae0b099a40 does crash.
Given that the float part of the testcase is necessary, the most obvious suspect would be the landings from bug 25888.
Range from comment 6: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0f4de606acd7&tochange=d0ae0b099a40

Blaming this on bug 25888 seems reasonable.  Good to know that this doesn't crash on branches!  I wonder whether that's true of variations of the testcase....
Oh, and comment 5 looks spot on to me too.

Timothy, do you think it's worth making the change I suggest in comment 2?

Another option would be to just create block frames for floated/positioned legends (and remove those !importants from forms.css).  They'd still have !important width and such, but that might be better than what we have now.  People have been asking for that sort of thing for a while now...
(In reply to comment #8)
> Timothy, do you think it's worth making the change I suggest in comment 2?

I actually made that change a while ago, but I am working on understanding the frame constructor more to make sure the change is right.

> Another option would be to just create block frames for floated/positioned
> legends (and remove those !importants from forms.css).  They'd still have
> !important width and such, but that might be better than what we have now. 
> People have been asking for that sort of thing for a while now...

Could do this...
For the undisplayed content assert, when we ClearAllUndisplayedContentIn should we also do the same for nodes in the content list and anonymous children list via nsBindingManager::GetContentListFor and nsBindingManager::GetAnonymousNodesFor respectively?
Attached patch fix the assertions (obsolete) — — Splinter Review 
This fixes the two asserts. For the undisplayed assert, in CleanupFrameReferences and DeletingFrameSubtree we walk the content and anonymous children lists of the current content and use content parent to remove any undisplayed entry.
As far as comment 10 goes, that might be right... David, what do you think?
The original crash was caused because bug 476063 allowed the frame tree to get into an inconsistent state. The changes of bug 25888 were unrelated, but they caused us to crash with this inconsistent frame tree, whereas before we luckily did not.

Using a source tree just after the landings of bug 25888 mentioned in comment 6, here is an abbreviated stack of the "inserting after sibling frame with different parent" assertion.

gklayout.dll!nsBlockFrame::InsertFrames(nsIAtom * aListName=0x00000000, nsIFrame * aPrevFrame=0x036bbc48, nsIFrame * aFrameList=0x03916c44)  Line 4886 + 0x30 bytes	C++
gklayout.dll!nsFieldSetFrame::InsertFrames(nsIAtom * aListName=0x00000000, nsIFrame * aPrevFrame=0x036bbc48, nsIFrame * aFrameList=0x03916c44)  Line 641 + 0xe bytes	C++
gklayout.dll!nsFrameManager::InsertFrames(nsIFrame * aParentFrame=0x036bba7c, nsIAtom * aListName=0x00000000, nsIFrame * aPrevFrame=0x036bbc48, nsIFrame * aFrameList=0x03916c44)  Line 698 + 0x13 bytes	C++
gklayout.dll!nsCSSFrameConstructor::AppendFrames(nsFrameConstructorState & aState={...}, nsIFrame * aParentFrame=0x036bba7c, nsFrameItems & aFrameList={...}, nsIFrame * aPrevSibling=0x036bbc48)  Line 5820 + 0x11 bytes	C++
gklayout.dll!nsCSSFrameConstructor::ContentInserted(nsIContent * aContainer=0x038bd448, nsIContent * aChild=0x036bba7c, int aIndexInContainer=6, nsILayoutHistoryState * aFrameState=0x00000000)  Line 6853	C++
gklayout.dll!nsCSSFrameConstructor::ContentAppended(nsIContent * aContainer=0x00000006, int aNewIndexInContainer=5)  Line 6288	C++

We are inserting the legend. In ContentInserted we call GetAdjustedParentFrame and that checks if the inserted content is a legend (it is), so it returns the fieldset frame as the parent (as opposed to the fieldset content frame, which holds all the regular elements inside the fieldset). This causes prevSibling to be the fieldset content frame. But in nsFieldSetFrame::InsertFrames we don't the treat inserted frame as a legend because it is floated so the inserted frame is actually a placeholder frame. So we try to insert the placeholder frame for the legend into the children of the fieldset content frame with the fieldset content frame itself as the prevSibling. And hence the assertion.

Bug 496840 fixed the crash because when the content is a legend we never insert a frame for the legend, we recreate the fieldset instead. On 1.9.0 and 1.9.1 in forms.css we don't have 'fieldset > legend' we just have 'legend' (changed in bug 476063), so the legend that gets inserted never gets the float property and we avoid all of this. So we should be safe from this kind of thing on the branches. And on trunk checking the content parent should fix this behaviour.
Blocks: 476063
Comment on attachment 385993 [details] [diff] [review]
fix the assertions

I would gladly accept a better name for the ClearUndisplayedContentWithBinding function.
Attachment #385993 - Flags: superreview?(bzbarsky)
Attachment #385993 - Flags: review?(dbaron)
So what's the thing that's undisplayed in this case?  Is it one of the toplevel nodes in the binding?  If so (which looks like the case based on my understanding of the patch), why don't we want to do this every time somebody calls ClearAllUndisplayedContentIn?  (If not, why aren't we hitting it when destroying the frames for the thing inside the binding?)  (Somebody remind me... what does child->GetParent() actually return in the cases where it's called inside ClearUndisplayedContentWithBinding?)
I'll use the second testcase in this bug as an example so I can refer to something concrete.

The relevant part of the content tree looks like this.

(div style="-moz-binding: url(#foo)")
  (div style="display:none")
  (span)

The (div style="-moz-binding: url(#foo)") has on its anonymous-children list (div style="position:relative"). The (div style="position:relative") has on its content-list the (div style="display:none") and the (span).

The relevant part of the frame tree looks like this.

(div style="-moz-binding: url(#foo)")
  (div style="position:relative")
    (span)

If we were to remove "display: none" style from the (div style="display:none") then the frame tree would look like this.

(div style="-moz-binding: url(#foo)")
  (div style="position:relative")
    (div style="")
    (span)

So, in this case, if we reconstructed the (div style="position:relative") frame, then the (div style="") frame would get reconstructed too.

But if we put the "display: none" back on that div and we reconstruct the (div style="position:relative") frame, the content tree parent of the (div style="display:none") is not the content of the (div style="position:relative") frame (nor any other frame in that subtree), so it doesn't get cleared, because the undisplayed map stores entries by their content tree parent.

(In reply to comment #16)
> why don't we want to do this every time somebody
> calls ClearAllUndisplayedContentIn?

I guess we might want to, as my patch does this for the only two callers of ClearAllUndisplayedContentIn.
Keywords: helpwanted
Flags: blocking1.9.2? → wanted1.9.2+
So I guess I'm ok with this, although I think the undisplayed map might be overdue for some redesign.

Two things, though:
 * you should move it inside ClearAllUndisplayedContentIn

 * can you (or bz) convince me that you need to call both GetAnonymousNodesFor and GetContentListFor rather than only GetContentListFor?  Based on the comments in nsBindingManager.h (which I admit seem rather sparse) it sounds like only GetContentListFor *might* be sufficient?  (And if those comments are misleading, maybe improve them?)
Comment on attachment 385993 [details] [diff] [review]
fix the assertions

And to be clear, I'd like to see the revised patch.
Attachment #385993 - Flags: review?(dbaron) → review-
(In reply to comment #18)
> So I guess I'm ok with this, although I think the undisplayed map might be
> overdue for some redesign.

CC me on any bugs about this redesign?

> Two things, though:
>  * you should move it inside ClearAllUndisplayedContentIn

Ok.

>  * can you (or bz) convince me that you need to call both GetAnonymousNodesFor
> and GetContentListFor rather than only GetContentListFor?  Based on the
> comments in nsBindingManager.h (which I admit seem rather sparse) it sounds
> like only GetContentListFor *might* be sufficient?  (And if those comments are
> misleading, maybe improve them?)

Good catch there. I think items on the anonymous nodes list for |content| will always have parent |content|. http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLBinding.cpp#523 is the only place the anonymous nodes list gets set, when their parent is the bound element.

I split out the fieldset fix from this patch. I'll post a separate patch for that with test soon.
Assignee: nobody → tnikkel
Attachment #392458 - Flags: review?(dbaron)
Comment on attachment 392458 [details] [diff] [review]
patch for the undisplayed asserts

>+  nsIDocument *document = aParentContent->GetOwnerDoc();
>+  if (!document)
>+    return;

I don't think GetOwnerDoc can return null, although it's possible I'm wrong.

r=dbaron

It would probably be good to have bzbarsky or sicking look over this as well.
GetOwnerDoc can return null.
Forget to change one instance of aContent to aParentContent in a comment. Otherwise the same.

I'll ask sicking since bz already pushed this one to dbaron.
Attachment #392458 - Attachment is obsolete: true
Attachment #392563 - Flags: review?(jonas)
Sorry for the lag here....  First off, the binding manager is a mess and the comments tend to suck.  Second of all, it's broken in various ways.

What GetContentListFor(foo) returns, in theory, is the entry for foo in mContentListTable, or foo.childNodes if there is no such entry.  In particular, what it returns is what the child nodes would be for foo in the flattened tree if foo did not have any bindings attached to it.  Note that if it _does_ have bindings attached to it, some of those nodes will end up further down the flattened tree.  Removing their undisplayed entries when removing |foo| is ok, of course.

But one problem with this patch is that for the common case (no XBL involved) it'll walk the child list of aParentContent, trying to remove undisplayed entries for every single node on that child list.

Another problem is that on dynamic mutations the mContentListTable list is not updated very well, and hence might be stale.

I think there are two basic options here.  Option 1 is to use GetXBLChildNodesFor(aParentContent).  This should, in theory, give the flattened tree child list for aParentContent, or null if there's no XBL involved in any way.  It'll certainly give whatever child nodes list we're actually using for frame construction (or null); see nsChildIterator.  If it's null, we're done; otherwise we need to walk it.

Option 2 is to add a new API to get the mContentListTable list, but then you run into the updating issues above.

I think option 1 is the way to go here.

I believe David is correct that in this case GetOwnerDoc can't return null.  In general it can, if the node is still alive after its owner document has been garbage collected, but you shouldn't see such nodes here.  And even that we plan to fix.
Wouldn't GetXBLChildNodesFor have the same updating issues as GetContentListFor? In getting the content list they both seem to do the same thing, just lookup in the mContentListTable table.

And GetXBLChildNodesFor would return the anonymous children (if they exist), which we don't really need to process, since their parents are fine. What about calling HasContentListFor first, if thats true then GetContentListFor? That avoids processing the regular children.
> In getting the content list they both seem to do the same thing, just lookup
> in the mContentListTable table.

GetXBLChildNodesFor only looks there if it doesn't find anything in the other hashtable...  On mutations, the GetXBLChildNodesFor return value is the only thing updated at the moment; see the mutation observer code in nsBindingManager.  And even then, it's not updated for all the relevant nodes, but there's not much you can do about that.

> And GetXBLChildNodesFor would return the anonymous children (if they exist)
> which we don't really need to process, since their parents are fine.

You could skip nodes that have aParentContent as parent, sure.

> What about calling HasContentListFor first, if thats true then
> GetContentListFor? That avoids processing the regular children.

That would probably still be ok to a first approximation; I still worry about the updating issue.  If we go this round, we want to introduce non-COM signatures for these methods, though.
GetXBLChildNodesFor calls GetAnonymousNodesInternal. GetAnonymousNodesInternal first checks the mAnonymousNodesTable, if there is nothing in there it calls GetBinding, which checks the mBindingTable table, if it finds a binding it returns the GetAnonymousNodes of that binding. If GetAnonymousNodesInternal returns nothing then GetXBLChildNodesFor returns whatever it finds in the mContentListTable hashtable.

My understanding is that anything from mAnonymousNodesTable at |content| will have parent |content|. So we don't care about it for this bug (whether or not it is updated). GetBinding I'm not sure about. The stuff in mContentListTable is what we want to get at because it can have a different parent.
> My understanding is that anything from mAnonymousNodesTable at |content| will
> have parent |content|.

This understanding is false.  If |content| has a binding attached, and that binding's <content> has a <children> node, then the mAnonymousNodesTable entry will consist of all the non-<children> kids of the <content> plus all the things from mContentListTable.

Again, if mContentListTable were properly updated on mutations we could just use it.  But it's not...
(In reply to comment #28)
> This understanding is false.  If |content| has a binding attached, and that
> binding's <content> has a <children> node, then the mAnonymousNodesTable entry
> will consist of all the non-<children> kids of the <content> plus all the
> things from mContentListTable.

"all the things from mContentListTable" meaning all the things from mContentListTable that should be there if updating of mContentListTable were done correctly?
Removed the document null check. Switched to GetXBLChildNodesFor. Check the parent to avoid unneeded hashtable lookups.
Attachment #392563 - Attachment is obsolete: true
Attachment #392814 - Flags: review?(dbaron)
Attachment #392563 - Flags: review?(jonas)
Attachment #392814 - Flags: review?(bzbarsky)
Comment on attachment 392814 [details] [diff] [review]
patch for the undisplayed asserts v3

Looks fine to me, but you should be asking Boris.  (No need to ask me again if he requests more changes.)
Attachment #392814 - Flags: review?(dbaron) → review+
Attachment #392814 - Flags: review?(bzbarsky) → review+
Attached patch patch for the fieldset asserts (obsolete) — — Splinter Review 
Also check the content tree parent of legends for fieldset before doing special legend stuff.

This applies on top of the 'patch for the undisplayed asserts' (so you don't need to merge crashtests.list).
Attachment #385993 - Attachment is obsolete: true
Attachment #392823 - Flags: review?(bzbarsky)
Attachment #385993 - Flags: superreview?(bzbarsky)
Comment on attachment 392823 [details] [diff] [review]
patch for the fieldset asserts

You need to check that the parent is HTML, not just a fieldset.  With that, looks good.
Attachment #392823 - Flags: review?(bzbarsky) → review+
Check that the parent is HTML too.
Attachment #392823 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [sg:critical?] → [sg:critical?][needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/35f8bd629153 for the undisplayed map and http://hg.mozilla.org/mozilla-central/rev/018f3fa01e3b for the fieldset check.

The latter we don't need on branches.  What about the former?
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 14 should answer this. In short, only on trunk do we have 'fieldset > legend' in forms.css, in the branches it's just 'legend', so the legend never gets float on the branches.
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs landing] → [sg:critical?]
Sure; that's why we don't need the second patch on branch.  But the undisplayed map issue is present on branch; can it lead to any problems?
Whoops, got mixed up there.

I didn't come across any crashes in working on it. Looking at the callers of GetUndisplayedContent I'm having a hard time trying to craft a scenario where we might do something seriously bad.
Crash Signature: [@ nsBlockFrame::CheckFloats]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: