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

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.

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

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)?

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.

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.

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?

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.

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?

(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 (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?

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!

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?

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

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

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

Hi gsvelto,

Can i work on this bug.

Thanks

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.

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

(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

(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.

(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.

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

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

(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 :-)