382376 - "ASSERTION: reflow dirty lines failed" and more with block-in-inline, wrapping, XBL

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Jesse Ruderman]

Loading the testcase triggers two assertions:

###!!! ASSERTION: reflow dirty lines failed: 'NS_SUCCEEDED(rv)', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 912

###!!! ASSERTION: child list is not empty for initial reflow: 'mFrames.IsEmpty()', file /Users/jruderman/trunk/mozilla/layout/generic/nsInlineFrame.cpp, line 334

Closing it triggers a third:

###!!! ASSERTION: Some frame destructors were not called: 'mFrameCount == 0', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 674

This bug appears to be exploitable.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 3

[Daniel Veditz [:dveditz]]

Any takers? dbaron? roc?

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase (minimized)

More minimized version of Jesse's test case.  (Removed some content and incorporated first "boom" function into the initial page layout)

Comment 5

[Daniel Holbert [:dholbert]]

The first time the code detects that something's wrong here is at
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsTextFrame.cpp#6048, with warning message "Content has no document".  Then an NS_ERROR_FAILURE value gets passed up through a chain of reflow function returns, to eventually trigger the assertion.

Per Jesse's request, the aforementioned warning will be made into an assertion in the bugfix.  (dbaron agrees, via IRC)

Comment 6

[Daniel Holbert [:dholbert]]

So, here's what I've found is going wrong.

The main issue seems to be in nsBindingManager::ContentRemoved (URL: http://mxr.mozilla.org/seamonkey/source/content/xbl/src/nsBindingManager.cpp#1322 )

When this function begins, the deleted content still exists in the content tree as anonymous content.  This anonymous content isn't removed until line 1351, with "point->RemoveChild(aChild);".  

On the first line of nsBindingManager::ContentRemoved, we call  NS_BINDINGMANAGER_NOTIFY_OBSERVERS, which triggers the frames to be reconstructed.  (specifically, frame reconstruction happens within PresShell::ContentRemoved, which gets called via an observer)  So, this means we create frames for the anonymous content, and then this content gets removed at line 1351, and we end up with frames that have no content.

The simplest fix is to just move the NS_BINDINGMANAGER_NOTIFY_OBSERVERS call to the end of nsBindingManager::ContentRemoved.  That way, frame construction happens *after* the anonymous content has been removed.  I'm not sure if that'd break other things, though.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: tentative patch: move NS_BINDINGMANAGER_NOTIFY_OBSERVERS to end of nsBindingManager::ContentRemoved

Implements the fix suggested at the end of my comment #6.  Fixes the test case.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: trunk: Convert warning to notreached

This patch creates a NS_NOTREACHED out of the lowest warning that causes this bug's assertion.  (see comment #5)

Note that this is not a fix, it just makes the bug (and future bugs like it) easier to diagnose.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 268174 [details] [diff] [review]
tentative patch: move NS_BINDINGMANAGER_NOTIFY_OBSERVERS to end of nsBindingManager::ContentRemoved

This actually looks correct to me given that nsGenericElement::doRemoveChildAt notifies after mutating the DOM.

Would like to get bzs input though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

The ordering in ContentRemoved is the way it is because we need to know insertion point information to properly tear down frames for a node.  I thought we'd had documentation to that effect, but apparently not.  We should add it.

Why are we _constructing_ frames on a ContentRemoved?  Is there an {ib} split involved or something?  I guess with the <div> inside the <span>s there is, in fact.

Ideally we would remove the frames that need removing, then update insertion point info, then construct whatever needs constructing, if anything...

The long-term approach, as discussed before, is to pass notifications on the flattened tree to the frame constructor, but that's not a 1.9 kind of thing at this point.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You've run this through our test suits right?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #11)
> You've run this through our test suits right?

Doing so right now.  So far, have tested by throwing variations on the test case at the patch and making sure nothing fails, along with testing normal browser usage.

Comment 13

[Daniel Holbert [:dholbert]]

D'oh... a few reftests are failing.  Gonna investigate.  More on this soon.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: patch: move NOTIFY_OBSERVERS to end, without returning early.

The first version of my patch had a bug -- it was possible to return from  nsBindingManager::ContentRemoved without ever calling NS_BINDINGMANAGER_NOTIFY_OBSERVERS, because of this if-test:

   if (aIndexInContainer == -1 ||
       (!mContentListTable.ops && !mAnonymousNodesTable.ops))
     // It's anonymous.
     return;

To fix that unintended consequence, I've made one more change: I inverted the test condition, and I'm having it guard the bulk of the function rather than guarding a "return" statement.  (as a result, the "return" is no longer necessary)

This patch passes the layout reftests and mochitests.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 268459 [details] [diff] [review]
patch: move NOTIFY_OBSERVERS to end, without returning early.

r=bzbarsky, but we should add some tests for this bug too; in particular some tests of removing and appending with XBL anon content...

I also assume you tested DOM inspector with anon content, right?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note that comment 10, as we've discovered, is at least partly wrong.  The orderign is the way it is because hyatt wrote it like that, but at the present moment nsCSSFrameConstructor::ContentRemoved doesn't seem to actually use insertion point information.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> (From update of attachment 268459 [details] [diff] [review])
> r=bzbarsky, but we should add some tests for this bug too; in particular some
> tests of removing and appending with XBL anon content...
> 
> I also assume you tested DOM inspector with anon content, right?
> 

Yes, I tested DOM inspector with anon content, and I've found an issue with deleting anon content that is unrelated to my patch (it occurs both with and without the patch). I filed this as Bug 384483.  

Aside from that issue (which isn't caused by the patch), I haven't run into any other issues with deleting anon content using my patch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I meant removing non-anonymous nodes that are inserted into anonymous insertion points...

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: new testcase for removing non-anonymous nodes that are inserted into anonymous insertion points

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to comment #19)
Just posted a test case for the situation that bz was requesting, to make sure the patch doesn't break something. (bz, let me know if this isn't what you were looking for)

Steps to test:
0. Apply patch (attachment 268459 [details] [diff] [review])
1. In patched trunk, load new testcase (attachment 268903 [details])
2. Fire up DOM inspector
3. Dink open the tree down through html | body | div#parent | div#anonparent
4. Select div#child and press delete
Expected results: no assertions, and "Here's the child div." should disappear from page.
Observed results: Works on my computer. yay!

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: -w version of patch (move NOTIFY_OBSERVERS to end, without returning early)

No-whitespace version of my last patch.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 268459 [details] [diff] [review]
patch: move NOTIFY_OBSERVERS to end, without returning early.

>+  } // else, it's anonymous.

Took me a bit to understand what this comment means. I'd instead put "it's not anonymous" at the top inside the if-statement instead.

sr=me with that.

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: trunk patch (incorporated jonas's suggestion)

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: trunk patch (nonwhitespace changes)

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

(attachment 268182 [details] [diff] [review] isn't relevant anymore since we've switched from nsTextFrame.cpp to nsTextFrameThebes.cpp.)

Comment 26

[Daniel Veditz [:dveditz]]

Affects 1.8.0 and 1.8.1 branches as well

Comment 27

[Daniel Veditz [:dveditz]]

Does this patch work for the 1.8 branches? Please request approval1.8.1.5 on  the correct branch patch. Code-freeze for 1.8.1.5 is July 13

Comment 28

[Daniel Veditz [:dveditz]]

Patch doesn't apply, code freeze got moved up, this will have to wait for 1.8.1.6

Comment 29

[Daniel Holbert [:dholbert]]

Reopening bug as affecting 1.8 branch.

Comment 30

[Daniel Holbert [:dholbert]]

After debugging a branch build running the minimized test case, it looks like the issue is this:

After the node has been removed via javascript, and when we're rebuilding that section of the frame tree based on the updated content tree, we come to a point where we're running nsCSSFrameConstructor::ProcessInlineChildren().  At this point, aContent is a nsHTMLSpanElement that *used* to have 2 children, but now it only has 1 child, because its second child was removed.  aContent->GetChildCount() returns 1, and aContent->GetChildAt(1) returns 0x0, as would be expected. 

However, ProcessInlineChildren uses a *ChildIterator* to get at its children, NOT GetChildCount/GetChildAt, and the ChildIterator is whacked.  During ChildIterator::Init, we call doc->BindingManager()->GetXBLChildNodesFor(aContent...), and *that* function returns *2* nodes, not 1.  So, we end up iterataing across 2 children (including the removed one), even though there should now only be 1 child.

So, it looks like we need to do an earlier update to whatever place that GetXBLChildNodesFor() gets its information from.  Or something like that.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Reopening bug as affecting 1.8 branch.

Please don't do that.  We have keywords to track branch state.  The resolution tracks the trunk state.

If you really have to have an open bug to work on, file a separate bug, please.

As for the branch issue, it's the same as trunk, no?  And a similar fix should work...

Comment 32

[Daniel Holbert [:dholbert]]

> Please don't do that.

Oops, sorry about that.  I'll just leave it closed, and post the branch patch here when I've got it.  (I just re-opened it 'cause I wasn't sure if it'd look weird to post a patch on a bug already marked RESOLVED/FIXED.)


> As for the branch issue, it's the same as trunk, no?  And
> a similar fix should work...

Well, yes and no... At least, the fix isn't in the same function as it is in trunk.  The non-whitespace trunk patch (attachment 269916 [details] [diff] [review]) shows that we just move "NS_BINDINGMANAGER_NOTIFY_OBSERVERS" from the beginning of nsBindingManager::ContentRemoved to the end, but in Branch, nsBindingManager::ContentRemoved doesn't call NS_BINDINGMANAGER_NOTIFY_OBSERVERS at all (that function doesn't even exist, actually). (see http://mxr.mozilla.org/mozilla1.8/source/content/xbl/src/nsBindingManager.cpp#1278)

Anyway, tracking it down, and should have a branch patch soon.

Comment 33

[Boris Zbarsky [:bzbarsky]]

On branch you probably want to change the observer enumeration order in nsDocument.

Comment 34

[Daniel Holbert [:dholbert]]

Yup, just discovered that -- thanks!

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: branch patch (tentative)

This patch does the following:
 - Reverses the order in which observers are notified in nsDocument::ContentRemoved, so that nsBindingManager gets a chance to clean up before nsCSSFrameConstructor starts rebuilding frames
 - Adds a check for !mAnonymousNodesTable.ops in  nsBindingManager::ContentRemoved.  This matches the behavior of trunk, and it's  this is required for this bug's testcases to work. (otherwise, the important run of nsBindingManager::ContentRemoved just returns right away without doing its clean-up.)

This patch fixes branch on all of the test cases for this bug.

I'm unsure of one thing, though (hence my labeling this patch as "tentative"). There's a comment in nsDocument::ContentRemoved implying that there was some hack-ish dependency on the original iteration order.  If that comment still applies, this may break something.

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: branch patch (ver. 2)

The check for mAnonymousNodesTable.ops mentioned above is actually part of another patch on Trunk that hadn't been merged back to Branch yet: bug 375299.

In this new patch, I include the rest of 375299's patch.  (Just added checks for mAnonymousNodesTable.ops in ContentAppended and ContentInserted.)

Also, I'm pretty sure the comment about ordering of observers is outdated.  bz's comment 16 applies refers to the same stuff and explains this.  I'm going to test this patch some more to make sure the ordering is ok, but so far it looks good.

Comment 37

[Daniel Holbert [:dholbert]]

(In reply to comment #36)
>  ... I'm going
> to test this patch some more to make sure the ordering is ok, but so far it
> looks good.

Tested the branch patch on a bunch of layout reftests, and it seems to be fine.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272119 [details] [diff] [review]
branch patch (ver. 2)

r=bzbarsky

Comment 39

[Daniel Holbert [:dholbert]]

Marking [checkin needed], for branch patch, attachment 272119 [details] [diff] [review]

Comment 40

[:Gavin Sharp [email: [e-mail]]]

The branch patch doesn't seem to have approval, you'll need to request it first. Also, you can use the "checkin-needed" keyword now instead of putting it in the whiteboard.

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 272119 [details] [diff] [review]
branch patch (ver. 2)

Thanks for the tip, Gavin.
Requesting approval.

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 272119 [details] [diff] [review]
branch patch (ver. 2)

oops -- removing approval1.8.1.5 flag, as it's too late for that release.

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 272119 [details] [diff] [review]
branch patch (ver. 2)

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers

Comment 44

[Daniel Holbert [:dholbert]]

checkin-needed for attachment 272119 [details] [diff] [review]  "branch patch (ver. 2)"

Comment 45

[Reed Loden [:reed]]

MOZILLA_1_8_BRANCH:

Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.566.2.35; previous revision: 3.566.2.34
done
Checking in content/xbl/src/nsBindingManager.cpp;
/cvsroot/mozilla/content/xbl/src/nsBindingManager.cpp,v  <--  nsBindingManager.cpp
new revision: 1.136.2.4; previous revision: 1.136.2.3
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/Attic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.513.4.17; previous revision: 1.513.4.16
done

MOZILLA_1_8_0_BRANCH:

Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.566.2.6.2.16; previous revision: 3.566.2.6.2.15
done
Checking in content/xbl/src/nsBindingManager.cpp;
/cvsroot/mozilla/content/xbl/src/nsBindingManager.cpp,v  <--  nsBindingManager.cpp
new revision: 1.136.2.1.4.3; previous revision: 1.136.2.1.4.2
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/Attic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.513.4.6.2.5; previous revision: 1.513.4.6.2.4
done

Comment 46

[Daniel Holbert [:dholbert]]

Thanks, Reed!

Comment 47

[juan becerra [:juanb]]

Verified using FF 2008rc2: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/2007100816 Firefox/2.0.0.8

No longer crashes with testcases in comment #2, nor comment #4. 

Comment 48

[Al Billings [:abillings - ex-MoCo]]

I'm looking at this in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.12) Gecko/20070508 Firefox/1.5.0.12 to see the bug before trying it in the nightlies built from the 1.8.0 branch to see the fix. I can't reproduce the bug.

Comment 49

[Jesse Ruderman]

Crashtests checked in.