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)

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.

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.

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

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.

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?

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.

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

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

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

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.

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.

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

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.