SetDocument/SetParent/SetBindingParent should be combined into two methods -- one to bind a node to a tree, and one to unbind it.

One current problem with SetDocument that BindToTree does not solve (iirc) is the ability to change ownerdocument of a node without moving the node into the tree. This is needed for adoptNode and importNode (though peterv is changing the latter). We could always move the node into the new tree and immideatly remove it. However that's not good for things like scripts and stylesheets for obvious reasons. Not sure if we need to solve this immediatly, but it might be worth keeping in mind.

Yeah, BindToTree is all about messing with what GetCurrentDoc() returns... I'm actually adding XXX comments to the places where BindToTree has to be overridden because it might change the ownerDocument.

This is the current state of things. This compiles. It should even avoid asserting on startup, I think... Haven't tested past that, yes. I'm also considering changing the api of UnbindFromTree to eliminate aDeep; need to think that through. Finally, I don't like the XBL setup all that much; need to think about that too.

Still needs nsIContent changes to make that aDeep argument optional, I think, and the XBL issue is still in the air.

This works and all, and I'm even happy with it. This patch is not the one I'd check in -- there are various chunks that are just #if 0'ed out instead of being removed. The main reason for this is that this makes it simpler to review, I think. Otherwise diff makes things pretty darn unreadable... If people would prefer, I can generate and post the patch with the chunks actually removed. Note that once this goes in there are a few followups that I'd like to do (removing the now-useless args from AppendChildTo and InsertChildAt, cleaning up the BF_PARSER_CREATING stuff for input elements, etc).

Other notes: 1) I feel this should be safe enough to do for 1.8, really. So if you guys can do the review before the freeze (and I know that's asking a lot), I'd be very grateful. 2) The patch includes the fix I just landed for bug 233641. I can rediff against the current tree if desired...

Comment on attachment 179075 [details] [diff] [review] Pretty much ready to go jst, Peter isn't going to be back in town until too late for freeze, so if you get the chance to look at this...

This is merged to tip, and I removed the ifdefs so I'll notice if those methods get changed.... If this is hard to read, there are no changes to the impls of the new methods I added, so you can use the previous patch for those.

Starting review...

Comment on attachment 179527 [details] [diff] [review] Patch without the ifdefs and merged to tip. Took me a while, but I had a look through this whole change and I really like it! I didn't find anything to note on, really, except for two style nits in the whole diff! Nits of the day :) - In nsGenericElement::UnbindFromTree() + nsDoc->SetBoxObjectFor( domElement, nsnull); Loose the space after '(' - In nsGenericHTMLFrameElement::UnbindFromTree(): + nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent); + + if (mFrameLoader) { + // This iframe is being taken out of the document, destroy the // iframe's frame loader (doing that will tear down the window in // this iframe). Tab? sr=jst

Comment on attachment 179075 [details] [diff] [review] Pretty much ready to go In nsGenericElement::BindToTree, should you handle the case when the element is passed a null document, but a parent that is in a new document. And then go down the current |if (aDocument != ownerDocument)| codepath? >Index: content/base/public/nsIContent.h ... >+ * Bind this content node to a tree. If this method throws, the >+ * caller must call UnbindFromTree() on the node. It's a little bit bad that you have to manually call UnbindFromTree if BindToTree fails. It'd be nicer if the binding either fails or succeeds "compleatly". But maybe it'll uglify the BindToTree code too much? Oh, and it'd be good to document when bind/unbind are called. Most importantly if it's done before or after the node has been removed/added to the parents childlist. > /** >+ * Unbind this content node from a tree. This will set its current document >+ * and binding parent to null. >+ * @param aDeep Whether to recursively unbind the entire subtree rooted at >+ * this node. The only time PR_FALSE should be passed is when the >+ * parent node of the content is being destroyed. >+ * @param aNullParent Whether to null out the parent pointer as well. This >+ * is usually desirable. Don't pass PR_FALSE unless you know what >+ * you're doing. "know what you're doing" is a big vauge. Would be nice with a better explanation of what this is intended for. Would "Should only be false while recursivly calling UnbindFromTree when a subtree is detached. >Index: content/base/src/nsGenericElement.cpp ... >+nsGenericElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, ... >+ NS_PRECONDITION(!GetBindingParent() || >+ aBindingParent == GetBindingParent() || >+ (aParent && >+ aParent->GetBindingParent() == GetBindingParent()), >+ "Already have a binding parent. Unbind first!"); Shouldn't the last comparison be aBindingPraent == aParent->GetBindingParent() I.e. the case where you're changing the binding parent? ... >+ nsresult rv = >+ nodeInfoManager->GetNodeInfo(mNodeInfo->NameAtom(), >+ mNodeInfo->GetPrefixAtom(), >+ mNodeInfo->NamespaceID(), >+ getter_AddRefs(newNodeInfo)); Style nit: I would just declare rv at top level without giving it a value. But that's a style thing... > void >+nsGenericElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) ... >+ nsCOMPtr<nsIDOMElement> domElement = do_QueryInterface(this); >+ >+ if (domElement) { >+ nsCOMPtr<nsIDOMNSDocument> nsDoc(do_QueryInterface(document)); Another style nit: be consistent about how you create nsCOMPtrs from do_QIs. I prefer the former style personally >Index: content/base/src/nsAttrAndChildArray.cpp ... > PRUint32 end = slotCount * ATTRSIZE + ChildCount(); > for (i = slotCount * ATTRSIZE; i < end; ++i) { > nsIContent* child = NS_STATIC_CAST(nsIContent*, mImpl->mBuffer[i]); >- child->SetParent(nsnull); // XXX is it better to let the owner do this? >+ // making this PR_FALSE so tree teardown doesn't end up being O(N^2). >+ child->UnbindFromTree(PR_FALSE); // XXX is it better to let the owner do this? Why would that make it O(N^2)? You wouldn't step through the same childlist on every unbind call, rather each childs childlist. But it seems like the right thing to do anyway since this should be during teardown of a (sub)tree. Or am I missing some part? Ok, time for bed. I'm on nsXULElement.

> when the element is passed a null document, but a parent that is in a new > document. That's a violation of the API (see the documentation on the aDocument arg for BindToTree in nsIContent.h). So no, I don't need to handle it, and handling it properly in subclasses would be quite painful. > But maybe it'll uglify the BindToTree code too much? Here's the problem.... At what point do you actually unbind? You want to unbind from the point where the "original" bind was called, not from the point where the failure happens. Which means that to handle this completely within BindToTree I'd need to pass along an argument that would indicate whether it's a recursive call or not. I could do that, and remove the "must unbind if bind fails" requirement, but I'm not really sure that's any cleaner. > Oh, and it'd be good to document when bind/unbind are called. Will do. > "know what you're doing" is a big vauge. Will document more clearly. > Shouldn't the last comparison be > aBindingPraent == aParent->GetBindingParent() Actually, I think I should remove the third condition altogether... I'm not quite sure why I put it in -- any time we already have a binding parent and are being bound to a tree, we better be getting bound to our existing binding parent. Will fix style nits. > Why would that make it O(N^2)? If we did a deep unbind here, then destruction of the root content node would be O(N) in the size of the tree (since it'd walk the entire DOM). Then we'd destroy its kids, which would walk all those subtrees again, etc. In the end, we'd walk through each node a number of times equal to the number of ancestors it has. So it's not quite O(N^2), but rather O(N*D) where D is the depth and N is the number of nodes.... I can adjust the comment accordingly, if you would like. Given that by this time the content tree should have been removed from the document (otherwise the document would be holding a ref to the content node, and we wouldn't be in its destructor), the binding parent and document are already cleared, so doing a deep unbind here would have exactly the same effect as doing a shallow one, but take more work.

> I'm not quite sure why I put it in I remember now. The thing is, aBindingParent can be null to indicate "use the parent's binding parent" (see the part right after the assert). So that assert should say: NS_PRECONDITION(!GetBindingParent() || aBindingParent == GetBindingParent() || (!aBindingParent && aParent && aParent->GetBindingParent() == GetBindingParent()), "Already have a binding parent. Unbind first!");

Is there a reason why you don't let nsIDocument::SetRootContent call BindToTree rather then doing it at all callsites of that function? Seems like that would be more in line with how InsertContentAt/AppendContent works.

Comment on attachment 179075 [details] [diff] [review] Pretty much ready to go >Index: content/xbl/src/nsBindingManager.cpp > nsBindingManager::ChangeDocumentFor(nsIContent* aContent, nsIDocument* aOldDocument, > nsIDocument* aNewDocument) > { >+ // XXXbz this code is pretty broken, since moving from one document >+ // to another always passes through a null document! Won't this break moving bound nodes between documents? Or was that broken before too? Though I guess keeping bindings across documents is akward when the binding is done through CSS. >+ >+ // now clear out the anonymous content for this node in the old presshell. >+ shell->SetAnonymousContentFor(aContent, nsnull); Why is this change needed? >Index: content/xbl/src/nsXBLBinding.cpp ... > // (1) The anonymous content should be fooled into thinking it's in the bound > // element's document, assuming that the bound element is in a document ... > // (2) The children's parent back pointer should not be to this synthetic root > // but should instead point to the enclosing parent element. >+ // Note that we don't change the current doc of aAnonParent here, since that >+ // quite simply does not matter. aAnonParent is just a way of keeping refs >+ // to all its kids, which are anonymous content from the point of view of >+ // aElement. Seems like the added comment fits better under (1)? Not related to this patch of course, but if we're just using aAnonParent as a childcontainer, why not just use an nsCOMArray<nsIContent> or something instead? >Index: content/html/content/src/nsGenericHTMLElement.cpp >+nsGenericHTMLElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, ... >+ if (ownerDoc != oldOwnerDoc) { >+ ReparseStyleAttribute(); >+ } Don't you always need to do this since the new position in the DOM could have a different baseURI? Or rather always when ownerDoc != null. >+nsGenericHTMLFormElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) >+{ >+ // Save state before doing anything >+ SaveState(); >+ >+ if (mForm) { >+ // Might need to unset mForm >+ if (aNullParent) { >+ // No more parent means no more form >+ SetForm(nsnull); >+ } else if (aDeep) { >+ // We're probably being recursed into from some ancestor being unbound. >+ // Recheck whether we should still have an mForm. >+ nsCOMPtr<nsIDOMHTMLFormElement> form = FindForm(); >+ if (!form) { >+ SetForm(nsnull); >+ } Wouldn't you want to do this even when aDeep is false? Couldn't you otherwise end up with a dangling pointer? It shouldn't be a perf issue since most of the time mForm will be nulled out when the document goes away. Also, the only time aDeep is false is when the parent goes away (right?) which means that the parent chain is only one item long. >+nsGenericHTMLFrameElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) >+{ >+ nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent); >+ >+ if (mFrameLoader) { >+ // This iframe is being taken out of the document, destroy the >+ // iframe's frame loader (doing that will tear down the window in >+ // this iframe). >+ // XXXbz we really want to only partially destroy the frame >+ // loader... we don't want to tear down the docshell. Food for >+ // later bug. >+ mFrameLoader->Destroy(); >+ mFrameLoader = nsnull; >+ } Not sure if it matters, or if this patch is the place to do it, but in most other places you tear down stuff before calling unbind on the parentclass which makes a bit more sense. >Index: content/html/content/src/nsHTMLFormElement.cpp >+nsHTMLFormElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent, ... >+ if (aDocument) { >+ nsCOMPtr<nsIHTMLDocument> htmlDoc(do_QueryInterface(aDocument)); >+ if (htmlDoc) { >+ htmlDoc->AddedForm(); >+ } >+ } Nit, you'd save yourself a couple of lines of code if you removed the aDocument check. do_QI is fast enough when passed a nullpointer that it shouldn't matter. >+nsHTMLFormElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) >+{ >+ nsIDocument* oldDoc = GetCurrentDoc(); >+ nsCOMPtr<nsIHTMLDocument> oldDocument = do_QueryInterface(oldDoc); >+ >+ nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent); >+ >+ if (oldDoc) { >+ if (oldDocument) { >+ oldDocument->RemovedForm(); >+ } >+ ForgetCurrentSubmission(); >+ } Same here if you move the ForgetCurrentSubmission outside the if. It seems harmless to call that function too much and something we'd always want to do when unbinding... >Index: content/html/content/src/nsHTMLImageElement.cpp ... >- ImageURIChanged(aValue); >+ // Force image loading here, so that we'll try to load the image from >+ // network if it's set to be not cacheable... >+ ImageURIChanged(aValue, PR_TRUE); What's with this force stuff, there's more of it earlier in the patch. Seems unrelated to this bug? Though not neccesarily wrong... >+nsHTMLImageElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) >+{ >+ nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent); >+ >+ // If aDeep is false, the document tree is being destroyed (destructors being >+ // called on the DOM nodes); don't bother with the ImageURIChanged call, >+ // since we're almost certainly about to go away. >+ if (aDeep) { >+ // Our base URI may have changed; claim that our URI changed, and the >+ // nsImageLoadingContent will decide whether a new image load is warranted. >+ nsAutoString uri; >+ nsresult result = GetAttr(kNameSpaceID_None, nsHTMLAtoms::src, uri); >+ if (result == NS_CONTENT_ATTR_HAS_VALUE) { >+ ImageURIChanged(uri, PR_FALSE); >+ } >+ } Why call ImageURIChanged at all on unbind? The old code didn't seem to. >Index: content/html/content/src/nsHTMLInputElement.cpp >+nsHTMLInputElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) ... >+ if (aDeep && mType == NS_FORM_INPUT_IMAGE) { >+ // Our base URI may have changed; claim that our URI changed, and the >+ // nsImageLoadingContent will decide whether a new image load is warranted. >+ nsAutoString uri; >+ nsresult result = GetAttr(kNameSpaceID_None, nsHTMLAtoms::src, uri); >+ if (result == NS_CONTENT_ATTR_HAS_VALUE) { >+ ImageURIChanged(uri, PR_FALSE); >+ } >+ } Same as in nsHTMLImageElement, why do this at all in unbind? Heading home.. finishing the rest when i get there.

> Is there a reason why you don't let nsIDocument::SetRootContent call BindToTree Not past "I didn't think about it." I could do that, either in this patch or a followup one, as you prefer. > Or was that broken before too? Bingo. It's broken, and badly. I'll file a followup bug on this (as on all my other XXX comments) if we don't have one already. > Why is this change needed? It's not, strictly. It's just a general correctness issue -- there should be no presshell-based anonymous content attached to nodes that are not in the presshell's document anymore. It happens to partially fix bug 268513 and I could move this part of the patch to that bug if you prefer (may be a good idea). > Seems like the added comment fits better under (1)? Hmm... Yes. I'll move it. For the rest, at some point we probably want to change XBL's anon node storage to more closely match what sXBL/XBL2 specifies; I really don't see a good reason to mess with it till then. For style attribute, you're right that we should reparse it any time we're bound, in case the base URI changed. I'll just remove the ownerDocument checks there and the comments I put around them. > Wouldn't you want to do this even when aDeep is false? So aDeep is false and so is aNullParent? That really doesn't seem like a sane way to call UnbindFromTree; I should just add asserts that it's not happening. But yes, I could remove this aDeep check, since as you point out it only helps in the trivial case. For the frame element, I'll change the order. Good catch. Will fix nsHTMLFormElement as you suggest. > What's with this force stuff It's needed to keep reloading the image when forced to by the page while not reloading it on unbind. See next part of comment. ;) > Why call ImageURIChanged at all on unbind? The old code didn't seem to. The goal there was to have the image we have loaded depend only on the state of the DOM, not on how the DOM got there... That is, appending an image to a parent and then removing that parent from the DOM (thus possibly changing the base URI of the image) should be identical to removing the parent first, then appending the image. I'm ok with removing this (so keeping current behavior) pending further discussion on the issue. In that case we can also not check in the "force stuff" for now (though we need it for bug 285428 anyway).

(In reply to comment #16) > Not past "I didn't think about it." I could do that, either in this patch or > a followup one, as you prefer. Separate bug sounds like a good idea. > > Why is this change needed? > > It's not, strictly. It's just a general correctness issue -- there should be no > presshell-based anonymous content attached to nodes that are not in the > presshell's document anymore. It happens to partially fix bug 268513 and I > could move this part of the patch to that bug if you prefer (may be a good idea). As you please. I just wanted to understand what it was. wrt the force stuff. I think i'd prefer if we don't call ImageURIChanged on unbind. At least for now. Mostly because it might cause us to fire off unneccesary network loads and it really only optimizes an edgecase. It is a bit bad that the loaded image could depend on where the element used to be, but I think it's a fair thing to do while the node isn't in the main tree.

> Separate bug sounds like a good idea. Bug 289209 filed. > As you please. I just wanted to understand what it was. I'll keep it for now, then, I think. > I think i'd prefer if we don't call ImageURIChanged on unbind. OK. Will do. I'll move the force stuff to bug 285428, then.

Comment on attachment 179075 [details] [diff] [review] Pretty much ready to go >Index: layout/base/nsCSSFrameConstructor.cpp >@@ -5655,68 +5665,65 @@ nsCSSFrameConstructor::CreateAnonymousFr ... > >- content->SetNativeAnonymous(PR_TRUE); >- content->SetParent(aParent); >- content->SetDocument(aDocument, PR_TRUE, PR_TRUE); The SetNativeAnonymous call is lost. That's all. This is a great patch. It's nice to see the messy if-tests in various SetDocument and SetParent calls be replace by much easier to read code. And loosing that extra SetDocument call in the sinks rock!

Comment on attachment 179527 [details] [diff] [review] Patch without the ifdefs and merged to tip. r=sicking

I checked in this bustage fix: --- layout/generic/nsObjectFrame.cpp 5 Apr 2005 23:54:30 -0000 1.496 +++ layout/generic/nsObjectFrame.cpp 6 Apr 2005 00:42:41 -0000 1.497 @@ -1566,5 +1566,5 @@ nsObjectFrame::CreateDefaultFrames(nsPre if (NS_FAILED(rv)) { anchor->UnbindFromTree(); - return rv; + return; } (It's a void method)

the checkin for this bug at 16:54 PDT seems to prevent scrollbars from appearing (with a gtk2 build, at least)

filed bug 289248

So it looks like with the force stuff checked in this was actually a perf improvment. And that there's likly even more improvments to be had by chaning to preappend?

OK. So here's the final performance score: 1) The patch in bug 285428 was needed to not break perf too much, largely because of bug 289311 -- the combination caused images to be loaded multiple times for each <img> tag. 2) With bug 285428 checked in and this patch checked in, there was an issue in non-SVG builds that basically prevented scrollbars from being created. That was fixed in bug 289248, but that affected Tp. 3) The net result is that Ts and Txul look unchanged, Tp has regressed somewhat. 4) Bug 289311 should bring Tp back down to something we can live with. I've filed (or we already have) the following bugs on the followup issues (the ones I have XXX comments on in the patch): Bug 289311 -- Tp regression Bug 286619 -- Event handlers for XUL elements that are being moved in/out of documents. Bug 289316 -- Removing the unused aDeepSetDocument args on nsIContent Bug 289317 -- nsBindingManager::ChangeDocumentFor is horked Bug 289318 -- XUL not playing nice with ranges Bug 289319 -- HTML's ReparseStyleAttribute not necessarily doing the right thing. Bug 289322 -- Removing the BF_PARSER_CREATING check in nsHTMLInputElement::BindToTree Bug 289209 -- Moving the BindToTree call on the root node into SetRootContent.

Tp is back to something sane; we have about a 1% overall Tp win from this patch and followups... Marking fixed; remaining regressions will go in bugs blocking this one.

Keyword "perf"?

No. This bug wasn't about a performance issue.

(In reply to comment #26) > OK. So here's the final performance score: > > 1) ... > 2) ... > 3) ... > 4) ... ... (In reply to comment #29) > No. This bug wasn't about a performance issue. I'm confused.

> I'm confused. Yes, you are. I suggest taking a look at comment 26 item 1 again, and then go look at the Tp graphs for the day when this patch was checked in to get an idea of why performance was being discussed at all.