Closed Bug 439258 Opened 16 years ago Closed 9 years ago

"ASSERTION: anonymous nodes should not be in child lists"

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
status2.0 --- ?

People

(Reporter: jruderman, Assigned: ehsan.akhgari)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [post-2.0])

Attachments

(2 files)

testcase
303 bytes, text/html
Details
Interactive testcase
2.07 KB, text/html
Details
Attached file testcase — 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a1pre) Gecko/2008061310 Minefield/3.1a1pre

Steps to reproduce:
1. Load the testcase.
2. Close the window.  (Reloading will not trigger the bug.)

Result:

###!!! ASSERTION: native anonymous nodes should not be in child lists: '!aOldChild->IsNativeAnonymous()', file /Users/jruderman/central/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13294

This assertion was added by the patch in bug 436453 comment 22.
Blocks: 436453
Fun times... nsHTMLEditor::DeleteRefToAnonymousNode in fact calls ContentRemoved on an anonymous node.

Perhaps we should allow ContentRemoved with an index of -1 on such nodes?  Either that, or provide some other way to tear down their frames, which is really all editor wants here.
I think calling RemoveContent will confuse enough of our other nsIMutationObservers that it'd be better of editor could use some other mechanism.
Still happens after http://hg.mozilla.org/mozilla-central/index.cgi/rev/80865e03f6af, with the new assertion text:

###!!! ASSERTION: anonymous nodes should not be in child lists: '!aOldChild->IsRootOfAnonymousSubtree()', file layout/base/nsCSSFrameConstructor.cpp, line 13093
Summary: "ASSERTION: native anonymous nodes should not be in child lists" → "ASSERTION: anonymous nodes should not be in child lists"
Jonas, editor is calling this on the presshell directly, so no other mutation observers are involved.

Basically it's a hack on the part of editor, but I'm not sure there's a better API for it to use currently.
Flags: blocking1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Still happens on trunk.
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1292663840.1292669312.24200.gz&fulltext=1
Linux comm-central-trunk debug test mochitests-4/5 on 2010/12/18 01:17:20
{
707 INFO TEST-START | /tests/layout/base/tests/test_bug558663.html
...
###!!! ASSERTION: anonymous nodes should not be in child lists: '!aOldChild->IsRootOfAnonymousSubtree()', file /builds/slave/comm-cen-trunk-lnx-dbg/build/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 11506
nsCSSFrameConstructor::RestyleForRemove [layout/base/nsCSSFrameConstructor.cpp:11508]
PresShell::ContentRemoved [layout/base/nsPresShell.cpp:5132]
nsHTMLEditor::DeleteRefToAnonymousNode [editor/libeditor/html/nsHTMLAnonymousUtils.cpp:262]
nsHTMLEditor::RemoveListenerAndDeleteRef [editor/libeditor/html/nsHTMLAnonymousUtils.cpp:223]
nsHTMLEditor::HideResizers [editor/libeditor/html/nsHTMLObjectResizer.cpp:454]
nsHTMLEditor::CheckSelectionStateForAnonymousButtons [editor/libeditor/html/nsHTMLAnonymousUtils.cpp:358]
...
2829 INFO TEST-START | /tests/layout/generic/test/test_invalidate_during_plugin_paint.html
...
###!!! ASSERTION: anonymous nodes should not be in child lists: '!aOldChild->IsRootOfAnonymousSubtree()', file /builds/slave/comm-cen-trunk-lnx-dbg/build/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 11506
nsCSSFrameConstructor::RestyleForRemove [layout/base/nsCSSFrameConstructor.cpp:11508]
PresShell::ContentRemoved [layout/base/nsPresShell.cpp:5132]
nsHTMLEditor::DeleteRefToAnonymousNode [editor/libeditor/html/nsHTMLAnonymousUtils.cpp:262]
nsHTMLEditor::RemoveListenerAndDeleteRef [editor/libeditor/html/nsHTMLAnonymousUtils.cpp:223]
nsHTMLEditor::HideResizers [editor/libeditor/html/nsHTMLObjectResizer.cpp:454]
nsHTMLEditor::~nsHTMLEditor [editor/libeditor/html/nsHTMLEditor.cpp:183]
nsHTMLEditor::~nsHTMLEditor [editor/libeditor/html/nsHTMLEditor.cpp:224]
nsEditor::Release [editor/libeditor/base/nsEditor.cpp:217]
nsHTMLEditor::Release [editor/libeditor/html/nsHTMLEditor.cpp:234]
nsCOMPtr<nsIEditor>::assign_assuming_AddRef [:519]
nsCOMPtr<nsIEditor>::assign_with_AddRef [nsCOMPtr.h:1205]
nsCOMPtr<nsIEditor>::operator= [nsCOMPtr.h:664]
nsDocShellEditorData::TearDownEditor [docshell/base/nsDocShellEditorData.cpp:82]
nsDocShellEditorData::~nsDocShellEditorData [docshell/base/nsDocShellEditorData.cpp:70]
nsAutoPtr<nsDocShellEditorData>::assign [:71]
nsAutoPtr<nsDocShellEditorData>::operator= [:135]
nsSHEntry::SetEditorData [docshell/shistory/src/nsSHEntry.cpp:942]
...
}
Severity: normal → major
status2.0: --- → ?
OS: Mac OS X → All
Had to annotate some mochitests because bug 633738 made us hit it more often: http://hg.mozilla.org/mozilla-central/rev/4b11eb3c12a0
Assignee: nobody → ehsan
Whiteboard: [post-2.0]
So, is there a better API for us to use here?  I couldn't find any...
I don't think there is one.  We should come up with a sane API for this.

What are editor's constraints here, exactly?  For example, when does it want to add/remove the anon content?
(In reply to comment #9)
> What are editor's constraints here, exactly?  For example, when does it want to
> add/remove the anon content?

The editor needs an API to dynamically attach and detach anonymous content to/from a node, and get the frames constructed and destroyed properly.  I'm not sure why the when is important, but it can be both in response to user actions (clicking to focus an image for example) or script (focusing an absolutely positioned element, for example).

The anonymous content are currently used for things like resizing objects, managing table cells, and positioning absolutely positioned elements.
When matters only in the sense of lifetime; if you need to take away the anon content before the frame goes away, then you need an API to do that.

So I think the right thing would be to just add dedicated API for this on presshell or even on the frame constructor.  While we're there we'd want to store the content in question in a property on the node, so that reframing the node will correctly recreate the frames.  Or we could do that bit in the existing bugs we have on this...
(In reply to comment #11)
> When matters only in the sense of lifetime; if you need to take away the anon
> content before the frame goes away, then you need an API to do that.

Yes, I need to take away the anon content even if the frame is going to persist.

> So I think the right thing would be to just add dedicated API for this on
> presshell or even on the frame constructor.  While we're there we'd want to
> store the content in question in a property on the node, so that reframing the
> node will correctly recreate the frames.  Or we could do that bit in the
> existing bugs we have on this...

Hmm, we currently bind the anonymous nodes to the tree and they get reframed properly.  Is that an acceptable solution?
> we currently bind the anonymous nodes to the tree and they get reframed
> properly

I don't see how they could (and they didn't last I checked), because the frame constructor knows nothing about them.

You have to bind to the tree no matter what just to get them to show up, but if they're anonymous and the thing they were bound to as parent is reframed, they won't have frames anymore.
(In reply to comment #13)
> > we currently bind the anonymous nodes to the tree and they get reframed
> > properly
> 
> I don't see how they could (and they didn't last I checked), because the frame
> constructor knows nothing about them.

Try this test case:

data:text/html,<div contenteditable><div id=x style="position:absolute">foo</div></div><script>onload=function(){var d=document.getElementById('x');d.focus();setTimeout(function(){d.style.display="inline";document.body.clientWidth},2000);setInterval(function(){document.body.appendChild(document.createTextNode(d.style.display))},500)}</script>

Click on "foo" as soon as the page loads.  The div gets reframed two seconds after the page has loaded.

> You have to bind to the tree no matter what just to get them to show up, but if
> they're anonymous and the thing they were bound to as parent is reframed, they
> won't have frames anymore.

Here's where we bind them to the tree: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLAnonymousUtils.cpp#188> and here's where we create frames for them: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLAnonymousUtils.cpp#207>.  We don't call SetInitialChildList on the parent frame to store the anonymous nodes there.  Honestly I don't know how the reframing works, but it seems to be working just fine!
> Try this test case:

That testcase is broken in two ways:

1)  It's not reframing the abs pos div, because display:inline is forced to
    display:block by position:absolute, so the computed display doesn't change.
2)  The resize handles are parented to the editor root (in this case the <div
    contenteditable>), not to the thing being resized.

If you change the testcase to reframe the <div contenteditable> or any of its ancestors (e.g. document.body) then the resize handles go away when the reframe happens.
(In reply to comment #15)
> > Try this test case:
> 
> That testcase is broken in two ways:
> 
> 1)  It's not reframing the abs pos div, because display:inline is forced to
>     display:block by position:absolute, so the computed display doesn't change.
> 2)  The resize handles are parented to the editor root (in this case the <div
>     contenteditable>), not to the thing being resized.
> 
> If you change the testcase to reframe the <div contenteditable> or any of its
> ancestors (e.g. document.body) then the resize handles go away when the reframe
> happens.

Heh, now that makes sense.  Thanks for confirming my sanity, and the fragility of my test.  :-)
So, I've been trying to contemplate what the new API should look like.

The problem is that so many parts of our code expect to use nsIAnonymousContentCreator to ask the content node about the required anonymous content.  I think the easiest approach is to "overlay" the element with a new nsIAnonymousContentCreator interface which gives us the desired anonymous content nodes, and then just reframe it, so that all of the magic happens as it does for regular content nodes.

Here is what I was thinking about: how about we add a flag on the content which indicates that it requires additional anonymous elements, and create a tearoff class based on a new noscript interface (let's call it nsIAdditionalAnonymousContentList) in nsGenericElement::DOMQueryInterface which communicates the required anonymous nodes back to the layout (it merely just stores a list of DOM nodes).  The implementation of the tearoff is based on nsBaseContentList.

Then, nsFrame would have a default implementation of nsIAnonymousContentCreator which would QI the content node for nsIAdditionalAnonymousContentList and incorporate the elements stored on the content node into the anonymous elements list.  Existing nsIAnonymousContentCreator implementation should be modified to call the base class version of the functions they override.

With this API, here's what the editor would do.  When adding the anonymous content, it would set the flag on the content node, QI it for nsIAdditionalAnonymousContentList, and append the anonymous content list, and then reframe the element.

When removing the anonymous content, the editor would QI the content node for nsIAdditionalAnonymousContentList, remove the desired anonymous element, and reframe the element.

(The reframing part can be done in the tearoff implementing nsIAdditionalAnonymousContentList by just using nsIPresShell::FrameNeedsReflow, I think, assuming that those reframe requests can be coalesced.)

Does this make sense?
I was thinking more like the boolean flag and a property holding the content list.  That way we don't need a new interface, etc...

That would need a bit of work on the frame constructor end to check the flag and get the property and such, but that's easy to do.  Probably about as much code as the frame bits you suggested.

FrameNeedsReflow won't reframe things, but there's an existing function on presshell which will: RecreateFramesFor.  So that should be easy.
(In reply to comment #18)
> I was thinking more like the boolean flag and a property holding the content
> list.  That way we don't need a new interface, etc...

By property, you mean a frame property?  Don't they go away when reframing?  Do we have any efficient property storage on content nodes like the frame property table?
> Do we have any efficient property storage on content nodes like the frame
> property table?

Dunno about "efficient", but nsINode::GetProperty/SetProperty/DeleteProperty do work and are what I was thinking of here.  They're not quite as optimized as the frame versions, but not that bad.
(In reply to comment #20)
> > Do we have any efficient property storage on content nodes like the frame
> > property table?
> 
> Dunno about "efficient", but nsINode::GetProperty/SetProperty/DeleteProperty do
> work and are what I was thinking of here.  They're not quite as optimized as
> the frame versions, but not that bad.

OK, I'll give those a shot then!
This plan looked great for a while, until I realized that we do not have any more content bits to use.  :(

While browsing around trying to see what I can do here, I found these flags <http://hg.mozilla.org/mozilla-central/diff/01f5785ccd97/content/html/content/src/nsGenericHTMLElement.h>.  The comment there seems to suggest that we can squeeze in one more node flag by merging these two flags, but I'm not really sure if merging them is safe/wanted...
Target Milestone: --- → mozilla2.0
Version: Trunk → Other Branch
Target Milestone: mozilla2.0 → ---
Version: Other Branch → Trunk
We've discussed expanding the size of nsINode to create more bits.
We already have.  We just need to use it.  Note nsINode::mNodeHasRenderingObservers and bug 581177.

If that's blocking you, I'll expedite it (like "do it tomorrow").
(In reply to comment #24)
> We already have.  We just need to use it.  Note
> nsINode::mNodeHasRenderingObservers and bug 581177.
> 
> If that's blocking you, I'll expedite it (like "do it tomorrow").

Yes, please!  I want to get this fixed for 2.2, which means that bug 581177 is blocking me...
Depends on: 581177
Eep, just catching up on the discussion here.

If we're calling ContentRemoved directly on the presshell, then my comment 2 is moot. So if that solves the problem then I'd be fine with doing that.

Of course, if the other approach discussed here solves more problems then I'm that might be even better.
(In reply to comment #26)
> Eep, just catching up on the discussion here.
> 
> If we're calling ContentRemoved directly on the presshell, then my comment 2 is
> moot. So if that solves the problem then I'd be fine with doing that.

Does that mean that calling ContentRemoved on the presshell seems like an acceptable solution to you?

> Of course, if the other approach discussed here solves more problems then I'm
> that might be even better.

Well, it's generally saner, in that the editor is not relying on the phase of the moon and bz reminding the next person to rewrite the frame constructor and such about this kind of usage...

And I think a good solution is not very hard to achieve here.  In fact, I had a tree which had a semi-working patch to materialize the plans here, before I got brave playing with rm -rf and accidentally ruined that plus a bunch of my other work...
Given that I haven't dug into this particular problem, and that I'm not an expert on neither the cssframector nor the editor code, I don't have much of an opinion. All I wanted to say was that I don't consider it an unacceptable solution to call ContentRemoved directly with -1 as index.

In sort: Please disregard comment 2 :)
Hmm, so everything was sort of going well, until I came across this problem.  When I tried to create anonymous resizer elements for an image element, I found out that nsImageFrame doesn't have its own implementation of SetInitialChildList, so we fall back to nsFrame::SetInitialChildList which expects the child list to be empty.

In other words, currently, we can't force the frames which do not expect to have anonymous children (I think any frame not derived from nsContainerFrame) to do so.  Which sucks.

Roc, Boris, do you guys have any suggestions on what's the best way to work around this?
Why are we trying to parent the resizer frames to the image?  That makes no sense...  (It wouldn't really make sense for tables either.)
(In reply to comment #30)
> Why are we trying to parent the resizer frames to the image?  That makes no
> sense...  (It wouldn't really make sense for tables either.)

Which element should we stick the anon content under?  I guess we could pick an arbitrary element (like body, etc) but is that a good choice?
The image frame's parent? At least we know that the image frame's parent supports children :-).
Either that or the document element's containing block or somethin.  That might be safer; I think various other situations can be very weird (e.g. per spec the parent of an image box might be a table-row box, and adding kids to that makes things very odd).

> I don't consider it an unacceptable solution to call ContentRemoved directly
> with -1 as index.

I consider that unacceptable, since I'm trying to _remove_ that index argument!
(In reply to comment #33)
> Either that or the document element's containing block or somethin.  That might
> be safer; I think various other situations can be very weird (e.g. per spec the
> parent of an image box might be a table-row box, and adding kids to that makes
> things very odd).

Sigh...

OK, I'll try experimenting with that to see how it goes.

> > I don't consider it an unacceptable solution to call ContentRemoved directly
> > with -1 as index.
> 
> I consider that unacceptable, since I'm trying to _remove_ that index argument!

The current patches that I have will free us from needing to call ContentRemoved all together.

I guess it's time for me to share some code here.  I still don't consider these patches ready for review, but here's a sneak peek:

http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/mq/file/tip/439258-1.patch
http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/mq/file/tip/439258-2.patch
http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/mq/file/tip/439258-3.patch

I _think_ the work left in this bug is fixing up stuff which relied on the exact position of the anonymous content in the tree, and making sure that the try server is happy with me, etc.  I'm also planning to rip out the nsIDOMNodeList changes in part 1 into its own patch before I submit this for review.  But so far I'm fairly happy with the new API design, especially since I got to rewrite it from scratch and avoided some of the mistakes that I made the first time around.  So, I don't expect the API to change significantly, unless one of you guys tells me that it's insane for some reason.
Hmm, using the document element frame wouldn't work either, since scroll frames have assumptions about their child frames...

This makes me not want to mess with the parent element in this bug at all.  I guess I'll just try to use the image's parent frame for now.  At least, doing that, we _shouldn't_ be worse off than we already are.
> Hmm, using the document element frame wouldn't work either

I was talking about using the frame constructor's mDocElementContainingBlock, not mRootElementFrame.
(In reply to comment #36)
> > Hmm, using the document element frame wouldn't work either
> 
> I was talking about using the frame constructor's mDocElementContainingBlock,
> not mRootElementFrame.

Ah.  I didn't try that, but I have things mostly working now, and I don't think that I want to experiment with this idea in this bug, as this change is already getting too large...
Attached file Interactive testcase — 
This is the test case that I've been playing with.  It includes samples for all anonymous content usage inside the editor.
(In reply to comment #36)
> > Hmm, using the document element frame wouldn't work either
> 
> I was talking about using the frame constructor's mDocElementContainingBlock,
> not mRootElementFrame.

Although, it may not be easy/possible to avoid this.  The editor currently tries to attach the table editing UI anonymous nodes to the "root element", which sometimes means the document element, which throws me into the same whole I was trying to dig myself out of in comment 35.

So I guess I have to do what you suggest in comment 36 after all... :(
What is the correct way of sticking frames under mDocElementContainingBlock?  I tried the obvious way <http://pastebin.mozilla.org/1209514> but it's breaking all sorts of assumptions about style contexts, etc.
We'll need to hack style reresolution to handle this situation.  :(  Presumably that's what the style context issues you ran into are about?
(In reply to comment #33)
> Either that or the document element's containing block or somethin.  That might
> be safer; I think various other situations can be very weird (e.g. per spec the
> parent of an image box might be a table-row box, and adding kids to that makes
> things very odd).

Yes, I suppose so.

The problem with using the document element's containing block is that it'll be impossible to get things right if the child is CSS-transformed or something. Or even if the child is just scrolled by a scrollable ancestor.

Maybe we should be adding the resizers to the positioned-child list of the image's nearest abs-pos containing block? Still wouldn't handle the scroll situation.

Ho hum...
I have to admit that sometime while writing comment 41 I started wondering whether the resizers should be elements at all...  :(
One question is whether we want container effects like clipping, filters, opacity etc to apply to the resizer handles or not.

If we mostly don't, we could take an approach where the handles are position:fixed in the document's viewport, we use APIs like getQuads (bug 626359) to put the handles in the right place even when elements are transformed, and we use some notification mechanism to update the positions of the handles to track the elements.
Talking to Ehsan about this on IRC, I think the best way to go might be to refactor absolute positioning so that any frame can be an abs-pos container. We need something like that anyway to fix abs-pos for tables and eventually flexboxes etc. If we really make *any* frame, even leaf frames like nsImageFrame, support abs-pos children, then we can add handles to images by making them abs-pos children of the image and that seems to give us the behavior Ehsan wants in all the cases I can think of.
(In reply to comment #45)
> If we really make *any* frame, even leaf frames like
> nsImageFrame, support abs-pos children

Of course, the frame constructor would still not allow child elements of <img> to have frames constructed for them, so the only way to give an nsImageFrame abs-pos children would be to construct them specially via a mechanism like anonymous content binding.
One nice thing about doing handles and other gadgetry using absolute positioning is that abs-pos elements can't affect each others' layout (except by causing scrollbars to appear, I guess), so there should be minimal disruption to the page.
Depends on: 10209
Where would we put the placeholders in that situation?
Hmm, good question. Maybe a special list just for that purpose?
Hmm.  We can make that work, I guess.
Depends on: 931495
Blocks: 1180036
Testcase no longer asserts on trunk.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → WORKSFORME
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: