All users were logged out of Bugzilla on October 13th, 2018

Stop using global char* in nsExceptionHandler

NEW
Unassigned

Status

()

4 years ago
4 years ago

People

(Reporter: ted, Unassigned, Mentored)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

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

4 years ago
Created attachment 8493978 [details] [diff] [review]
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 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!
Attachment #8493978 - Flags: review?(ted)

Comment 5

4 years ago
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 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.
Attachment #8493978 - Flags: review?(ted) → review+
You need to log in before you can comment on or make changes to this bug.