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

RESOLVED FIXED in mozilla1.5alpha

Status

()

Core
DOM: Core & HTML
P1
normal
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.5alpha
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

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....
Created attachment 125800 [details]
simple testcase
Created attachment 125801 [details]
XML testcase
Created attachment 125802 [details] [diff] [review]
Proposed patch (for generic XML and HTML only)
Comment on attachment 125802 [details] [diff] [review]
Proposed patch (for generic XML and HTML only)

Thoughts?
Created attachment 126086 [details] [diff] [review]
Proposed patch including XUL and SVG
Attachment #125802 - Attachment is obsolete: true
Attachment #126086 - Flags: superreview?(jst)
Attachment #126086 - Flags: review?(bugmail)
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
Attachment #126086 - Flags: superreview?(jst) → superreview+
> 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.
Assignee: dom_bugs → bzbarsky
Priority: -- → P1
Summary: No-op attribute changes can lead to processing and mutation events → [FIX]No-op attribute changes can lead to processing and mutation events
Target Milestone: --- → mozilla1.5alpha
Attachment #126086 - Flags: review?(bugmail) → review?(caillon)
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.
Attachment #126086 - Flags: review?(caillon) → review+
Created attachment 126169 [details] [diff] [review]
Patch with comments addressed, merged to post-jstalyptic world
Attachment #126086 - Attachment is obsolete: true
Created attachment 126217 [details] [diff] [review]
Additional patch I landed (r=caillon, sr=jag) to improve Tp a bit
fixed.  This seems to not have affected Tp after all and improved Txul by 3%, Ts
by 1% or so.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 12

14 years ago
Nice work.

Comment 13

14 years ago
*** Bug 207054 has been marked as a duplicate of this bug. ***
The patch for nsXULElement.cpp caused the regression described in bug 212625
(verified by backing it out). Can anyone look at this, please?

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.