Closed
Bug 804742
Opened 12 years ago
Closed 11 years ago
EnsureStringLength doesn't work
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: Ms2ger, Assigned: sumedhhere)
References
Details
(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])
Attachments
(2 files, 1 obsolete file)
15.44 KB,
patch
|
ehsan.akhgari
:
review+
Ms2ger
:
feedback+
|
Details | Diff | Splinter Review |
16.39 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
It calls SetLength without the fallible_t argument, which means it'll crash if the malloc fails. We should probably replace the 20-something users with SetLength with or without the fallible_t argument, as is appropriate for the call site.
Reporter | ||
Comment 1•11 years ago
|
||
We call EnsureStringLength in the following places: http://mxr.mozilla.org/mozilla-central/ident?i=EnsureStringLength In all these cases, we currently have: if (!EnsureStringLength(string, length)) { // ... and we should replace those by: if (!string.SetLength(length, fallible_t())) { (or mozilla::fallible_t(), if required).
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++]
Assignee | ||
Comment 3•11 years ago
|
||
Hi. I am an undergraduate college student and am new here. I would like to work on this and hopefully someone could guide me with it.
Assignee | ||
Comment 5•11 years ago
|
||
Just one small doubt, how do i check for namespace collisions. I mean how to check when mozilla::fallible_t() is required?
Reporter | ||
Comment 6•11 years ago
|
||
In general, .cpp files should have a "using namespace mozilla;" at the top, if they aren't wrapped in "namespace mozilla { ... }"; in either case, you don't need the "mozilla::". For headers... Well, actually, this function isn't called in headers, so you should never need the "mozilla::". If the "using" statement is missing, just add it.
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #727393 -
Flags: superreview?(josh)
Attachment #727393 -
Flags: review?(Ms2ger)
Reporter | ||
Comment 8•11 years ago
|
||
Comment on attachment 727393 [details] [diff] [review] Replaced all occurrences of EnsureStringLength(string,length) with string.SetLength(length,fallible_t()) Review of attachment 727393 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, generally. One nit, though: there should always be a space after the comma, as in SetLength(length, fallible_t()).
Attachment #727393 -
Flags: review?(ehsan)
Attachment #727393 -
Flags: review?(Ms2ger)
Attachment #727393 -
Flags: feedback+
Assignee | ||
Comment 9•11 years ago
|
||
Replaced all EnsureStringLength(string, length) with string.SetLength(length, fallible_t()). Also added a space before fallible_t().
Attachment #727393 -
Attachment is obsolete: true
Attachment #727393 -
Flags: superreview?(josh)
Attachment #727393 -
Flags: review?(ehsan)
Attachment #727605 -
Flags: superreview?(josh)
Attachment #727605 -
Flags: review?(Ms2ger)
Comment 10•11 years ago
|
||
Comment on attachment 727605 [details] [diff] [review] Bug 804742 I don't need to look at this. Ms2ger's review is sufficient.
Attachment #727605 -
Flags: superreview?(josh)
Reporter | ||
Comment 11•11 years ago
|
||
Comment on attachment 727605 [details] [diff] [review] Bug 804742 Review of attachment 727605 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me; I'm not a reviewer, though. I bet ehsan can claim to own this all :)
Attachment #727605 -
Flags: review?(ehsan)
Attachment #727605 -
Flags: review?(Ms2ger)
Attachment #727605 -
Flags: feedback+
Updated•11 years ago
|
Attachment #727605 -
Flags: review?(ehsan) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 12•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ed5545baf056 - build fails on Windows.
Keywords: checkin-needed
Assignee | ||
Comment 13•11 years ago
|
||
Does not the result against the patch I submitted says the build was successful? Or did I misunderstand something?
The builds that are red (and underlined) failed to build. In particular: https://tbpl.mozilla.org/php/getParsedLog.php?id=21025450&tree=Try https://tbpl.mozilla.org/php/getParsedLog.php?id=21025479&tree=Try
Comment 15•11 years ago
|
||
Hi Sumedh, are you still working on this bug? Just checking! Thanks very much!
Flags: needinfo?(sumedhhere)
Assignee | ||
Comment 16•11 years ago
|
||
I would very much like to but I cant figure out what's causing the error. (In reply to Liz Henry :lizzard from comment #15) > Hi Sumedh, are you still working on this bug? Just checking! Thanks very > much!
Flags: needinfo?(sumedhhere)
Comment 17•11 years ago
|
||
Ms2ger, do you know what file needs to be included for nsNativeCharsetUtils.cpp to recognize fallible_t?
Flags: needinfo?(Ms2ger)
Reporter | ||
Comment 18•11 years ago
|
||
That would be mozilla/fallible.h, I suppose.
Flags: needinfo?(Ms2ger)
Comment 19•11 years ago
|
||
need using namespace mozilla on XP_WIN and XP_OS2
Attachment #746254 -
Flags: review?(ehsan)
Comment 20•11 years ago
|
||
Is it a follow-up bug to remove EnsureStringLength definition?
Comment 21•11 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #20) > Is it a follow-up bug to remove EnsureStringLength definition? No, we should remove this function if unnecessary. (I don't check this function is still used yet...)
Updated•11 years ago
|
Attachment #746254 -
Flags: review?(ehsan) → review+
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b5d65eaedb thanks, Sumedh!
Target Milestone: --- → mozilla23
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63b5d65eaedb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•