209634 - [FIX]No-op attribute changes can lead to processing and mutation events

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We have some code in nsGenericHTMLElement::SetAttr to prevent processing of
attribute changes that don't actually change the attribute.  This code is
bypassed if we call out to SetHTMLAttribute, however, and there is no equivalent
check in SetHTMLAttribute.  The URL field has an example page where our CPU
usage is ridiculously higher than it should be because of this problem.

In addition, nsGenericElement has no such checks at all; we probably want to add
some.  (same for nsSVGElement, and then there is XUL....)

This bug depends on bug 188803; until that's checked in, fixing this bug would
break inline style changes....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: simple testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: XML testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch (for generic XML and HTML only)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125802 [details] [diff] [review]
Proposed patch (for generic XML and HTML only)

Thoughts?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch including XUL and SVG

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 126086 [details] [diff] [review]
Proposed patch including XUL and SVG

- In nsXULElement::SetAttr():

     if (tag == nsXULAtoms::window &&
	 aNodeInfo->Equals(nsXULAtoms::hidechrome)) {
-      nsAutoString val;
-      val.Assign(aValue);
+      nsAutoString val(aValue);
       HideWindowChrome(val.EqualsIgnoreCase("true"));

How about using aValue.Equals("true", nsCaseInsensitiveStringComparator()) and
loosing the nsAutoString?

sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

> How about using aValue.Equals("true", nsCaseInsensitiveStringComparator()) and
> loosing the nsAutoString?

That may give the wrong result in some locales, because the comparator is
locale-sensitive....  See the problems we had with similar code when determining
input types in HTML (bug number provided on request).

Taking bug.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 126086 [details] [diff] [review]
Proposed patch including XUL and SVG

>Index: content/svg/content/src/nsSVGAttributes.cpp

>+  if (index < count) {  // found the attr in the list
>+    NS_ASSERTION(attr, "How did we get here with a null attr pointer?");
>+    modification = PR_TRUE;
>+    attr->GetValue()->SetValueString(aValue);
>+  } else  { // didn't find it
>     if (GetMappedAttribute(aNodeInfo, &attr)) {
>       AppendElement(attr);
>       attr->GetValue()->SetValueString(aValue);
>     }
>     else {
>       rv = nsSVGAttribute::Create(aNodeInfo, aValue, &attr);
>       NS_ENSURE_TRUE(attr, rv);
>       AppendElement(attr);
>     }
>     attr->Release();

Add the comment we talked about, plus only one AppendElement() call.


>Index: content/xul/content/src/nsXULElement.cpp

>+    // Check to see if the STYLE attribute is being set.  If so, we need to
>+    // create a new style rule based off the value of this attribute, and we
>+    // need to let the document know about the StyleRule change.
>     if (aNodeInfo->Equals(nsXULAtoms::style, kNameSpaceID_None) &&
>-        (mDocument != nsnull)) {
>+        mDocument) {

This can fit on one line in under 80 chars now, if you like.  I don't care one
way or another.

r=caillon.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with comments addressed, merged to post-jstalyptic world

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch I landed (r=caillon, sr=jag) to improve Tp a bit

Comment 11

[Boris Zbarsky [:bzbarsky]]

fixed.  This seems to not have affected Tp after all and improved Txul by 3%, Ts
by 1% or so.

Comment 12

[Adam D. Moss]

Nice work.

Comment 13

[Olivier Cahagne]

*** Bug 207054 has been marked as a duplicate of this bug. ***

Comment 14

[Malte Rücker (mbr)]

The patch for nsXULElement.cpp caused the regression described in bug 212625
(verified by backing it out). Can anyone look at this, please?