Closed Bug 804742 Opened 12 years ago Closed 11 years ago

EnsureStringLength doesn't work

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

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)

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.
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++]
I don't mind taking a stab at this if that is okay.
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.
Is anything in comment 1 unclear?
Assignee: nobody → sumedhhere
Just one small doubt, how do i check for namespace collisions. I mean how to check when mozilla::fallible_t() is required?
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.
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+
Attached patch Bug 804742Splinter Review
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 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)
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+
Attachment #727605 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
Does not the result against the patch I submitted says the build was successful? Or did I misunderstand something?
Hi Sumedh, are you still working on this bug? Just checking! Thanks very much!
Flags: needinfo?(sumedhhere)
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)
Ms2ger, do you know what file needs to be included for nsNativeCharsetUtils.cpp to recognize fallible_t?
Flags: needinfo?(Ms2ger)
That would be mozilla/fallible.h, I suppose.
Flags: needinfo?(Ms2ger)
Blocks: 837438
Attached patch fix bustageSplinter Review
need using namespace mozilla on XP_WIN and XP_OS2
Attachment #746254 - Flags: review?(ehsan)
Is it a follow-up bug to remove EnsureStringLength definition?
(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...)
Attachment #746254 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b5d65eaedb

thanks, Sumedh!
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/63b5d65eaedb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.