Last Comment Bug 754592 - Lazily allocate conditional processing data
: Lazily allocate conditional processing data
Status: REOPENED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on: 757718 CVE-2012-3970 761507 CVE-2012-4183
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-12 09:40 PDT by Robert Longson
Modified: 2012-09-05 10:47 PDT (History)
3 users (show)
longsonr: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (15.18 KB, patch)
2012-05-12 09:42 PDT, Robert Longson
jwatt: review+
Details | Diff | Splinter Review
address review comments (13.82 KB, patch)
2012-05-18 05:49 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description Robert Longson 2012-05-12 09:40:55 PDT
conditional processing attributes are rarely set and yet exist on most elements. We should allocate space for them lazily.
Comment 1 Robert Longson 2012-05-12 09:42:42 PDT
Created attachment 623439 [details] [diff] [review]
patch
Comment 2 Jonathan Watt [:jwatt] 2012-05-15 10:10:47 PDT
Robert and I just took a look at this, and SVGStringList takes 16 bytes on 64-bit Mac. So we get a 48 byte saving with his patch for almost every SVG element. There is a slight overhead of the hash lookup for GetProperty(), but that's probably negligible compared to the other overhead during painting.
Comment 3 Jonathan Watt [:jwatt] 2012-05-15 10:33:18 PDT
Comment on attachment 623439 [details] [diff] [review]
patch

Review of attachment 623439 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGTests.cpp
@@ +251,5 @@
>  {
>    for (PRUint32 i = 0; i < ArrayLength(sStringListNames); i++) {
>      if (aAttribute == *sStringListNames[i]) {
> +      SVGStringList *stringList = GetStringListAttribute(i);
> +      if (stringList) {

I think you need a comment here noting why you don't remove the property. I.e. the DOMSVGStringList DOM wrappers depend on there always being a property object as long as they exist.

@@ +260,5 @@
>      }
>    }
>  }
>  
> +// Callback function, for freeing PRUint64 values stored in property table

Append "when the element goes away".

@@ +276,5 @@
> +SVGStringList*
> +DOMSVGTests::GetStringListAttribute(PRUint8 aAttrEnum)
> +{
> +  nsIAtom *attrName = GetAttrName(aAttrEnum);
> +  nsCOMPtr<nsSVGElement> element = do_QueryInterface(this);

I think you should const_cast |this| so you can avoid removing the |const| from all the methods.

Also, shouldn't this be an nsRefPtr?

@@ +289,5 @@
> +  if (stringListPtr) {
> +    return stringListPtr;
> +  }
> +  nsIAtom *attrName = GetAttrName(aAttrEnum);
> +  nsCOMPtr<nsSVGElement> element = do_QueryInterface(this);

nsRefPtr
Comment 4 Robert Longson 2012-05-18 05:49:44 PDT
Created attachment 625075 [details] [diff] [review]
address review comments

I've added comments and reverted the const changes but I can't change things to nsRefPtr as it doesn't compile.
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-05-19 18:32:18 PDT
https://hg.mozilla.org/mozilla-central/rev/92f2cf2f42b2
Comment 7 Daniel Holbert [:dholbert] 2012-09-03 21:51:36 PDT
Backed out for the time being, for possibly causing bug 786895:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d10518d51
Comment 8 Robert Longson 2012-09-04 01:35:17 PDT
The backout changeset seems empty. I don't think it worked.
Comment 9 Ed Morley [:emorley] 2012-09-04 02:54:44 PDT
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Backed out for the time being, for possibly causing bug 786895:
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d10518d51

(In reply to Robert Longson from comment #8)
> The backout changeset seems empty. I don't think it worked.

(Coming here from marking the merge) 

It is unfortunately empty.

On a related note:
Both Mak's backout script and sfink's port of it to a mercurial extension protect against this + do all the hard work for you (including getting rid of the unnecessary merge changesets and adding both cset and bug # to the commit message, giving a more sheriff-friendly format).
https://wiki.mozilla.org/User:Mak77
https://bitbucket.org/sfink/qbackout
Comment 10 Daniel Holbert [:dholbert] 2012-09-04 08:39:32 PDT
(In reply to Ed Morley [:edmorley] from comment #9)
> (In reply to Robert Longson from comment #8)
> > The backout changeset seems empty. I don't think it worked.
> 
> (Coming here from marking the merge) 
> 
> It is unfortunately empty.

oops, thanks! Not sure what went wrong.

> On a related note:
> Both Mak's backout script and sfink's port of it to a mercurial extension
> protect against this + do all the hard work for you

Thanks -- good to know. Unfortunately, they can't do all the hard work in this case -- this isn't a straight "hg backout", because the code has changed quite a lot since this landed, so there's a good deal of manual merging required.  So, I imported (or rather, intended to import) a cleanly-applying backout patch that longsonr posted on bug 786895.

Anyway, I'll redo the backout later today (with more sanity checks :)).
Comment 11 Daniel Holbert [:dholbert] 2012-09-04 14:55:37 PDT
Backed out for reals (not empty this time):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/f867845a9956
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-09-04 18:46:29 PDT
https://hg.mozilla.org/mozilla-central/rev/f867845a9956
Comment 13 Daniel Holbert [:dholbert] 2012-09-05 10:47:24 PDT
Backed out on Aurora / Beta, too (with a=akeybl granted over on bug 786895):
 https://hg.mozilla.org/releases/mozilla-aurora/rev/e8503a9a261c
 https://hg.mozilla.org/releases/mozilla-beta/rev/b8cc5e2e8ed5

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