Peter Van der Beken [:peterv] Assignee
	Comment 3 • 18 years ago

Comment on attachment 223495 [details] [diff] [review]
v1.1

I don't call BindToTree in AdoptNode. I think I don't have to, correct me if I'm wrong. I also didn't add a call to nsIBindingManager::ChangeDocumentFor in AdoptInto. I think we do want that eventually, no? I'm not adding it here because after this patch AdoptInto is being called by BindToTree. I plan to remove that call when I land my patch for bug 47903.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 18 years ago

Comment on attachment 223495 [details] [diff] [review]
v1.1

>Index: content/base/public/nsINode.h

>+   * Give this node a new ownerDocument.

Does this recurse and set the new ownerDocument on he descendants?  Or no?  That should be documented, imo.

I wish we could make this method protected, but I guess we're calling it from various static methods... Could we make it protected and make those static methods class statics as needed?

>+   * @param aCallHandler whether to call UserDataHandlers

This should probably document that those would execute scripts, right?  Frankly, I think we should not enable NODE_ADOPTED handlers until we have a way to delay execution to a safe point.  For example, consider what happens if we call adoptNode back into a different document for parts of a subtree while under AdoptNodeAndDescendants or some such...

>+  virtual nsresult AdoptInto(nsIDocument *aOwnerDocument,
>+                             PRBool aCallHandler = PR_TRUE);

Let's make the aCallHandler arg required, please?  There's only one caller that doesn't pass an explicit value, in any case.

>Index: content/base/src/nsDOMAttributeMap.cpp

>+  PRUint32 n = mAttributeCache.Enumerate(AdoptIntoFunc, &data);
>   NS_ENSURE_TRUE(n == mAttributeCache.Count(), NS_ERROR_FAILURE);

So if we run NODE_ADOPTED UserData handlers then the hashtable can get modified during the enumeration, right?  That's another reason to not run those handlers for the time being.

>+    // XXX This needs to be removed once we throw the right exceptions when
>+    //     somebody tries to move a node into a document without calling
>+    //     adoptNode or importNode.

Reference the bug# here?  And mention this code in that bug?

>Index: content/base/src/nsDocument.cpp
>+AdoptNodeAndDescendants(nsINode *aAdoptedNode, nsIDocument *aOwnerDocument)
>+{
>+  nsresult rv = aAdoptedNode->AdoptInto(aOwnerDocument);
>+  NS_ENSURE_SUCCESS(rv, rv);

So if adoption fails at some point, we'll end up with a partially-adopted tree that contains nodes with two different ownerDocuments in it?  I'm not so happy with that, but I don't see a good way to handle it, since reverting back to the original ownerDocument could also fail...

The only thing that could really fail when calling AdoptNode() is creating the new nodeinfo, right?  Could we have AdoptNode() return the old nodeinfo and have a way to restore to the old nodeinfo or something?  I guess the problem is that we'd need to restore descendants of already-adopted nodes too.  :(

Maybe the right thing to do is to build a tree of nodeinfo/nodepointer pair structs or something, then once we successfully do that do a second tree walk and change all the mNodeInfo pointers with a non-failing (void) method?

>+nsDocument::AdoptNode(nsIDOMNode *aAdoptedNode, nsIDOMNode **aResult)

>+  nsresult rv = nsContentUtils::CheckSameOrigin(this, aAdoptedNode);

So chrome can adopt nodes from any document into any other document, right?  I guess that makes sense...  There's also asymmetry in CheckSameOrigin, but I plan to remove that, so let's not worry about it for now.

>+  nsCOMPtr<nsINode> adoptedNode;

This is unused -- you redeclare it in the two cases that use it.

>+      nsCOMPtr<nsIDOMElement> ownerElement;
>+      adoptedAttr->GetOwnerElement(getter_AddRefs(ownerElement));
>+      NS_ENSURE_SUCCESS(rv, rv);

Assign to rv when calling GetOwnerElement?

>Index: content/base/src/nsGenericDOMDataNode.cpp
>+  nsresult rv = AdoptInto(newOwnerDocument, PR_FALSE);

So this is always called, right?

>Index: content/base/src/nsGenericElement.cpp

>+nsINode::AdoptInto(nsIDocument *aOwnerDocument, PRBool aCallHandler)

>+  nsIDocument *oldOwnerDocument = GetOwnerDoc();

Should this bail out right away if oldOwnerDocument == aOwnerDocument?  Otherwise we'll lose all properties, no?

>+  if (oldOwnerDocument && HasFlag(NODE_HAS_PROPERTIES)) {

HasProperties() (like generic DOM data node used to have).

>+  if (aOwnerDocument) {

This isn't quite what the code used to do for the "auto-import" case.  In particular, when appending a node which has an ownerDocument as a child of a node whose ownerDocument is null (but which has a nodeinfo manager), we used to properly change the nodeinfo, and hence the principal.  With this patch we no longer do.

I guess we don't care because we plan to enforce that such mutations have the same (non-null) ownerDocument, right?

>+    if (aCallHandler && HasFlag(NODE_HAS_PROPERTIES)) {

Again, HasProperties().  And again, I think we shouldn't do this for now.

	Peter Van der Beken [:peterv] Assignee
	Comment 5 • 18 years ago

(In reply to comment #4)
> (From update of attachment 223495 [details] [diff] [review] [edit])
> >Index: content/base/public/nsINode.h
> 
> >+   * Give this node a new ownerDocument.
> 
> Does this recurse and set the new ownerDocument on he descendants?  Or no? 
> That should be documented, imo.

I doesn't recurse, it sets the ownerDocument on the node, and its attributes if it's an element. We could make it recurse if you prefer that.

> >+  virtual nsresult AdoptInto(nsIDocument *aOwnerDocument,
> >+                             PRBool aCallHandler = PR_TRUE);
> 
> Let's make the aCallHandler arg required, please?  There's only one caller that
> doesn't pass an explicit value, in any case.

No, the boolean goes away once we don't call this from BindToTree anymore.

> Maybe the right thing to do is to build a tree of nodeinfo/nodepointer pair
> structs or something, then once we successfully do that do a second tree walk
> and change all the mNodeInfo pointers with a non-failing (void) method?

I thought about it, none of the solutions seem very attractive. The point to adoptNode is that it's supposed to be fairly cheap :-/.

> >Index: content/base/src/nsGenericDOMDataNode.cpp
> >+  nsresult rv = AdoptInto(newOwnerDocument, PR_FALSE);
> 
> So this is always called, right?

Yes, AdoptInto can deal.

> >Index: content/base/src/nsGenericElement.cpp
> 
> >+nsINode::AdoptInto(nsIDocument *aOwnerDocument, PRBool aCallHandler)
> 
> >+  nsIDocument *oldOwnerDocument = GetOwnerDoc();
> 
> Should this bail out right away if oldOwnerDocument == aOwnerDocument? 
> Otherwise we'll lose all properties, no?

Hmm, I dropped a |aOwnerDocument != oldOwnerDocument| when rearranging that code. Too many conditions.

> >+  if (oldOwnerDocument && HasFlag(NODE_HAS_PROPERTIES)) {

> >+  if (aOwnerDocument) {
> 
> This isn't quite what the code used to do for the "auto-import" case.  In
> particular, when appending a node which has an ownerDocument as a child of a
> node whose ownerDocument is null (but which has a nodeinfo manager), we used to
> properly change the nodeinfo, and hence the principal.  With this patch we no
> longer do.
> 
> I guess we don't care because we plan to enforce that such mutations have the
> same (non-null) ownerDocument, right?

I think so, yes (and when calling adoptNode we always have a document).

I was trying to minimize the codesize impact by making BindToTree call AdoptInto, maybe I shouldn't bother and leave that code alone instead.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 18 years ago

> We could make it recurse if you prefer that.

Nah, I just want the comment to say what it does.  ;)

Calling AdoptInto from BindToTree makes a lot of sense to me.

I'd still really like to figure out a way to safely handle failures partway through an adopt... :(

sicking, any ideas?

	Peter Van der Beken [:peterv] Assignee
	Comment 7 • 18 years ago

I have a patch that does the two passes, one to gather and one to set the nodeinfos. It doesn't look too bad, I'll attach it soon.

For the UserDataHandler I could make the handlers be called from an event. I have it mostly coded up (currently I use one nsIRunnable per node, not sure if that makes sense). Let me know if I should include that or if you still prefer to postpone that part.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 18 years ago

> Let me know if I should include that or if you still prefer to postpone that
> part.

Whatever is easiest for you works for me.  I assume the userdata handlers don't guarantee firing order wrt each other or anything else going on, right?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 18 years ago

One way to deal with the UserDataHandlers is to do what we should do for the deleted notification, i.e. fire the handlers asynchronously after the operation is done.

Alternativly, you could first do the adoption, and in a second pass walk the resulting tree and fire any handlers.

	Peter Van der Beken [:peterv] Assignee
	Comment 10 • 18 years ago

(In reply to comment #9)
> One way to deal with the UserDataHandlers is to do what we should do for the
> deleted notification, i.e. fire the handlers asynchronously after the operation
> is done.

The spec doesn't mention it for adoptNode, but for importNode it says: "However, if any UserDataHandlers has been specified along with the associated data these handlers will be called with the appropriate parameters before this method returns.".

> Alternativly, you could first do the adoption, and in a second pass walk the
> resulting tree and fire any handlers.

Which doesn't solve any of the issues that bz mentioned.

I've tried at least four different approaches now :-/. The one I ended up with goes as follows:

- Walk the subtree. If adopting into a different document: create a new nodeinfo for the node, set it on the node and store the old nodeinfo in an array. If the node has properties, store it in an array. Bail out if anything fails.
- If something failed: walk over the tree and set back the old nodeinfos. Bail out once the array of old nodeinfos is empty.
- Loop over the array with nodes that have properties. If adopting into a different document: copy the userdata properties and delete the other properties. Call the userdata handlers if there are any.

It's more code than I'd like, but we don't leave the tree in an inconsistent state when something fails, we only walk the tree once unless something fails, we only do the properties stuff if there are nodes with properties and calling a handler can't mess anything up, because that array is immutable at that point. The downside is that we'll have an array of nodeinfos as big as the number of nodes being adopted (if adopting into a different node) plus another array of nodes as big as the number of nodes being adopted that have properties.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 11 • 18 years ago

> > Alternativly, you could first do the adoption, and in a second pass walk the
> > resulting tree and fire any handlers.
> 
> Which doesn't solve any of the issues that bz mentioned.

It solves the issue of being in a consistent state when calling script. But no, it doesn't solve the OOM issue.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 18 years ago

Comment on attachment 223495 [details] [diff] [review]
v1.1

I'd love to see the updated patch whenever it's set.  ;)

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 18 years ago

Comment on attachment 226495 [details] [diff] [review]
v2

>Index: content/base/src/nsDocument.cpp
>+nsDocument::Adopt(nsINode *aNode, nsNodeInfoManager *aNodeInfoManager,

>+      PRBool ok = map->Enumerate(AdoptFunc, &data);
>+      NS_ENSURE_SUCCESS(ok, NS_ERROR_FAILURE);

NS_ENSURE_TRUE

>+    PRBool ok = aOldNodeInfos.AppendObject(aNode->mNodeInfo);
>+    NS_ENSURE_SUCCESS(ok, NS_ERROR_OUT_OF_MEMORY);

Same.

>+nsDocument::RestoreNodeInfos(nsINode *aNode,
>+      map->Enumerate(AdoptFunc, aNodeInfos);

RestoreNodeInfosFunc

>+struct AdoptUserDataHandlerFuncData {

This is unused.

r+sr=bzbarsky with those nits

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 18 years ago

One more question.  Don't we need to reparent the XPConnect wrappers?  I suspect that doing that might force us to go sicking's way, unless we hack nsContentUtils some.... :(

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 16 • 18 years ago

Yes, you do need to reparent the wrappers.

Btw, I had one idea, though i'm not at all sure it is a good one: When we do run into an error (which can only be OOM, right?) we simply shoot the entire subtree into bits. I.e. remove all children from all nodes in the tree.

This way we'll be protected from exploitable crashes since each individual node will be in a valid state, whether it has been successfully moved or not.

Again, I'm not at all convinced this is a good idea. Basically I think we should only do it if errors only happens in very extream cases (like OOM), and if it turns out to be very hard to recover from the error.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 18 years ago

Comment on attachment 227491 [details] [diff] [review]
v3

>Index: content/base/public/nsContentUtils.h
>+  static nsresult GetContextAndParent(nsIDocument *aNewDocument,
>+                                      nsIDocument *aOldDocument,
>+                                      JSContext **aCx, JSObject **aScope,
>+                                      JSObject **aNewParent);

Wouldn't this be better as GetContextAndScopes, with the last two args being aOldScope and aNewScope?  That's what this thing returns, after all...   Might want to document what this returns too.

>Index: content/base/src/nsContentUtils.cpp
>+  JSObject *globalObj = nsnull, *newScope = nsnull;

And here s/globalObj/oldScope/

>+nsContentUtils::GetContextAndParent(nsIDocument *aNewDocument,
>+                                    nsIDocument *aOldDocument, JSContext **aCx,
>+                                    JSObject **aScope, JSObject **aNewParent)
>+{
>+  JSObject *newParent = nsnull;

This should probably set *aNewParent = *aScope = *aCx = nsnull (as different statements, etc).

And probably s/newParent/newScope/ here.

>Index: content/base/src/nsDOMAttributeMap.h
>+  PRBool Enumerate(AttrCache::EnumReadFunction aFunc, void *aUserArg) const;

Document what the return value means?

>Index: content/base/src/nsDocument.cpp

>+struct AdoptFuncData {
>+  nsNodeInfoManager *mNodeInfoManager;

Maybe mNewNodeInfoManager?  Not sure about that; might be clear enough if just documented.

>+  JSObject *mScope;
>+  JSObject *mNewParent;

mOldScope / mNewScope?

>+  // Note that the restore algorithm in nsDocument::RestoreNodeInfos needs to be
>+  // identical to this one.

No longer relevant, right?

>+  // aNode's attributes.
>+  if (aNode->IsNodeOfType(eELEMENT)) {

Swap those two lines.

>+      if (NS_FAILED(rv)) {
>+        newNodeInfo.swap(aNode->mNodeInfo);

and return rv, right?

>+BlastSubtreeToPieces(nsINode *aNode)
>+  if (aNode->IsNodeOfType(nsINode::eATTRIBUTE)) {
>+      rv = ownerElement->RemoveAttributeNode(adoptedAttr,
>+                                             getter_AddRefs(newAttr));

So doesn't this modify the hashtable while we're enumerating it?  I thought we were going to avoid that....  I see you're passing an array to the hashtable enumeration, but not using it; should BlastFunc be AppendFunc and just append to the array, then we blast in a separate loop over the array?

>+        nsCOMArray<nsINode> attributes;
>+        PRBool ok = map->Enumerate(BlastFunc, &attributes);

So here we want to make use of the array.

>+  count = aNode->GetChildCount();
>+  for (i = 0; i < count; ++i) {
>+    rv = BlastSubtreeToPieces(aNode->GetChildAt(i));

This is no good, since we'll remove the kids as we go.  We probably want something more like:

  while (aNode->GetChildCount()) {
    rv = BlastSubtreeToPieces(aNode->GetChildAt(0));
  }

>+nsDocument::AdoptNode(nsIDOMNode *aAdoptedNode, nsIDOMNode **aResult)
>+      // Remove from ownerElement.

I really wish we could factor out this code better; we have it in two spots now...

>+    // Disconnect all nodes from their parents, since some have the old document
>+    // as their ownerDocument and some have this as their ownerDocument.
>+    BlastSubtreeToPieces(adoptedNode);

Would it also make sense to DeleteAllPropertiesFor() on all the old nodes too here?  Would need to pass oldDocument to BlastSubtreeToPieces, but....

If this is a non-issue (that is, if properties won't end up holding weak refs to nodes or some such), then don't worry about it.

>+    nsContentUtils::CallUserDataHandler(this,
>+                                        nsIDOMUserDataHandler::NODE_ADOPTED,
>+                                        nodeWithProperties, source, nsnull);

Hmm...  It occurs to me that this could trigger GC and we're not necessarily holding refs to all the nodes with properties.  Perhaps that should be an nsCOMArray<nsINode> instead?

>Index: content/base/src/nsDocument.h
>+   * Walks the attribute and desdendant nodes of aNode. If aNodeInfoManager is

"descendant"

>+   * not null, it is used to create new nodeinfos for the nodes and the
>+   * existing nodeinfos are collected in aOldNodeInfos. aNodesWithProperties
>+   * will be filled with all the nodes that have properties.

Mention what the aCx, etc args are?

	Peter Van der Beken [:peterv] Assignee
	Comment 20 • 18 years ago

Comment on attachment 227785 [details] [diff] [review]
v3.1

Back to bz :-/.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 18 years ago

I should get to this in the next few days, but I have one interesting question.  What should happen if someone does:

var iframe = document.getElementsByTagName("iframe")[0];
var doc = iframe.contentDocument;
doc.adoptNode(iframe);
doc.documentElement.appendChild(iframe);

or some such?  At the moment, removing the iframe from the DOM (which adoptNode will do) will kill the docshell inside it and effectively leave |doc| alive only because of GC, but longer-term we want to change that behavior, I think... I guess this should be a followup bug or something.

	Peter Van der Beken [:peterv] Assignee
	Comment 22 • 18 years ago

(In reply to comment #21)
> I should get to this in the next few days, but I have one interesting question.
>  What should happen if someone does:
> 
> var iframe = document.getElementsByTagName("iframe")[0];
> var doc = iframe.contentDocument;
> doc.adoptNode(iframe);
> doc.documentElement.appendChild(iframe);
> 
> or some such?  At the moment, removing the iframe from the DOM (which adoptNode
> will do) will kill the docshell inside it and effectively leave |doc| alive
> only because of GC, but longer-term we want to change that behavior, I think...
> I guess this should be a followup bug or something.

How is that different from:

 var iframe = document.getElementsByTagName("iframe")[0];
 var doc = iframe.contentDocument;
 iframe = iframe.parentNode.removeChild(iframe);
 doc.documentElement.appendChild(iframe);

I don't see why/how it's related to this bug.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 18 years ago

It's related because in general we may want to throw if a node is adopted into a document that's in the frame subtree rooted at that node.  Right now it's not an issue because it's impossible to really perform such an adoption, but that may change.

	Peter Van der Beken [:peterv] Assignee
	Comment 24 • 18 years ago

I'm still unclear how this is different from the removeChild case.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 18 years ago

The removeChild case, as written, should throw WRONG_DOCUMENT_ERR.  ;)  As in, if we didn't suck the only way to try to produce an iframe that owns itself would be via adoptNode.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 18 years ago

Comment on attachment 227785 [details] [diff] [review]
v3.1

>Index: content/base/public/nsContentUtils.h
>+   * Get the a scope from aOldDocument and one from aNewDocument. Also get a

s/the a/a/

>Index: content/base/src/nsDocument.cpp
>+nsDocument::Adopt(nsINode *aNode, nsNodeInfoManager *aNewNodeInfoManager,
>+                  JSContext *aCx, JSObject *aOldScope, JSObject *aNewScope,
>+                  nsCOMArray<nsINode> &aNodesWithProperties)

add

NS_PRECONDITION(!aCx || (aOldScope && aNewScope), "Must have scopes");

>+static nsresult
>+BlastSubtreeToPieces(nsINode *aNode)

So... this should realistically be a void method, since this is error-recovery -- we need to make sure it succeeds.

Specifically:

>+      nsCOMArray<nsINode> attributes;
>+      PRBool ok = map->Enumerate(BlastFunc, &attributes);
>+      NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);

This fails to blast the attr nodes.  The only issue here is that we might fail to allocate array entries, right?  Maybe we should change this to something more like:

  while (have attributes) {
    nsCOMPtr<nsINode> attr;
    map->Enumerate(BlastFunc, &attr);
    // UnsetAttr the one attr
  }

And have BlastFunc assign its aData arg into the nsCOMPtr and return PL_DHASH_STOP (so we only "enumerate" one node at a time).  This seems like it couldn't fail...

Similar for the other early returns in this method -- we need to press on in the face of error returns from UnsetAttr or RemoveChild.

Another option is to call exit() when these fail -- we'll "crash", but at least not be exploitable...
  
>+nsDocument::AdoptNode(nsIDOMNode *aAdoptedNode, nsIDOMNode **aResult)
>+    case nsIDOMNode::ELEMENT_NODE:
...
>+        newChild.swap(aAdoptedNode);

Since aAdoptedNode is not an nsCOMPtr, this releases newChild one too few times and releases aAdoptedNode one too many times.  Now it happens not to matter because the two are actually the same node.  But in that case, why are we bothering with the swap?

You might want to just use an nsCOMPtr<nsINode> here like you use the nsCOMPtr<nsIAttribute> in the attribute case.

>+  if (NS_FAILED(rv)) {
>+    // Disconnect all nodes from their parents, since some have the old document
>+    // as their ownerDocument and some have this as their ownerDocument.
>+    BlastSubtreeToPieces(adoptedNode);

Do we need to worry about deleting properties on the old document for the nodes that got moved to the new document?  They should be in nodesWithProperties, right?

>+  PRUint32 i, count = nodesWithProperties.Count();
>+  for (i = 0; i < count; ++i) {
...
>+    nsContentUtils::CallUserDataHandler(this,
>+                                        nsIDOMUserDataHandler::NODE_ADOPTED,
>+                                        nodeWithProperties, source, nsnull);

So what if this handler takes one of our other nodes with properties and imports it into some third document?  It looks to me like we'll not copy properties correctly then...  We should probably loop once copying and deleting properties, and then loop a second time calling userdata handlers.

>Index: content/base/src/nsDocument.h
>+   * Walks the attribute and desdendant nodes of aNode. If aNewNodeInfoManager

s/desdendant/descendant/

r+sr=bzbarsky with these issues fixed.

	Peter Van der Beken [:peterv] Assignee
	Comment 27 • 18 years ago

(In reply to comment #23)
> It's related because in general we may want to throw if a node is adopted into
> a document that's in the frame subtree rooted at that node.

I tried adding this, but I think it might require two loops over the children, one to check for (i)frame and object elements and one to set the new nodeinfo. I tried adding the check for (i)frame and object in the nodeinfo loop, but that means we'd blow the subtree to pieces if someone tries to adopt a node into a document that's in the frame subtree rooted at that node. I don't think we'd want to disconnect the subtree in that case, right?
Also, I need to QI every element to nsIDOMHTMLFrameElement, nsIDOMHTMLIFrameElement and nsIDOMHTMLObjectElement. I think that covers all the cases? (We should probably have a GetContentDocument on nsIContent to make this all a bit simpler)

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 18 years ago

> I tried adding this, but I think it might require two loops over the children,

Er... why not just walk the parentDocument (or window parent, but that doesn't cross chrome boundaries unless you do it right) chain from |this| and look at the frameElementInternal of those windows?  If any of those matches the node being adopted, bail.

	Peter Van der Beken [:peterv] Assignee
	Comment 29 • 18 years ago

(In reply to comment #28)
> Er... why not just walk the parentDocument (or window parent, but that doesn't
> cross chrome boundaries unless you do it right) chain from |this| and look at
> the frameElementInternal of those windows?  If any of those matches the node
> being adopted, bail.

But they can also match one of the descendants of the node being adopted, so I'd still need to add a loop.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 18 years ago

Ah, so instead of equality you'd need to check nsContentUtils::IsDescendantOf.

	Peter Van der Beken [:peterv] Assignee
	Comment 31 • 18 years ago

Hopefully the final one.

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 18 years ago

Comment on attachment 230815 [details] [diff] [review]
v3.2

>Index: content/base/src/nsDocument.cpp
>+BlastFunc(nsAttrHashKey::KeyType aKey, nsIDOMNode *aData, void* aUserArg)
...
>+  nsresult rv = element->UnsetAttr(attr->NodeInfo()->NamespaceID(),
>+                                   attr->NodeInfo()->NameAtom(), 

This removes the attr from the hashtable in the middle of the hashtable enumeration...  Not good.

I was pretty serious about enumerating only one attr at a time from the hashtable.  The other option is to just UnsetAttr for all the attrs the element has, whether they have nsIDOMAttrs or not.  That might be the simplest thing to do...

r+sr=bzbarsky with this issue fixed.

	Peter Van der Beken [:peterv] Assignee
	Comment 33 • 18 years ago

Sorry for being a bit absent-minded when I did the last one. Here's what I checked in.

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 18 years ago

We should probably add some tests to content/tests/unit testing this...

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 18 years ago

Let's do the tests in bug 346627.