1069890 - Stop using global char* in nsExceptionHandler

Status: RESOLVED DUPLICATE of bug 1442053

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

Back when I first wrote this file I was copying some things that Breakpad did to avoid calling string::c_str for fear that it could allocate. Nowadays C++11 states that string::c_str is equivalent to string::data, and neither of them allocate. We could ditch all the places where we allocate char*/whcar_t* in advance and just use global string/wstring instead. This source file is pretty ugly in a number of other ways, but this would remove a little bit of ugliness.

Comment 1

[Connor [:cojo]]

Just to make sure I understand the scope of the bug, would an example of exchanging char* for string be modifying [1] by exchanging XP_CHAR for xpstring and then making the necessary downstream changes (Concat now becomes += (or some such derivation), the call to WriteFile now uses miniDumpPath.c_str(), etc.) or is that not within the scope of this bug (or not at all what this bug is referring to).

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#537

Comment 2

[(not currently active) Ted Mielczarek]

Good question, let me clarify! Inside the MinidumpCallback we can't use std::string for much because we have to be sure we're not allocating new memory, so we'll have to keep using XP_CHAR there. What we could change, however, are all the static variables, like pendingDirectory here:
http://hg.mozilla.org/mozilla-central/annotate/5bd6e09f074e/toolkit/crashreporter/nsExceptionHandler.cpp#l164

We currently jump through hoops to allocate a new char* every time we change these values:
http://hg.mozilla.org/mozilla-central/annotate/5bd6e09f074e/toolkit/crashreporter/nsExceptionHandler.cpp#l2328

but we could instead just use a std::string/std::wstring there and call .c_str() or .data() on it to safely get the data out, and then not have to deal with the memory management of it.

Does that clear it up?

Comment 3

[Connor [:cojo]]

Attachment: 1069890-stop-using-char-nsexceptionhandler.diff

Think so. I've attached what I think might be the correct fixes for exchanging char* for std::string for pendingDirectory (as a first step).

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8493978 [details] [diff] [review]
1069890-stop-using-char-nsexceptionhandler.diff

Sorry I missed this patch, I've been backlogged on bugmail for a long time. If you attach a patch and want someone to look at it you should set the review? or feedback? flags and put someone's bugmail address there. I'll tag myself for review on this, I'm still working down my bugmail backlog from last week's vacation but I should get to this in the next few days.

Thanks for the patch!

Comment 5

[Connor [:cojo]]

Ah, makes sense. Didn't realize there was a feedback? option (this is less of a patch and more of "does this seem like a good first step" so wasn't quite sure how to classify it). Thanks!

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8493978 [details] [diff] [review]
1069890-stop-using-char-nsexceptionhandler.diff

Review of attachment 8493978 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! This is indeed what I was looking for. We can get this landed if you'd like, and you can finish the rest in another patch, or you can finish the rest and upload an updated patch and we can land it all at once.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +2319,5 @@
>  
>  #ifdef XP_WIN
>      nsString path;
>      pendingDir->GetPath(path);
> +    pendingDirectory.assign(path.get());

You should just be able to say pendingDirectory = path.get(); here and below.

@@ +2345,5 @@
>      NS_WARNING("Can't set up pending directory during shutdown.");
>      return false;
>    }
>  #ifdef XP_WIN
> +  pending->InitWithPath(nsDependentString(pendingDirectory.data()));

I'm not 100% sure if this is the correct pattern to use here, let's check with bsmedberg. I *think* it's correct, but Mozilla strings have never been my strong suit.

Comment 7

[Mark Banner (:standard8)]

This was done in bug 1442053