Last Comment Bug 804742 - EnsureStringLength doesn't work
: EnsureStringLength doesn't work
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Sumedh Shekhar
:
Mentors:
Depends on:
Blocks: 737164 837438
  Show dependency treegraph
 
Reported: 2012-10-23 13:37 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2013-05-07 19:34 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replaced all occurrences of EnsureStringLength(string,length) with string.SetLength(length,fallible_t()) (15.42 KB, patch)
2013-03-20 15:44 PDT, Sumedh Shekhar
Ms2ger: feedback+
Details | Diff | Splinter Review
Bug 804742 (15.44 KB, patch)
2013-03-21 03:51 PDT, Sumedh Shekhar
ehsan: review+
Ms2ger: feedback+
Details | Diff | Splinter Review
fix bustage (16.39 KB, patch)
2013-05-06 23:27 PDT, Makoto Kato [:m_kato]
ehsan: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-10-23 13:37:58 PDT
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.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2013-03-01 05:53:28 PST
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).
Comment 2 techrandy 2013-03-08 15:23:06 PST
I don't mind taking a stab at this if that is okay.
Comment 3 Sumedh Shekhar 2013-03-13 12:38:59 PDT
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 Josh Matthews [:jdm] 2013-03-13 13:09:27 PDT
Is anything in comment 1 unclear?
Comment 5 Sumedh Shekhar 2013-03-15 00:26:37 PDT
Just one small doubt, how do i check for namespace collisions. I mean how to check when mozilla::fallible_t() is required?
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2013-03-15 03:08:01 PDT
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 7 Sumedh Shekhar 2013-03-20 15:44:08 PDT
Created attachment 727393 [details] [diff] [review]
Replaced all occurrences of EnsureStringLength(string,length) with string.SetLength(length,fallible_t())
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2013-03-21 03:03:14 PDT
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()).
Comment 9 Sumedh Shekhar 2013-03-21 03:51:20 PDT
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().
Comment 10 Josh Matthews [:jdm] 2013-03-21 07:55:53 PDT
Comment on attachment 727605 [details] [diff] [review]
Bug 804742

I don't need to look at this. Ms2ger's review is sufficient.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2013-03-21 13:21:15 PDT
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 :)
Comment 12 Phil Ringnalda (:philor, back in August) 2013-03-23 20:45:49 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ed5545baf056 - build fails on Windows.
Comment 13 Sumedh Shekhar 2013-03-23 21:15:23 PDT
Does not the result against the patch I submitted says the build was successful? Or did I misunderstand something?
Comment 14 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2013-03-23 21:46:38 PDT
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 Liz Henry (:lizzard) (needinfo? me) 2013-04-24 16:48:50 PDT
Hi Sumedh, are you still working on this bug? Just checking! Thanks very much!
Comment 16 Sumedh Shekhar 2013-04-25 00:34:52 PDT
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!
Comment 17 Josh Matthews [:jdm] 2013-04-25 02:59:08 PDT
Ms2ger, do you know what file needs to be included for nsNativeCharsetUtils.cpp to recognize fallible_t?
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2013-04-25 03:00:16 PDT
That would be mozilla/fallible.h, I suppose.
Comment 19 Makoto Kato [:m_kato] 2013-05-06 23:27:50 PDT
Created attachment 746254 [details] [diff] [review]
fix bustage

need using namespace mozilla on XP_WIN and XP_OS2
Comment 20 Masatoshi Kimura [:emk] 2013-05-06 23:45:09 PDT
Is it a follow-up bug to remove EnsureStringLength definition?
Comment 21 Makoto Kato [:m_kato] 2013-05-06 23:50:43 PDT
(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...)
Comment 22 Makoto Kato [:m_kato] 2013-05-07 08:54:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/63b5d65eaedb

thanks, Sumedh!
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-05-07 19:34:21 PDT
https://hg.mozilla.org/mozilla-central/rev/63b5d65eaedb

Note You need to log in before you can comment on or make changes to this bug.