Last Comment Bug 209634 - [FIX]No-op attribute changes can lead to processing and mutation events
: [FIX]No-op attribute changes can lead to processing and mutation events
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Linux
: P1 normal (vote)
: mozilla1.5alpha
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
http://ln.hixie.ch/media/diagrams/spe...
: 207054 (view as bug list)
Depends on: 188803
Blocks:
  Show dependency treegraph
 
Reported: 2003-06-17 01:34 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2008-07-31 02:37 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple testcase (493 bytes, text/html)
2003-06-17 01:37 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
XML testcase (537 bytes, text/xml)
2003-06-17 01:55 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Proposed patch (for generic XML and HTML only) (3.45 KB, patch)
2003-06-17 01:58 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Proposed patch including XUL and SVG (17.39 KB, patch)
2003-06-19 17:49 PDT, Boris Zbarsky [:bz] (still a bit busy)
caillon: review+
jst: superreview+
Details | Diff | Splinter Review
Patch with comments addressed, merged to post-jstalyptic world (12.27 KB, patch)
2003-06-20 21:22 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Additional patch I landed (r=caillon, sr=jag) to improve Tp a bit (2.77 KB, patch)
2003-06-22 01:50 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2003-06-17 01:34:01 PDT
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 [:bz] (still a bit busy) 2003-06-17 01:37:50 PDT
Created attachment 125800 [details]
simple testcase
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2003-06-17 01:55:40 PDT
Created attachment 125801 [details]
XML testcase
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2003-06-17 01:58:59 PDT
Created attachment 125802 [details] [diff] [review]
Proposed patch (for generic XML and HTML only)
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2003-06-17 01:59:32 PDT
Comment on attachment 125802 [details] [diff] [review]
Proposed patch (for generic XML and HTML only)

Thoughts?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2003-06-19 17:49:41 PDT
Created attachment 126086 [details] [diff] [review]
Proposed patch including XUL and SVG
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2003-06-19 18:15:41 PDT
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 [:bz] (still a bit busy) 2003-06-19 18:21:14 PDT
> 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) 2003-06-20 20:56:17 PDT
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 [:bz] (still a bit busy) 2003-06-20 21:22:24 PDT
Created attachment 126169 [details] [diff] [review]
Patch with comments addressed, merged to post-jstalyptic world
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2003-06-22 01:50:14 PDT
Created attachment 126217 [details] [diff] [review]
Additional patch I landed (r=caillon, sr=jag) to improve Tp a bit
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2003-06-22 01:51:27 PDT
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 2003-06-22 01:57:47 PDT
Nice work.
Comment 13 Olivier Cahagne 2003-08-25 23:38:42 PDT
*** Bug 207054 has been marked as a duplicate of this bug. ***
Comment 14 Malte Rücker (mbr) 2003-10-23 13:04:52 PDT
The patch for nsXULElement.cpp caused the regression described in bug 212625
(verified by backing it out). Can anyone look at this, please?

Note You need to log in before you can comment on or make changes to this bug.