Closed Bug 349170 Opened 18 years ago Closed 18 years ago

nsCSSValue should use nsMemory allocators

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sayrer, Unassigned)

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 3 obsolete files)

It looks like nsCSSValue mixes allocators. Sometimes it uses nsCRT::strdup, and sometimes it uses ToNewUnicode.
Summary: nsCSSValue mixes allocators → nsCSSValue should use nsMemory allocators
Note that mString was already being allocated with ToNewUnicode in SetString().
Attachment #235018 - Flags: superreview?(bzbarsky)
Attachment #235018 - Flags: review?(bzbarsky)
Comment on attachment 235018 [details] [diff] [review]
Convert mString and mURL.mString to nsMemory

>-      mValue.mString = nsCRT::strdup(aCopy.mValue.mString);
>+      mValue.mString = ToNewUnicode(nsDependentString(aCopy.mValue.mString));

Shouldn't we have a simpler way of doing this?
(In reply to comment #2)
> 
> Shouldn't we have a simpler way of doing this?

Er, it looks we do on trunk, in nsCRTGlue.h. That file isn't on branch, though. Should I use nsCRTGlue on trunk and nsDependentString on branch?

Comment on attachment 235018 [details] [diff] [review]
Convert mString and mURL.mString to nsMemory

>Index: layout/style/nsCSSValue.cpp
>+      mValue.mString = ToNewUnicode(nsDependentString(aCopy.mValue.mString));

I'm not that happy with this...  We really shouldn't need the extra object here.  We don't hit this code _that_ much (order of hundreds to thousands of times per page), but still....

If there's a better way to do it, I'd like to see a patch.

I also wonder whether we should just store a nsStringBuffer* instead of a PRUnichar*...  That might let us eliminate a bunch of the ToNewUnicode() stuff in general, if we're getting passed in strings that have string buffers.  It'd certainly help in the copy-ctor and URL cases.
Attached patch Use nsCRTGlue on trunk (obsolete) — Splinter Review
Ok, the nsStringBuffer idea doesn't seem to be on the path to fixing this on the branch (API stability), so I want to do this patch first.

Is there a good test around to measure performance for this? Maybe a page with lots of font rules and background images?
Attachment #235018 - Attachment is obsolete: true
Attachment #235090 - Flags: superreview?(bzbarsky)
Attachment #235090 - Flags: review?(bzbarsky)
Attachment #235018 - Flags: superreview?(bzbarsky)
Attachment #235018 - Flags: review?(bzbarsky)
Attachment #235090 - Flags: superreview?(bzbarsky)
Attachment #235090 - Flags: review?(bzbarsky)
Attached patch Use nsCRTGlue on trunk (obsolete) — Splinter Review
Here's what I'd like to do first.
Attachment #235090 - Attachment is obsolete: true
Attachment #235094 - Flags: superreview?(bzbarsky)
Attachment #235094 - Flags: review?(bzbarsky)
Attached patch Use nsCRTGlue on trunk (obsolete) — Splinter Review
Missed an ifdef
Attachment #235094 - Attachment is obsolete: true
Attachment #235096 - Flags: superreview?(bzbarsky)
Attachment #235096 - Flags: review?(bzbarsky)
Attachment #235094 - Flags: superreview?(bzbarsky)
Attachment #235094 - Flags: review?(bzbarsky)
Comment on attachment 235096 [details] [diff] [review]
Use nsCRTGlue on trunk

I'd really prefer not adding these MOZILLA_1_8_BRANCH to this code.  This isn't code we expect to do much on branch with; just diverge it between branch and trunk.  Post separate patches.

I'm not aware of much in the way of tests around, but it's easy to write one; just a perl script that outputs lots of <span style="font-family: Times, serif">Text</span> or something?
Attached patch trunk patchSplinter Review
Trunk patch with no ifdefs. The branch can use attachment 235018 [details] [diff] [review].
Attachment #235096 - Attachment is obsolete: true
Attachment #235348 - Flags: superreview?(bzbarsky)
Attachment #235348 - Flags: review?(bzbarsky)
Attachment #235096 - Flags: superreview?(bzbarsky)
Attachment #235096 - Flags: review?(bzbarsky)
Attachment #235018 - Attachment is obsolete: false
Attachment #235018 - Flags: superreview?(bzbarsky)
Attachment #235018 - Flags: review?(bzbarsky)
Comment on attachment 235348 [details] [diff] [review]
trunk patch

Looks good.  Thanks!
Attachment #235348 - Flags: superreview?(bzbarsky)
Attachment #235348 - Flags: superreview+
Attachment #235348 - Flags: review?(bzbarsky)
Attachment #235348 - Flags: review+
Attachment #235018 - Flags: superreview?(bzbarsky)
Attachment #235018 - Flags: superreview+
Attachment #235018 - Flags: review?(bzbarsky)
Attachment #235018 - Flags: review+
Checking in nsCSSValue.h;
/cvsroot/mozilla/layout/style/nsCSSValue.h,v  <--  nsCSSValue.h
new revision: 1.41; previous revision: 1.40
done
Checking in nsCSSValue.cpp;
/cvsroot/mozilla/layout/style/nsCSSValue.cpp,v  <--  nsCSSValue.cpp
new revision: 1.37; previous revision: 1.36
done
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
We should make sure to file a followup bug on evaluating nsStringBuffer.
(In reply to comment #13)
> We should make sure to file a followup bug on evaluating nsStringBuffer.
> 

filed bug 350141.
Comment on attachment 235018 [details] [diff] [review]
Convert mString and mURL.mString to nsMemory

drivers: low risk.
Attachment #235018 - Flags: approval1.8.1?
Hey Robert - can we get a quick summary of the benefits to landing this on the 1.8 branch?
(In reply to comment #16)
> Hey Robert - can we get a quick summary of the benefits to landing this on the
> 1.8 branch?

Mismatched allocators may or may not lead to heap corruption. In Mozilla, my understanding is that allocating with something other than nsMemory is dangerous if you plan to send the result across XPConnect or thread boundaries, but it may not always bite.

Whiteboard: [schrep-181approval pending]
Comment on attachment 235018 [details] [diff] [review]
Convert mString and mURL.mString to nsMemory

a=schrep/beltnzer for drivers.
Attachment #235018 - Flags: approval1.8.1? → approval1.8.1+
Checking in layout/style/nsCSSValue.h;
/cvsroot/mozilla/layout/style/nsCSSValue.h,v  <--  nsCSSValue.h
new revision: 1.33.12.1; previous revision: 1.33
done
Checking in layout/style/nsCSSValue.cpp;
/cvsroot/mozilla/layout/style/nsCSSValue.cpp,v  <--  nsCSSValue.cpp
new revision: 1.32.4.1; previous revision: 1.32
done
Keywords: fixed1.8.1
Whiteboard: [schrep-181approval pending]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: