1442053 - Use regular C++ strings in the exception handler instead of C ones

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement, P3)

Description

[Gabriele Svelto [:gsvelto]]

Starting from C++11 the c_str() method of the string class is guaranteed not to allocate memory; we can thus get rid of all the C strings we keep around for access in the exception handler and use std::string / std::wstring directly instead.

Comment 1

[S P Kausthubh]

Hi, I'd like to work on this; could you point the exact set of files that require this modification to me?

Comment 2

[Gabriele Svelto [:gsvelto]]

This is the exception handler code:

https://searchfox.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp

Note that this is not necessarily a simple change due to the overall complexity of this code. Have a look and let me know if you want to work on it.

Comment 3

[S P Kausthubh]

Thank you, I'll take a look at the code and let you know if I'll be going forward with this.

Comment 4

[S P Kausthubh]

Sorry for the long break, I was busy with something else. I've taken a look at the code, and it seems to me that the c_str() function is being used only for calls by reference (4 instances if I am correct). Would the best thing to do, then, be to replace calling this function and passing the address via other means, preferably by simple dereferencing (I'm assuming it is from what has been said in the description)?

Comment 5

[S P Kausthubh]

Also, the variable 'path' that invokes the c_str() function is already defined as variable of the xpstring type, which is in turn an object of the 'wstring' class.

Comment 6

[Gabriele Svelto [:gsvelto]]

The idea is to change all the global strings that are either declared as XP_CHAR* or char*. There's quite a few of them. In the exception handler code you will have to only use .c_str() to access them since that's safe even in a crashed context.

Comment 7

[S P Kausthubh]

So what we aim to do is remove all the 'intermediary' c style strings that are declared globally and just pass pointers using 'c_str()'? Basically, we're getting rid of the c strings we don't need anymore, right?

Comment 8

[Gabriele Svelto [:gsvelto]]

Yes though we still need those strings. It's just that it's a lot easier to use C++ string objects than raw char*. This is all the global variables that would need to be changed: pendingDirectory, crashReporterPath, memoryReportPath, libraryPath, eventsDirectory, currentSessionId and lastCrashTimeFilename.

Comment 9

[NicolasPacheco]

Hey, I would like to work in this bug. I've been reading the thread and the exception handler code. I've some doubts, there are more global variables using char* that the ones that you mentioned, should I change them? for example: childCrashNotifyPipe, androidUserSerial.
And in the code when those string are used I should .c_str() to access them?

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to NicolasPacheco from comment #9)

Hey, I would like to work in this bug. I've been reading the thread and the exception handler code. I've some doubts, there are more global variables using char* that the ones that you mentioned, should I change them? for example: childCrashNotifyPipe, androidUserSerial.

Yes, some variables were added in the meantime.

And in the code when those string are used I should .c_str() to access them?

It depends. In some cases you can use them with .c_str() in others it might be worth using the C++ string. That's something you should establish on a case-by-case basis as you modify the code. Do you want to be assigned to this bug and give it a try?

Comment 11

[NicolasPacheco]

Yes I (In reply to Gabriele Svelto [:gsvelto] from comment #10)

(In reply to NicolasPacheco from comment #9)

Hey, I would like to work in this bug. I've been reading the thread and the exception handler code. I've some doubts, there are more global variables using char* that the ones that you mentioned, should I change them? for example: childCrashNotifyPipe, androidUserSerial.

Yes, some variables were added in the meantime.

And in the code when those string are used I should .c_str() to access them?

It depends. In some cases you can use them with .c_str() in others it might be worth using the C++ string. That's something you should establish on a case-by-case basis as you modify the code. Do you want to be assigned to this bug and give it a try?

Yes I would like to be assigned and give it a try, can you assign me or I should make a patch?

So, all in all the idea is to change all the C strings for C++ ones?

Comment 12

[Gabriele Svelto [:gsvelto]]

Here you go! Follow the Phabricator guide to submit a patch and flag me for review. Feel free to needinfo? me or ask on chat.mozilla.org on the #breakpad channel if you need help. Happy hacking!

Comment 13

[NicolasPacheco]

Hey Gabriele, when I change from the C declared strings to the C++ ones using std:string, I also have to delete the part where the code free the memory because with the std::string the destructor is called automatically the it goes out of the scope, am I right?

Comment 14

[Gabriele Svelto [:gsvelto]]

Yes, of course. That's part of the reason why using C++ strings would be better here.

Comment 15

[NicolasPacheco]

Attachment: Bug 1442053 - Change the following global variables from the C string to the C++ ones:pendingDirectory,crashReporterPath,memoryReportPath,libraryPath and eventsDirectory. r?gsvelto

Comment 16

[Gabriele Svelto [:gsvelto]]

Resetting assignment, this is a good first bug to start contributing.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 18

[achmurali]

Hi gsvelto,

Can i work on this bug.

Thanks

Comment 19

[Gabriele Svelto [:gsvelto]]

Sure, you can pickup the existing patch and correct the issues that still remained or start from scratch, your choice! If you need help join the #breakpad channel on chat.mozilla.org, I'm usually hanging in there. I'm in GMT+1 so keep that in mind if I don't respond quickly.

Comment 20

[achmurali]

Attachment: Bug 1442053 - Change the following global variables from the C string to the C++ ones:pendingDirectory,crashReporterPath,memoryReportPath,libraryPath and eventsDirectory. r?gsvelto

Comment 21

[achmurali]

Hi Gabriele,

Updated the patch as per the comments, ran the tests locally and they seem to be fine.
Just had a question, was redirected here after asking on the matrix introduction channel -

1. Initially the tests were failing because of my changes, found it really difficult to figure out the issue as the logs were not much helpful. Could you please provide some pointers on where to look and if i was doing anything wrong above.

Thanks

Comment 22

[Gabriele Svelto [:gsvelto]]

(In reply to achmurali from comment #21)

Updated the patch as per the comments, ran the tests locally and they seem to be fine.
Just had a question, was redirected here after asking on the matrix introduction channel -

1. Initially the tests were failing because of my changes, found it really difficult to figure out the issue as the logs were not much helpful. Could you please provide some pointers on where to look and if i was doing anything wrong above.

I have run the tests on my machine with your patch applied but none of them failed, can you attach a full log with the failure on this bug so that I can figure out what's going wrong? Additionally you might want to run the test suites individually to narrow down which one is failing with the following commands:

./mach test toolkit/crashreporter/test/unit
./mach test toolkit/crashreporter/test/unit_ipc
./mach test toolkit/crashreporter/test/browser

Comment 23

[achmurali]

(In reply to Gabriele Svelto [:gsvelto] from comment #22)

(In reply to achmurali from comment #21)

Updated the patch as per the comments, ran the tests locally and they seem to be fine.
Just had a question, was redirected here after asking on the matrix introduction channel -

1. Initially the tests were failing because of my changes, found it really difficult to figure out the issue as the logs were not much helpful. Could you please provide some pointers on where to look and if i was doing anything wrong above.

I have run the tests on my machine with your patch applied but none of them failed, can you attach a full log with the failure on this bug so that I can figure out what's going wrong? Additionally you might want to run the test suites individually to narrow down which one is failing with the following commands:

./mach test toolkit/crashreporter/test/unit
./mach test toolkit/crashreporter/test/unit_ipc
./mach test toolkit/crashreporter/test/browser

Like the tests were failing on the first commit, so i wasn't able to figure out the issue by looking at the log. So, just wanted to learn how do i narrow down the issue and all. Now the tests are running fine.
Could you kindly please suggest what IDE should i prefer for MacOS, finding it hard with vscode.

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to achmurali from comment #23)

Like the tests were failing on the first commit, so i wasn't able to figure out the issue by looking at the log. So, just wanted to learn how do i narrow down the issue and all. Now the tests are running fine.
Could you kindly please suggest what IDE should i prefer for MacOS, finding it hard with vscode.

I don't use an IDE on macOS, I generally do most of my coding in Geany.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97765d3fb479
Change the following global variables from the C string to the C++ ones:pendingDirectory,crashReporterPath,memoryReportPath,libraryPath and eventsDirectory. r=gsvelto

Comment 26

[achmurali]

Thank You Gabriele, without your help wouldn't have completed my first bug :D

Comment 27

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/97765d3fb479

Comment 28

[Gabriele Svelto [:gsvelto]]

(In reply to achmurali from comment #26)

Thank You Gabriele, without your help wouldn't have completed my first bug :D

You're welcome! That's what mentoring is about :-)