Closed
Bug 1201287
Opened 9 years ago
Closed 9 years ago
Cleanup nsSupportsPrimitives.cpp
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: mikokm, Assigned: mikokm)
Details
Attachments
(1 file, 3 obsolete files)
20.37 KB,
patch
|
Details | Diff | Splinter Review |
- There is a lot of code duplication in xpcom/ds/nsSupportsPrimitives.cpp. - The buffer lengths for nsMemory::clone() are checked using O(n) strlen even though snprintf can return the number of characters written.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=091dae04cb44
Updated•9 years ago
|
Attachment #8656251 -
Flags: review?(bugs)
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8656251 -
Attachment is obsolete: true
Attachment #8656251 -
Flags: review?(bugs)
Updated•9 years ago
|
Assignee: nobody → mikokm
Updated•9 years ago
|
Attachment #8656265 -
Flags: review?(bugs)
Comment 4•9 years ago
|
||
Comment on attachment 8656265 [details] [diff] [review] Cleanup nsSupportsPrimitives.cpp >+template<int N, typename T> >+static char* >+DataToString(const char* aFormat, T aData) >+{ >+ char buf[N]; >+ int len = snprintf_literal(buf, aFormat, aData); >+ >+ if(len < 0 || len > N) { >+ return NULL; s/NULL/nullptr/ >+ } >+ >+ return (char*)nsMemory::Clone(buf, (len + 1) * sizeof(char)); >+} Hmm, do we actually need to pass N explicitly? We could just use some large enough size for buf and then MOZ_ASSERT here that len is always smaller than that size. Since buf is stack only allocation, it doesn't affect much to performance whether say 8 or 128 is used. Also, looks like nsMemory::Clone uses NS_Alloc, which is infallible, so this all can be simplified some more. https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsMemory.cpp?rev=b276ce875275#31 http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPCOM.h?rev=90e184f46b30#208 using C++ casting here would be nice.
Attachment #8656265 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8656265 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bugs)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Olli Pettay [:smaug] > Comment on attachment 8656265 [details] [diff] [review] Thank you for the review and help. > Cleanup nsSupportsPrimitives.cpp > > s/NULL/nullptr/ Done. > Hmm, do we actually need to pass N explicitly? > We could just use some large enough size for buf and then MOZ_ASSERT here > that len is always smaller than that size. Since buf is stack only > allocation, it doesn't affect much to performance whether say 8 or 128 is > used. Changed N to fixed size 32. Added MOZ_ASSERT. > Also, looks like nsMemory::Clone uses NS_Alloc, which is infallible, so this > all can be simplified some more. > using C++ casting here would be nice. Removed all memory allocation checks from infallible allocations and replaced the C-style casts. Also fixed line breaks to be more consistent and removed few useless temporary variables.
Comment 7•9 years ago
|
||
Comment on attachment 8656798 [details] [diff] [review] Cleanup nsSupportsPrimitives.cpp z>+template<typename T> >+static char* >+DataToString(const char* aFormat, T aData) >+{ >+ static const int size = 32; >+ char buf[size]; >+ >+ int len = snprintf_literal(buf, aFormat, aData); >+ MOZ_ASSERT(len > 0 && len < size); >+ >+ return static_cast<char*>(nsMemory::Clone(buf, (len + 1) * sizeof(char))); Silly me, double for example take more than 32 chars as string. So, remove the assert and we just keep the existing behavior where some decimals are possibly cut and for cloning use std::min(len + 1, buf); since len may be longer than buf.
Flags: needinfo?(bugs)
Attachment #8656798 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8656798 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8657598 [details] [diff] [review] Cleanup nsSupportsPrimitives.cpp - Fixed some trailing white space and line lengths. - Buffer length is now min(length + 1, 32).
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db338d1f7e21
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/13104b0d5cfd
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/13104b0d5cfd
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•