MutatePrep fails if the system is out of memory. This is not handled by ReplacePrep and SetCapacity. Memory is overwritten because ReplacePrep is used in string manipulating routines like Copy and Assign.

Daniel, do you have time to work on this, by any chance?

I will try it.

MutatePrep is used in a) ReplacePrep and b) Setcapacity. In case of a) If MutatePrep fails, then also ReplacePrep should fail. The result of ReplacePrep should be handled before performing the string operation. => In oom situations are some string operations not performed without any information or warning to the caller. (new bug?) b) I'm not sure how to handle oom here. 1) Should SetCapacity return a result which is handled later? (Like it is done in the patch for SetLength) I have no overview how far this effects other (base or derived) classes. 2) Give only an assertion?

you can't assert that an allocation succeeds. assertions are for programming errors. As for handling OOM in general... having every function return an error code is maybe not the best idea... maybe something like a member variable indicating whether OOM occured and a function to to query it (this would allow doing various stuff with a string and only checking for OOM at the end). but that also kinda sucks... other ideas?

Is this patch ready for reviews? Would be nice to get this into the upcoming releases

*** Bug 281701 has been marked as a duplicate of this bug. ***

> Is this patch ready for reviews? The changes for SetCapacity does not match the base class. (But VC give no compile errors.) The other functions are ready. Only OOM checks are included, no further error handling as mentioned in comment 4 is done. If it's ok and you have a few (10) hours, then I could create a new reviewable patch without changing the SetCapacity return type and without further error handling. Otherwise someone else should take the bug.

The problem is SetLength, which sets the length of the buffer. 1) I think the best way to handle oom in SetCapacity is by setting a new member variable (as suggested by comment). The variable should reflect the result of MutatePrep and could be used outside SetCapacity and ReplacePrep (e.g. SetLength). or 2) Change SetCapacity as proposed in the attachment, which also includes a change of SetCapacity in nsTAString_CharT. I don't think this a good idea. Other recommendations?

Johnny, can you help with a super-review here?

Comment on attachment 173170 [details] [diff] [review] Proposal how to handle oom in MutatePrep I really don't think I'm the best person to review string changes. changing to r?darin sr?dbaron

Comment on attachment 173170 [details] [diff] [review] Proposal how to handle oom in MutatePrep >+ * returns PR_FALSE of reallocation fails s/of/if/

Comment on attachment 173170 [details] [diff] [review] Proposal how to handle oom in MutatePrep Please avoid changing the public string API. If you need to return a value from SetCapacity so that you can check it in SetLength, then I think we should move the body of SetCapacity into a private method, and call it from both places. While in the long run, I think it makes sense to add some sort of error reporting to the public string API, I think that would involve more than just adding a return code to SetCapacity. Anyways, SetCapacity need not succeed if you consider that it is only an advisory call. If anything SetLength should report errors, but again let's save API changes for later and attack the entire API at that point.

FYI, I'm working on an improved version of this patch.

This patch is similar to the original with the following changes: * In SetLength(), I use Capacity() to verify that SetCapacity() succeeded. I know this is less efficient than a return code, but I doubt the difference is significant. At some point this will go away when we have proper error handling in the API itself. * In MutatePrep, I got rid of the code that changes mData to point to the fixed buffer in case realloc fails because it seemed pointless given that when realloc fails it does not modify the original buffer. So, it is possible for MutatePrep to instead return without modifying any state. I'll file a follow-up bug on implementing proper error handling in the string API unless such a bug doesn't already exist.

Comment on attachment 174104 [details] [diff] [review] v2 patch fwiw r=dveditz

Comment on attachment 174104 [details] [diff] [review] v2 patch >-// XXXdarin what is wrong with STDC's tolower? >+// XXX(darin): what is wrong with STDC's tolower? Probably that it's allowed to be locale-sensitive, although I couldn't get it to do weird things with 'i's in tr_TR. sr=dbaron

Comment on attachment 174104 [details] [diff] [review] v2 patch a=asa for checkin everwhere.

> Probably that it's allowed to be locale-sensitive, although I couldn't get it > to do weird things with 'i's in tr_TR. OK, i changed the comment accordingly.

fixed-on-trunk for mozilla 1.8b1 looks like most of the patch conflicts with the 1.7 and aviary branches :-/

fixed1.7.6, fixed-aviary1.0.1

here's the patch that i landed on the 1.7 branch and 1.0.1 aviary branch.

can somebody with the proper bits make bug 281701 public? it is a dupe of this public bug.

darin: re: comment 14, did you ever get around to filing a bug on implementing proper error handling in the string API?

See bug 302063 (thanks for the reminder)