EnsureStringLength doesn't work

RESOLVED FIXED in mozilla23

Status

()

Core
String
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ms2ger, Assigned: Sumedh Shekhar)

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

4 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++]

Comment 2

4 years ago
I don't mind taking a stab at this if that is okay.
(Assignee)

Comment 3

4 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.

Comment 4

4 years ago
Is anything in comment 1 unclear?
Assignee: nobody → sumedhhere
(Assignee)

Comment 5

4 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

4 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

4 years ago
Created attachment 727393 [details] [diff] [review]
Replaced all occurrences of EnsureStringLength(string,length) with string.SetLength(length,fallible_t())
Attachment #727393 - Flags: superreview?(josh)
Attachment #727393 - Flags: review?(Ms2ger)
(Reporter)

Comment 8

4 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

4 years ago
Created attachment 727605 [details] [diff] [review]
Bug 804742

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)
(Reporter)

Comment 11

4 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+
Attachment #727605 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&rev=ed5545baf056 - build fails on Windows.
Keywords: checkin-needed
(Assignee)

Comment 13

4 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
Hi Sumedh, are you still working on this bug? Just checking! Thanks very much!
Flags: needinfo?(sumedhhere)
(Assignee)

Comment 16

4 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)
Ms2ger, do you know what file needs to be included for nsNativeCharsetUtils.cpp to recognize fallible_t?
Flags: needinfo?(Ms2ger)
(Reporter)

Comment 18

4 years ago
That would be mozilla/fallible.h, I suppose.
Flags: needinfo?(Ms2ger)

Updated

4 years ago
Blocks: 837438
Created attachment 746254 [details] [diff] [review]
fix bustage

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
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.