Closed
Bug 349170
Opened 18 years ago
Closed 18 years ago
nsCSSValue should use nsMemory allocators
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: sayrer, Unassigned)
Details
(Keywords: fixed1.8.1)
Attachments
(2 files, 3 obsolete files)
2.80 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
4.39 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
It looks like nsCSSValue mixes allocators. Sometimes it uses nsCRT::strdup, and sometimes it uses ToNewUnicode.
Reporter | ||
Updated•18 years ago
|
Summary: nsCSSValue mixes allocators → nsCSSValue should use nsMemory allocators
Reporter | ||
Comment 1•18 years ago
|
||
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?
Reporter | ||
Comment 3•18 years ago
|
||
(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?
Sure.
Comment 5•18 years ago
|
||
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.
Reporter | ||
Comment 6•18 years ago
|
||
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)
Reporter | ||
Updated•18 years ago
|
Attachment #235090 -
Flags: superreview?(bzbarsky)
Attachment #235090 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 7•18 years ago
|
||
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)
Reporter | ||
Comment 8•18 years ago
|
||
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 9•18 years ago
|
||
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?
Reporter | ||
Comment 10•18 years ago
|
||
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)
Reporter | ||
Updated•18 years ago
|
Attachment #235018 -
Attachment is obsolete: false
Attachment #235018 -
Flags: superreview?(bzbarsky)
Attachment #235018 -
Flags: review?(bzbarsky)
Comment 11•18 years ago
|
||
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+
Updated•18 years ago
|
Attachment #235018 -
Flags: superreview?(bzbarsky)
Attachment #235018 -
Flags: superreview+
Attachment #235018 -
Flags: review?(bzbarsky)
Attachment #235018 -
Flags: review+
Reporter | ||
Comment 12•18 years ago
|
||
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
Comment 13•18 years ago
|
||
We should make sure to file a followup bug on evaluating nsStringBuffer.
Reporter | ||
Comment 14•18 years ago
|
||
(In reply to comment #13) > We should make sure to file a followup bug on evaluating nsStringBuffer. > filed bug 350141.
Reporter | ||
Comment 15•18 years ago
|
||
Comment on attachment 235018 [details] [diff] [review] Convert mString and mURL.mString to nsMemory drivers: low risk.
Attachment #235018 -
Flags: approval1.8.1?
Comment 16•18 years ago
|
||
Hey Robert - can we get a quick summary of the benefits to landing this on the 1.8 branch?
Reporter | ||
Comment 17•18 years ago
|
||
(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.
Updated•18 years ago
|
Whiteboard: [schrep-181approval pending]
Comment 18•18 years ago
|
||
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+
Reporter | ||
Comment 19•18 years ago
|
||
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.
Description
•