Closed Bug 284950 Opened 19 years ago Closed 19 years ago

DeCOMtaminate nsIContent::GetAttrNameAt

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Patch to fix
74.06 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Fix review comments
79.92 KB, patch
Details | Diff | Splinter Review 
we should make GetAttrNameAt faster by not forcing it to addref the localname
and prefix atoms on the way out. Bug 284708 is one example of wasted cycles due
to excessive atom refcounting.

One simple solution would be to simply make the signature:

const nsAttrName* GetAttrNameAt(PRUint32 aIndex)

returning nsnull for out-of-bounds indexes (In the same spirit as GetChildAt).
However this is impossible to do in XTF due to the nsIXTFAttributeHandler
interface which is defined in idl. See

http://lxr.mozilla.org/mozilla/source/content/xtf/src/nsXTFElementWrapper.cpp#312
http://lxr.mozilla.org/mozilla/source/content/xtf/public/nsIXTFAttributeHandler.idl

This makes it impossible to return an nsAttrName*, or even a weak nsIAtom*. The
interface isn't currently used inside mozilla it looks like, but I suspect that
it's used elsewhere? Alex, is there any chance that we could turn that interface
from .idl into .h and make getAttributeNameAt signature something like
const nsAttrName* GetAttributeNameAt(PRUInt32 aIndex)? Or are you implementing
this interface in js?

In general I'm a little worried about not keeping attribute storing code inside
gkcontent. It has allowed us to do neat optimizations like making GetID return a
weak atom, and GetClasses to return the classlist without shuffeling memory.
Though I guess that as long as that interface is only handling 'unknown'
attributes that shouldn't be a problem.
(In reply to comment #0)
 Alex, is there any chance that we could turn that interface
> from .idl into .h and make getAttributeNameAt signature something like
> const nsAttrName* GetAttributeNameAt(PRUInt32 aIndex)? Or are you implementing
> this interface in js?

I am actually implementing this interface in js. It is handy to map attributes
into an xtf element's visual content (anonymous content). E.g. think a fancy
kind-of editwidget <myEditWidget value="...">, containing a <html:input>, where
you want to map myEditWidget's value onto the html:input's value and vica versa.
I was afraid of that...

How about if that mapping is just done one way. So that the attribute is stored
in the xtfwrapper, but we still call various hooks so that the xtfelement can
still keep the visual element or whatnot in sync.

We could call just the Will(Set|Remove)Attribute hooks and let the attrhandler
react to those. Or we could keep the mAttributeHandler->SetAttribute call but
also always call nsXTFElementWrapperBase::SetAttr.

I think this is more or less what xbl does.

It'd be a little more wastefull since the attributevalue would be stored in two
places. But I don't think it'd make a big difference. And it would make it
easier on the xtfelement mapping attributes to another element, since you
wouldn't have to keep track of which attributes on the element was mapped in and
which were set for some other reason. I.e. you wouldn't have to implement
getAttribute, hasAttribute, getAttributeCount and getAttributeNameAt.

Come to think of it. If we did that we strictly speaking wouldn't need the
nsIXTFAttributeHandler interface at all. The attribute notifications on
nsIXTFElement would be enough. Or does the nsIXTFAttributeHandler interface have
advantages?
(In reply to comment #2)
> I was afraid of that...
> 
> How about if that mapping is just done one way. So that the attribute is stored
> in the xtfwrapper, but we still call various hooks so that the xtfelement can
> still keep the visual element or whatnot in sync.

The problem is not mapping the outer attribute into the visual content, but
mapping from the visual content back to the outer attribute. Staying with the
myEditWidget example, consider the user editing a large piece of text with the
widget. Apart from the inefficiency of requiring double the storage for the
text, how would you hook into <html:input> to synchronise myEditWidget::value to
html:input's value? 
 

The html:input's value attribute doesn't change when the user edits the
textfield anyway. But if it did you could attach a mutation-event-listener, or
an onchange eventlistener. But I do agree it is inefficient...

Though, I wonder if the current code will ever work properly anyway. Right now
you don't send off nsIDocumentObserver notifications or mutationevents when an
'inner' attribute is changed. How can that be fixed with the current solution?
(In reply to comment #4)
> The html:input's value attribute doesn't change when the user edits the
> textfield anyway. But if it did you could attach a mutation-event-listener, or
> an onchange eventlistener. But I do agree it is inefficient...
> 

Yeah, I have quite a few interactive widgets where serializing the internal
state onto their attributes would be too expensive to perform everytime the
state changes.

> Though, I wonder if the current code will ever work properly anyway. Right now
> you don't send off nsIDocumentObserver notifications or mutationevents when an
> 'inner' attribute is changed. How can that be fixed with the current solution?

I'm not sure which nsIDocumentObserver notifications you mean. As for mutation
events, I haven't looked into it in detail (never had a need for mutation
events), but I could envisage a forwarding scheme by intercepting
addEventListener etc.

(In reply to comment #5)
> Yeah, I have quite a few interactive widgets where serializing the internal
> state onto their attributes would be too expensive to perform everytime the
> state changes.

Does such state have to be mapped into attributes? Couldn't you do what
html:input.value (and animated svg attributes) do and make the attribute just be
the initializer and then have the active value accessible through a DOM property?

> I'm not sure which nsIDocumentObserver notifications you mean. As for mutation
> events, I haven't looked into it in detail (never had a need for mutation
> events), but I could envisage a forwarding scheme by intercepting
> addEventListener etc.

The notifications are BeginUpdate/AttributeChanged/EndUpdate. These
notifications drive various things such as updating layout and contentlists.

As with mutationevents there's probably nothing breaking in your apps because of
this, but various features don't work with xtfelements unless this is fixed.
(In reply to comment #6)
> (In reply to comment #5)
> Does such state have to be mapped into attributes? Couldn't you do what
> html:input.value (and animated svg attributes) do and make the attribute just be
> the initializer and then have the active value accessible through a DOM property?

No, the whole point is to get it to 'automatically serialize'.
  
> The notifications are BeginUpdate/AttributeChanged/EndUpdate. These
> notifications drive various things such as updating layout and contentlists.
> 
> As with mutationevents there's probably nothing breaking in your apps because of
> this, but various features don't work with xtfelements unless this is fixed.

Aren't these notifications irrelevant when attributes are mapped - don't they
get called by whatever element the attribute is mapped to?

How much of a performance problem is the current GetAttrNameAt implementation?
Would you really make noticeable performance improvements by changing the api in
the way you are suggesting? If yes, I don't want to stand in the way of progress
here. I'm sure I can redesign nsIXTFAttributeHandler to work around any changes
you make.
One simple solution would be to hold a pointer to the last-asked-for attr name
in nsXTFElementWrapper.  As long as people use the returned nsAttrValue
"immediately" (without other calls to GetAttrNameAt()), this would work.

> Aren't these notifications irrelevant when attributes are mapped - don't they
> get called by whatever element the attribute is mapped to?

They do, but they get called on the wrong content node, no?  This would affect
frame construction, content lists, etc, etc.

> How much of a performance problem is the current GetAttrNameAt implementation?

See bug 284708 (cited in comment 0).  In that bug, it's about 25-30% of the
total time, and by far the biggest single contributor.
(In reply to comment #8)
> One simple solution would be to hold a pointer to the last-asked-for attr name
> in nsXTFElementWrapper.  As long as people use the returned nsAttrValue
> "immediately" (without other calls to GetAttrNameAt()), this would work.

Yeah, sounds like a good idea if callers can live with it.

> 
> > Aren't these notifications irrelevant when attributes are mapped - don't they
> > get called by whatever element the attribute is mapped to?
> 
> They do, but they get called on the wrong content node, no?  This would affect
> frame construction, content lists, etc, etc.

Hmm, still not convinced on this one. No attribute set on the wrapper element
should have any bearing on layout stuff unless it (the attribute) is mapped into
the wrapper's visual content.

> > How much of a performance problem is the current GetAttrNameAt implementation?
> 
> See bug 284708 (cited in comment 0).  In that bug, it's about 25-30% of the
> total time, and by far the biggest single contributor.

Wow, sounds bad.
> No attribute set on the wrapper element should have any bearing on layout stuff

So it shouldn't affect style resolution?  It shouldn't affect membership in
getElementsByAttribute lists?
The last-asked-for solution sounds like it should work fine. I can't think of
any caller that wouldn't be happy with that. And in any case these pointers were
never very longlived anyway, a call to (Un)SetAttr will make them invalid.

Any attribute can be mapped into style using the appropriate css-selector. A
css-rule like:

myXTFElement[foo=hello] { color: green }

will not work if 'foo' is handeled by the the attributehandler. But if we want
to support rapidly changing values mapped into attributes then the current
solutions might be the best we can do. And xtf-users can always choose not to
use nsIXTFAttributeHandler for the attributes that it needs to support this
stuff for.
A solution to both GetAttrNameAt deCOM and to nsIDocumentObserver notifications
might be to do something similar to what we do in svg. There we send a
notification back to the element that the attribute has changed and then the
element sends out appropriate nsIDocumentObserver notifications.

What we could do is to add |attributeAdded|, |attributeRemoved| and
|attributeChanged| to nsIXTFElementWrapper. The attributehandler would then be
responsible for calling these methods when the attribute is changed. We'd then
send out the nsIDocumentObserver notifications as needed as well as keep a list
of nsAttrNames in the wrapper.

Or we could just go with the last-asked-for solution and advice against using
nsIXTFAttributeHandler. Though it feels bad to keep an interface that we advice
against using...
(In reply to comment #12)
> What we could do is to add |attributeAdded|, |attributeRemoved| and
> |attributeChanged| to nsIXTFElementWrapper. The attributehandler would then be
> responsible for calling these methods when the attribute is changed. We'd then
> send out the nsIDocumentObserver notifications as needed as well as keep a list
> of nsAttrNames in the wrapper.
Yeah, sounds like a good idea. (Finally, after bz's patient explanations on irc,
I understand what the problem is here :-)
Attached patch Patch to fix — — Splinter Review 
This implements the suggestion in comment 0 and uses the suolution in comment 8 for XTF.
Attachment #206048 - Flags: superreview?(bzbarsky)
Attachment #206048 - Flags: review?
Comment on attachment 206048 [details] [diff] [review]
Patch to fix

Won't get to a full review till early January, probably, but a few questions:

>Index: content/base/public/nsIContent.h
>+   * @returns The name at the given index, or null if the index is
>+   *          out-of-bounds.
>+   *          *Note* that the owner document (if one is present) is *not*
>+   *          neccesarily the owner document of the element.

What owner document?

Also, document that this returns null if the index is out of range?

Also, in the XTF changes you lost a subtraction of innerCount when calling nsXTFElementWrapperBase::GetAttrNameAt.

Finally, please document the restrictions on GetAttrNameAt callers on nsIContent so that what XTF does is ok (that is, the pointer is only valid up until the next GetAttrNameAt call).
> >Index: content/base/public/nsIContent.h
> >+   * @returns The name at the given index, or null if the index is
> >+   *          out-of-bounds.
> >+   *          *Note* that the owner document (if one is present) is *not*
> >+   *          neccesarily the owner document of the element.
> 
> What owner document?

The document returned by nodeInfo->GetDocument(). I'll describe that better.

> Also, document that this returns null if the index is out of range?

That's already in there.

> Also, in the XTF changes you lost a subtraction of innerCount when calling
> nsXTFElementWrapperBase::GetAttrNameAt.

Good catch!

> Finally, please document the restrictions on GetAttrNameAt callers on
> nsIContent so that what XTF does is ok (that is, the pointer is only valid up
> until the next GetAttrNameAt call).

Will do.
Comment on attachment 206048 [details] [diff] [review]
Patch to fix

>Index: content/base/src/nsGenericElement.cpp
>@@ -477,26 +477,24 @@ nsNode3Tearoff::LookupPrefix(const nsASt

You probably want to remove the existing declarations of |name|, |prefix|, and |namespace_id| in this function (since they're very much unused after your changes).  For that matter, the |nsAutoString ns;| also seems to be unused, no?

>Index: content/base/src/nsXMLContentSerializer.cpp
>@@ -579,27 +580,26 @@ nsXMLContentSerializer::AppendElementSta
>-  nsCOMPtr<nsIAtom> attrName, attrPrefix;
>+  nsIAtom *attrName, *attrPrefix;

Why declare these all the way up here?  It'd be better to declare them at the points where we use them (in the two loops below).  If we use them outside the loops (which is what declaring them out here allows), then we should make these strong refs just in case, imo.  But we really shouldn't be using these outside the loops, so I think we should move the declaration point into the loops.

Same for the namespaceID int here.  Declare where it's used.

>Index: content/svg/content/src/nsSVGUseElement.cpp
>+    PRUint32 i, count = newcontent->GetAttrCount();
>+    for (i = 0; i < count; i++) {
>       nsAutoString value;
>+      const nsAttrName* name = newcontent->GetAttrNameAt(i);
>+      if (!name) {
>+        break;

How would that happen?  If it can, other places that do such loops should worry about null as well.  But as best I can tell this can't happen.

>Index: content/xbl/src/nsXBLBinding.cpp
>@@ -612,23 +613,20 @@ nsXBLBinding::GenerateAnonymousContent()
>+    nsCOMPtr<nsIAtom> name = attrName->LocalName();

Why does that need to be an nsCOMPtr?  I don't see a reason...

>Index: content/xslt/src/xpath/txMozillaXPathTreeWalker.cpp
>@@ -377,21 +373,18 @@ txXPathNodeUtils::getLocalName(const txX
>+    nsIAtom* localName = aNode.mContent->GetAttrNameAt(aNode.mIndex)->LocalName();
>+    NS_ADDREF(localName);

It seems to me that this could stop addrefing (and change the function signature accordingly).  Followup bug if you agree and think that would be the right thing to do.

>Index: content/xul/content/src/nsXULElement.cpp
>@@ -1618,37 +1604,33 @@ nsXULElement::List(FILE* out, PRInt32 aI
>         nsAutoString s;
>+        if (name->GetPrefix()) {
>+            name->GetPrefix()->ToString(s);

How about replacing all that and some code following with name->GetQualifiedName(s) instead of manually messing with the localname and prefix?

>Index: content/xul/document/src/nsXULDocument.cpp
>@@ -3793,27 +3791,27 @@ nsXULDocument::OverlayForwardReference::
>+        const nsAttrName* name = aOverlayNode->GetAttrNameAt(i);
>+        NS_ENSURE_TRUE(name, NS_ERROR_UNEXPECTED);

Why do we need the null-check here?

>Index: content/xul/templates/src/nsXULContentBuilder.cpp
>@@ -895,22 +892,23 @@ nsXULContentBuilder::SynchronizeUsingTem
>+        const nsAttrName* name = aTemplateNode->GetAttrNameAt(loop);
>+        if (!name) {

And here?

>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
>@@ -2082,47 +2082,42 @@ nsXULTemplateBuilder::CompileSimpleRule(
>+        const nsAttrName* name = aRuleElement->GetAttrNameAt(i);
>+        NS_ENSURE_TRUE(name, NS_ERROR_UNEXPECTED);

And here?

>Index: layout/generic/nsObjectFrame.cpp
>@@ -2899,21 +2900,17 @@ nsresult nsPluginInstanceOwner::EnsureCa
>     PRInt32 nameSpaceID;
>+    nsIAtom* atom = content->GetAttrNameAt(index)->LocalName();
>     nsAutoString value;
>     content->GetAttr(nameSpaceID, atom, value);

nameSpaceID should be initialized and all, no?

>Index: editor/libeditor/html/nsHTMLEditorStyle.cpp
>@@ -777,28 +778,21 @@ nsresult nsHTMLEditor::RemoveStyleInside
>+    content->GetAttrNameAt(i)->LocalName()->ToString(attrString);

This probably needs some sort of check for the null namespace (possibly in a followup bug).

Fix all that up, and r+sr=bzbarsky
Attachment #206048 - Flags: superreview?(bzbarsky)
Attachment #206048 - Flags: superreview+
Attachment #206048 - Flags: review?
Attachment #206048 - Flags: review+
> >Index: content/svg/content/src/nsSVGUseElement.cpp
> >+    PRUint32 i, count = newcontent->GetAttrCount();
> >+    for (i = 0; i < count; i++) {
> >       nsAutoString value;
> >+      const nsAttrName* name = newcontent->GetAttrNameAt(i);
> >+      if (!name) {
> >+        break;
> 
> How would that happen?  If it can, other places that do such loops should worry
> about null as well.  But as best I can tell this can't happen.

I kept current checks everywhere where i couldn't absolutly tell that attributes wouldn't be manipulated during the loop. In the case of this function the SetAttr call could trigger a mutation event handler that removes attributes from newcontent.

I realized that a safer (and slighly faster) way to loop is to instead of doing:

count = c->GetAttrCount()
for (i = 0; i < count; i++) {
  name = c->GetAttrNameAt(i);
  ...
}

do

for (i = 0; (name = c->GetAttrNameAt(i)); i++) {
  ...
}

This made me find some bogus out-of-bounds warnings in nsAttrAndChildArray::GetSafeAttrNameAt so I removed those. (We should add a faster bounds-unsafe version, but that should be a new function).

> >Index: content/xslt/src/xpath/txMozillaXPathTreeWalker.cpp
> >@@ -377,21 +373,18 @@ txXPathNodeUtils::getLocalName(const txX
> >+    nsIAtom* localName = aNode.mContent->GetAttrNameAt(aNode.mIndex)->LocalName();
> >+    NS_ADDREF(localName);
> 
> It seems to me that this could stop addrefing (and change the function
> signature accordingly).  Followup bug if you agree and think that would be the
> right thing to do.

Unfortunatly no. PIs hold their names as strings so we need to return an addrefed atom. Eventually this function should be replaced by
|PRBool LocalNameIs(nsIAtom*)|.

> >Index: content/xul/document/src/nsXULDocument.cpp
> >@@ -3793,27 +3791,27 @@ nsXULDocument::OverlayForwardReference::
> >+        const nsAttrName* name = aOverlayNode->GetAttrNameAt(i);
> >+        NS_ENSURE_TRUE(name, NS_ERROR_UNEXPECTED);
> 
> Why do we need the null-check here?

Same reason as above. Changed this to use the safer loop construct instead. 

> >Index: layout/generic/nsObjectFrame.cpp
> >@@ -2899,21 +2900,17 @@ nsresult nsPluginInstanceOwner::EnsureCa
> >     PRInt32 nameSpaceID;
> >+    nsIAtom* atom = content->GetAttrNameAt(index)->LocalName();
> >     nsAutoString value;
> >     content->GetAttr(nameSpaceID, atom, value);
> 
> nameSpaceID should be initialized and all, no?

Ouch!

> >Index: editor/libeditor/html/nsHTMLEditorStyle.cpp
> >@@ -777,28 +778,21 @@ nsresult nsHTMLEditor::RemoveStyleInside
> >+    content->GetAttrNameAt(i)->LocalName()->ToString(attrString);
> 
> This probably needs some sort of check for the null namespace (possibly in a
> followup bug).

Fixed. This made me finally cave in and add nsAttrName::NamespaceEquals. It's stlightly faster then just comparing with returnvalue of NamespaceID() and somewhat easier to read. Made a couple of more callsites use it.
I'll land this tomorrow morning.
Looks like this needs one more build bustage fix. nsDocumentFragment.cpp has this signature:

virtual nsAttrName* GetAttrNameAt(PRUint32 aIndex) const

whereas nsGenericElement.h has this signature:

virtual const nsAttrName* GetAttrNameAt(PRUint32 aIndex) const;

Notice the const difference on nsAttrName*
It was a rocky landing, but now it's in.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Depends on: 321810
Sorry to spam, but I am seeing a lot of crashes on MacOS-X, while trying to use manage bookmarks tool.

Here is some infos :

"Thread 0 Crashed:
0   org.mozilla.firefox      	0x0094f9e0 nsAttrName::LocalName() const + 20
1   org.mozilla.firefox      	0x0021ca5c nsXULElement::GetAttrNameAt(unsigned) const + 152
2   org.mozilla.firefox      	0x004f4824 nsXULTemplateBuilder::CompileSimpleRule(nsIContent*, int, InnerNode*) + 984
3   org.mozilla.firefox      	0x004f5474 nsXULTemplateBuilder::CompileRules() + 564
4   org.mozilla.firefox      	0x001bcbd0 nsXULTreeBuilder::RebuildAll() + 172
5   org.mozilla.firefox      	0x004f1f48 nsXULTemplateBuilder::Rebuild() + 132
6   org.mozilla.firefox      	0x001bd530 nsXULTreeBuilder::SetTree(nsITreeBoxObject*) + 484"

Or, the last time nsXULTemplateBuilder.cpp is related to this checkin...

I will open a new bug soon :(
Sounds like bug 321810 that has already been fixed
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: