737976 - deCOMtaminate nsINodeInfo

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 608044 [details] [diff] [review]
Patch

bent gave me an rs for this.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b00bf7f3869c

Comment 3

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b00bf7f3869c - something started leaking in mochitest-1, sometimes including a domwindow from a media test and sometimes not, beginning with your push.

Comment 4

[Phil Ringnalda (:philor)]

Among the various possibilities is that the leaks actually came from bug 633602 or one of the things in that push that I also backed out, so I pushed you to try in https://tbpl.mozilla.org/?tree=Try&rev=7e3313380616

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Hmm, looks like it was me :-(

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

I tried to be consistent here and use nsINode::Info in anything already already using nsINode.h, otherwise forward declare mozilla::dom::NodeInfo.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 795550 [details] [diff] [review]
Patch


> #define NS_IMPL_ELEMENT_CLONE_WITH_INIT(_elementName)                       \
> nsresult                                                                    \
>-_elementName::Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const        \
>+_elementName::Clone(nsINode::Info *aNodeInfo, nsINode **aResult) const  \
> {                                                                           \
>-  *aResult = nullptr;                                                        \
>-  nsCOMPtr<nsINodeInfo> ni = aNodeInfo;                                     \
>+  *aResult = nullptr;                                                       \
>+  nsRefPtr<nsINode::Info> ni = aNodeInfo;                               \
>   _elementName *it = new _elementName(ni.forget());                         \
>   if (!it) {                                                                \
>     return NS_ERROR_OUT_OF_MEMORY;                                          \
>   }                                                                         \
Align \


> class nsINode : public mozilla::dom::EventTarget
> {
> public:
>   NS_DECLARE_STATIC_IID_ACCESSOR(NS_INODE_IID)
> 
>+  typedef mozilla::dom::NodeInfo Info;
So this makes the code hard to understand.
One needs to know that there is NodeInfo and that nsINode::Info is the same thing.
There is no need for nsINode::Info, or if we want that, 'class Info' could be declared inside nsINode.
But please, no both Info and NodeInfo

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8442269 [details] [diff] [review]
Patch

Changes to NameSpaceManager look rather unrelated, but fine.

Feels odd to move NodeInfo to dom/base before nsINode, but ok, no need to do the
move later.

I'm not going to complain about wrong style for *. (it should go with the type)

You could have used dom::NodeInfo in many places, no?

> nsNodeInfoManager::GetNodeInfoInnerHashValue(const void *key)
> {
>-  NS_ASSERTION(key, "Null key passed to nsNodeInfo::GetHashValue!");
>+  MOZ_ASSERT(key, "Null key passed to NodeInfo::GetHashValue!");
> 
>-  const nsINodeInfo::nsNodeInfoInner *node =
>-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key);
>+  auto *node = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key);
> 
>   return node->mName ? node->mName->hash() : HashString(*(node->mNameString));
> }
> 
> 
> int
> nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> {
>-  NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
>+  MOZ_ASSERT(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> 
>-  const nsINodeInfo::nsNodeInfoInner *node1 =
>-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key1);
>-  const nsINodeInfo::nsNodeInfoInner *node2 =
>-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key2);
>+  auto *node1 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key1);
>+  auto *node2 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key2);

There is really no reason for auto usage here.


>-  already_AddRefed<nsINodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
>+  already_AddRefed<mozilla::dom::NodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
>                                             int32_t aNamespaceID,
>                                             uint16_t aNodeType,
>                                             nsIAtom* aExtraName = nullptr);
Please fix indentation






>     nsPrototypeArray         mChildren;
> 
>-    nsCOMPtr<nsINodeInfo>    mNodeInfo;           // [OWNER]
>+    nsRefPtr<mozilla::dom::NodeInfo>    mNodeInfo;           // [OWNER]
No need for so many empty spaces between > and m


>+NodeInfo::GetName(nsAString& aName) const
>+{
>+  mInner.mName->ToString(aName);
>+}
>+
>+void
>+NodeInfo::GetPrefix(nsAString& aPrefix) const
>+{
>+  if (mInner.mPrefix) {
>+    mInner.mPrefix->ToString(aPrefix);
>+  } else {
>+    SetDOMStringToNull(aPrefix);
>+  }
>+}
Why you un-inline these?
Could you just move to NodeInfoInlines.h ?

>- * nsINodeInfo is an interface to node info, such as name, prefix, namespace
>- * ID and possibly other data that is shared between nodes (elements
>- * and attributes) that have the same name, prefix and namespace ID within
>- * the same document.
>+ * Class that represents a prefix/namespace/localName triple; a single
>+ * nodeinfo is shared by all elements in a document that have that
>+ * prefix, namespace, and localName.
That isn't quite accurate description, since there is also qname etc.
So perhaps s/ triple/\/etc. /

>  *
>  * nsNodeInfoManager's are internal objects that manage a list of
>- * nsINodeInfo's, every document object should hold a strong reference to
>- * a nsNodeInfoManager and every nsINodeInfo also holds a strong reference
>- * to their owning manager. When a nsINodeInfo is no longer used it will
>+ * NodeInfo's, every document object should hold a strong reference to
Shouldn't it be nsNodeInfoManagers are....
manage a list of NodeInfos.




>+ * a nsNodeInfoManager and every NodeInfo also holds a strong reference
>+ * to their owning manager. When a NodeInfo is no longer used it will
>  * automatically remove itself from its owner manager, and when all
>- * nsINodeInfo's have been removed from a nsNodeInfoManager and all external
>+ * NodeInfo's have been removed from a nsNodeInfoManager and all external
Shouldn't this be NodeInfos

>-  nsNodeInfoManager *NodeInfoManager() const
>+  nsNodeInfoManager* NodeInfoManager() const
Ooh, correct style here

> 
>   // nsNodeInfoManager needs to pass mInner to the hash table.
>-  friend class nsNodeInfoManager;
>+  friend class ::nsNodeInfoManager;
Hmm, :: is needed here, I guess

>  
>+++ b/dom/xslt/xslt/txMozillaTextOutput.cpp
>@@ -14,16 +14,17 @@
> #include "nsCharsetSource.h"
> #include "nsIPrincipal.h"
> #include "txURIUtils.h"
> #include "nsContentCreatorFunctions.h"
> #include "nsContentUtils.h"
> #include "nsGkAtoms.h"
> #include "mozilla/dom/EncodingUtils.h"
> #include "nsTextNode.h"
>+#include "nsNamespaceManager.h"
Is this really needed?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8442269 [details] [diff] [review]
> Patch
> 
> Changes to NameSpaceManager look rather unrelated, but fine.
> 
> Feels odd to move NodeInfo to dom/base before nsINode, but ok, no need to do
> the
> move later.
> 
> I'm not going to complain about wrong style for *. (it should go with the
> type)

Yeah, I didn't want to try to change the world.

> You could have used dom::NodeInfo in many places, no?

Yes.  It would have made this a lot less automatic to write though.
 
> > nsNodeInfoManager::GetNodeInfoInnerHashValue(const void *key)
> > {
> >-  NS_ASSERTION(key, "Null key passed to nsNodeInfo::GetHashValue!");
> >+  MOZ_ASSERT(key, "Null key passed to NodeInfo::GetHashValue!");
> > 
> >-  const nsINodeInfo::nsNodeInfoInner *node =
> >-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key);
> >+  auto *node = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key);
> > 
> >   return node->mName ? node->mName->hash() : HashString(*(node->mNameString));
> > }
> > 
> > 
> > int
> > nsNodeInfoManager::NodeInfoInnerKeyCompare(const void *key1, const void *key2)
> > {
> >-  NS_ASSERTION(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> >+  MOZ_ASSERT(key1 && key2, "Null key passed to NodeInfoInnerKeyCompare!");
> > 
> >-  const nsINodeInfo::nsNodeInfoInner *node1 =
> >-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key1);
> >-  const nsINodeInfo::nsNodeInfoInner *node2 =
> >-    reinterpret_cast<const nsINodeInfo::nsNodeInfoInner *>(key2);
> >+  auto *node1 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key1);
> >+  auto *node2 = reinterpret_cast<const NodeInfo::NodeInfoInner*>(key2);
> 
> There is really no reason for auto usage here.

Sure there is, we avoid writing the (rather lengthy) type twice.

> >-  already_AddRefed<nsINodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
> >+  already_AddRefed<mozilla::dom::NodeInfo> GetNodeInfo(nsIAtom *aName, nsIAtom *aPrefix,
> >                                             int32_t aNamespaceID,
> >                                             uint16_t aNodeType,
> >                                             nsIAtom* aExtraName = nullptr);
> Please fix indentation

Done.

> >     nsPrototypeArray         mChildren;
> > 
> >-    nsCOMPtr<nsINodeInfo>    mNodeInfo;           // [OWNER]
> >+    nsRefPtr<mozilla::dom::NodeInfo>    mNodeInfo;           // [OWNER]
> No need for so many empty spaces between > and m

Fixed.

> >+NodeInfo::GetName(nsAString& aName) const
> >+{
> >+  mInner.mName->ToString(aName);
> >+}
> >+
> >+void
> >+NodeInfo::GetPrefix(nsAString& aPrefix) const
> >+{
> >+  if (mInner.mPrefix) {
> >+    mInner.mPrefix->ToString(aPrefix);
> >+  } else {
> >+    SetDOMStringToNull(aPrefix);
> >+  }
> >+}
> Why you un-inline these?
> Could you just move to NodeInfoInlines.h ?

They're used all over the place so it would require including it everywhere.

> >- * nsINodeInfo is an interface to node info, such as name, prefix, namespace
> >- * ID and possibly other data that is shared between nodes (elements
> >- * and attributes) that have the same name, prefix and namespace ID within
> >- * the same document.
> >+ * Class that represents a prefix/namespace/localName triple; a single
> >+ * nodeinfo is shared by all elements in a document that have that
> >+ * prefix, namespace, and localName.
> That isn't quite accurate description, since there is also qname etc.
> So perhaps s/ triple/\/etc. /
> 
> >  *
> >  * nsNodeInfoManager's are internal objects that manage a list of
> >- * nsINodeInfo's, every document object should hold a strong reference to
> >- * a nsNodeInfoManager and every nsINodeInfo also holds a strong reference
> >- * to their owning manager. When a nsINodeInfo is no longer used it will
> >+ * NodeInfo's, every document object should hold a strong reference to
> Shouldn't it be nsNodeInfoManagers are....
> manage a list of NodeInfos.
> 
> 
> 
> 
> >+ * a nsNodeInfoManager and every NodeInfo also holds a strong reference
> >+ * to their owning manager. When a NodeInfo is no longer used it will
> >  * automatically remove itself from its owner manager, and when all
> >- * nsINodeInfo's have been removed from a nsNodeInfoManager and all external
> >+ * NodeInfo's have been removed from a nsNodeInfoManager and all external
> Shouldn't this be NodeInfos

Yeah, fixed.

> >-  nsNodeInfoManager *NodeInfoManager() const
> >+  nsNodeInfoManager* NodeInfoManager() const
> Ooh, correct style here
> 
> > 
> >   // nsNodeInfoManager needs to pass mInner to the hash table.
> >-  friend class nsNodeInfoManager;
> >+  friend class ::nsNodeInfoManager;
> Hmm, :: is needed here, I guess
> 
> >  
> >+++ b/dom/xslt/xslt/txMozillaTextOutput.cpp
> >@@ -14,16 +14,17 @@
> > #include "nsCharsetSource.h"
> > #include "nsIPrincipal.h"
> > #include "txURIUtils.h"
> > #include "nsContentCreatorFunctions.h"
> > #include "nsContentUtils.h"
> > #include "nsGkAtoms.h"
> > #include "mozilla/dom/EncodingUtils.h"
> > #include "nsTextNode.h"
> >+#include "nsNamespaceManager.h"
> Is this really needed?

Yes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac426472ceec

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ac426472ceec