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

RESOLVED FIXED in mozilla1.9alpha6

Status

()

defect
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: jruderman, Assigned: dholbert)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla1.9alpha6
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.8 +
wanted1.8.1.x +
blocking1.8.0.14 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(6 attachments, 5 obsolete attachments)

792 bytes, application/xhtml+xml
Details
522 bytes, application/xhtml+xml
Details
471 bytes, application/xhtml+xml
Details
3.18 KB, patch
Details | Diff | Splinter Review
2.57 KB, patch
Details | Diff | Splinter Review
4.78 KB, patch
bzbarsky
: review+
sicking
: superreview+
Details | Diff | Splinter Review
Reporter

Description

12 years ago
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.
Flags: blocking1.9?
Reporter

Comment 1

12 years ago
Posted file testcase
Reporter

Updated

12 years ago
Whiteboard: [sg:critical?]
Any takers? dbaron? roc?
Flags: blocking1.9? → blocking1.9+
Assignee: nobody → dsicore
Assignee

Comment 4

12 years ago
More minimized version of Jesse's test case.  (Removed some content and incorporated first "boom" function into the initial page layout)
Assignee: dsicore → dholbert
Assignee

Comment 5

12 years ago
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)
Assignee

Comment 6

12 years ago
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.
Assignee

Comment 7

12 years ago
Implements the fix suggested at the end of my comment #6.  Fixes the test case.
Assignee

Comment 8

12 years ago
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 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.
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.
Assignee

Updated

12 years ago
Attachment #268174 - Flags: superreview?(jonas)
Attachment #268174 - Flags: review?(bzbarsky)
Assignee

Updated

12 years ago
Attachment #268182 - Flags: superreview?(jonas)
Attachment #268182 - Flags: review?(bzbarsky)
Attachment #268174 - Flags: superreview?(jonas) → superreview+
You've run this through our test suits right?
Attachment #268182 - Flags: superreview?(jonas)
Attachment #268182 - Flags: superreview+
Attachment #268182 - Flags: review?(bzbarsky)
Attachment #268182 - Flags: review+
Assignee

Comment 12

12 years ago
(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.
Assignee

Comment 13

12 years ago
D'oh... a few reftests are failing.  Gonna investigate.  More on this soon.
Assignee

Comment 14

12 years ago
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.
Attachment #268174 - Attachment is obsolete: true
Attachment #268459 - Flags: superreview?(bzbarsky)
Attachment #268459 - Flags: review?(jonas)
Attachment #268174 - Flags: review?(bzbarsky)
Assignee

Updated

12 years ago
Attachment #268459 - Flags: superreview?(jonas)
Attachment #268459 - Flags: superreview?(bzbarsky)
Attachment #268459 - Flags: review?(jonas)
Attachment #268459 - Flags: review?(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?
Attachment #268459 - Flags: review?(bzbarsky) → review+
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.
Assignee

Comment 17

12 years ago
(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.
I meant removing non-anonymous nodes that are inserted into anonymous insertion points...
Assignee

Comment 20

12 years ago
(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!
Assignee

Updated

12 years ago
Status: NEW → ASSIGNED
Assignee

Updated

12 years ago
OS: Mac OS X → All
Assignee

Comment 21

12 years ago
No-whitespace version of my last patch.
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.
Attachment #268459 - Flags: superreview?(jonas) → superreview+
Assignee

Comment 23

12 years ago
Attachment #268459 - Attachment is obsolete: true
Assignee

Comment 24

12 years ago
Attachment #269897 - Attachment is obsolete: true
Checked in to trunk.

(attachment 268182 [details] [diff] [review] isn't relevant anymore since we've switched from nsTextFrame.cpp to nsTextFrameThebes.cpp.)
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha6
Affects 1.8.0 and 1.8.1 branches as well
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
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
Patch doesn't apply, code freeze got moved up, this will have to wait for 1.8.1.6
Flags: blocking1.8.1.5+ → blocking1.8.1.6?
Assignee

Comment 29

12 years ago
Reopening bug as affecting 1.8 branch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Version: Trunk → 1.8 Branch
Assignee

Updated

12 years ago
Attachment #269915 - Attachment description: patch (incorporated jonas's suggestion) → trunk patch (incorporated jonas's suggestion)
Assignee

Updated

12 years ago
Attachment #269916 - Attachment description: patch (nonwhitespace changes) → trunk patch (nonwhitespace changes)
Assignee

Updated

12 years ago
Attachment #268182 - Attachment description: Convert warning to notreached → trunk: Convert warning to notreached
Attachment #268182 - Attachment is obsolete: true
Assignee

Comment 30

12 years ago
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.
> 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...
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Version: 1.8 Branch → Trunk
Assignee

Comment 32

12 years ago
> 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.
On branch you probably want to change the observer enumeration order in nsDocument.
Assignee

Comment 34

12 years ago
Yup, just discovered that -- thanks!
Assignee

Comment 35

12 years ago
Posted patch branch patch (tentative) (obsolete) — Splinter Review
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.
Assignee

Comment 36

12 years ago
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.
Attachment #272112 - Attachment is obsolete: true
Assignee

Comment 37

12 years ago
(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.
Assignee

Updated

12 years ago
Attachment #272119 - Flags: superreview?(jonas)
Attachment #272119 - Flags: review?(bzbarsky)
Comment on attachment 272119 [details] [diff] [review]
branch patch (ver. 2)

r=bzbarsky
Attachment #272119 - Flags: review?(bzbarsky) → review+
Attachment #272119 - Flags: superreview?(jonas) → superreview+
Assignee

Comment 39

12 years ago
Marking [checkin needed], for branch patch, attachment 272119 [details] [diff] [review]
Whiteboard: [sg:critical?] → [sg:critical?] [checkin needed]
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.
Whiteboard: [sg:critical?] [checkin needed] → [sg:critical?]
Assignee

Comment 41

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

Thanks for the tip, Gavin.
Requesting approval.
Attachment #272119 - Flags: approval1.8.1.6?
Attachment #272119 - Flags: approval1.8.1.5?
Attachment #272119 - Flags: approval1.8.0.13?
Assignee

Comment 42

12 years ago
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.
Attachment #272119 - Flags: approval1.8.1.5?
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
Attachment #272119 - Flags: approval1.8.0.13? → approval1.8.0.14?
Flags: blocking1.8.1.7? → blocking1.8.1.7+
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
Attachment #272119 - Flags: approval1.8.1.7?
Attachment #272119 - Flags: approval1.8.1.7+
Attachment #272119 - Flags: approval1.8.0.14?
Attachment #272119 - Flags: approval1.8.0.14+

Updated

12 years ago
Flags: in-testsuite?
Assignee

Comment 44

12 years ago
checkin-needed for attachment 272119 [details] [diff] [review]  "branch patch (ver. 2)"
Keywords: checkin-needed
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
Assignee

Comment 46

12 years ago
Thanks, Reed!
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. 
Group: security
Flags: blocking1.8.0.14? → blocking1.8.0.14+
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.
Reporter

Comment 49

12 years ago
Crashtests checked in.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.