The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9alpha6

Status

()

Core
Layout: Block and Inline
--
critical
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: dholbert)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla1.9alpha6
x86
All
assertion, crash, fixed1.8.0.14, testcase, verified1.8.1.8
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
sicking
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

10 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

10 years ago
Created attachment 266527 [details]
testcase
(Reporter)

Updated

10 years ago
Whiteboard: [sg:critical?]
Any takers? dbaron? roc?

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Assignee: nobody → dsicore
(Assignee)

Comment 4

10 years ago
Created attachment 267994 [details]
testcase (minimized)

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

Updated

10 years ago
Assignee: dsicore → dholbert
(Assignee)

Comment 5

10 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

10 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

10 years ago
Created attachment 268174 [details] [diff] [review]
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.
(Assignee)

Comment 8

10 years ago
Created attachment 268182 [details] [diff] [review]
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 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

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

Updated

10 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

10 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

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

Comment 14

10 years ago
Created attachment 268459 [details] [diff] [review]
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.
Attachment #268174 - Attachment is obsolete: true
Attachment #268459 - Flags: superreview?(bzbarsky)
Attachment #268459 - Flags: review?(jonas)
Attachment #268174 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 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

10 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 19

10 years ago
Created attachment 268903 [details]
new testcase for removing non-anonymous nodes that are inserted into anonymous insertion points
(Assignee)

Comment 20

10 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

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
OS: Mac OS X → All
(Assignee)

Comment 21

10 years ago
Created attachment 269897 [details] [diff] [review]
-w version of patch (move NOTIFY_OBSERVERS to end, without returning early)

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

10 years ago
Created attachment 269915 [details] [diff] [review]
trunk patch (incorporated jonas's suggestion)
Attachment #268459 - Attachment is obsolete: true
(Assignee)

Comment 24

10 years ago
Created attachment 269916 [details] [diff] [review]
trunk patch (nonwhitespace changes)
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: 10 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+
Blocks: 368276
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

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

Updated

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

Updated

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

Updated

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

Comment 30

10 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: 10 years ago10 years ago
Resolution: --- → FIXED
Version: 1.8 Branch → Trunk
(Assignee)

Comment 32

10 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

10 years ago
Yup, just discovered that -- thanks!
(Assignee)

Comment 35

10 years ago
Created attachment 272112 [details] [diff] [review]
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.
(Assignee)

Comment 36

10 years ago
Created attachment 272119 [details] [diff] [review]
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.
Attachment #272112 - Attachment is obsolete: true
(Assignee)

Comment 37

10 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

10 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

10 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

10 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

10 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+
Flags: in-testsuite?
(Assignee)

Comment 44

10 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
Keywords: checkin-needed → fixed1.8.0.14, fixed1.8.1.8
(Assignee)

Comment 46

10 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. 
Keywords: fixed1.8.1.8 → verified1.8.1.8
Group: security
Depends on: 400421
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

9 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.