Closed Bug 286000 Opened 19 years ago Closed 19 years ago

[FIXr]Implement BindToTree

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Checkpoint (obsolete) — Splinter Review
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.
Depends on: 233641
Blocks: 268513
Attached patch Another checkpoint (obsolete) — Splinter Review
Still needs nsIContent changes to make that aDeep argument optional, I think,
and the XBL issue is still in the air.
Attachment #177540 - Attachment is obsolete: true
Blocks: 285428
Attached patch Pretty much ready to go (obsolete) — Splinter Review
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).
Attachment #178336 - Attachment is obsolete: true
Attachment #179075 - Flags: superreview?(peterv)
Attachment #179075 - Flags: review?(bugmail)
Summary: Implement BindToTree → [FIX]Implement BindToTree
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...
Depends on: 286619
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...
Attachment #179075 - Flags: superreview?(peterv) → superreview?(jst)
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.
Attachment #179075 - Attachment is obsolete: true
Attachment #179527 - Flags: superreview?(jst)
Attachment #179527 - Flags: review?(bugmail)
Attachment #179075 - Flags: superreview?(jst)
Attachment #179075 - Flags: review?(bugmail)
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
Attachment #179527 - Flags: superreview?(jst) → superreview+
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!");
Blocks: 289175
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.
No longer blocks: 289175
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.
Blocks: 289175
> 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.
Blocks: 289209
> 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
Attachment #179527 - Flags: review?(bugmail) → review+
Attachment #179527 - Attachment is obsolete: true
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
Summary: [FIX]Implement BindToTree → [FIXr]Implement BindToTree
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?
Depends on: 289248
Depends on: 289311
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.
No longer depends on: 286619
Blocks: 289391
Depends on: 289379
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.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Depends on: 289456
Target Milestone: --- → mozilla1.8beta2
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.
Depends on: 291042
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: