Replace NS_tstrcmp with NS_tstrncmp in app update
Categories
(Toolkit :: Application Update, task, P5)
Tracking
()
People
(Reporter: spohl, Assigned: rogheliavandan, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file)
NS_tstrncmp is landing in bug 1574139. We should switch from NS_tstrcmp to this safer version in app update. Current occurrences:
|
||
Updated•5 years ago
|
|
||
Comment 1•5 years ago
|
||
Hello, May I submit a patch for this?
|
Reporter | |
Comment 2•5 years ago
|
||
(In reply to Jayati Shrivastava from comment #1)
Hello, May I submit a patch for this?
Sure! Please make sure to also remove the definition for NS_tstrcmp in your patch after switching over to NS_tstrncmp. Thanks!
|
|
||
Comment 3•5 years ago
|
||
What should be passed as the third argument to NS_tstrncmp- the strlen of the first or second argument?
|
|
Reporter | |
Comment 4•5 years ago
|
||
(In reply to Jayati Shrivastava from comment #3)
What should be passed as the third argument to
NS_tstrncmp- the strlen of the first or second argument?
Molly is the mentor on this bug, but good-first-bugs are meant as a learning opportunity. I encourage you to read the documentation for strncmp[1] and to read up on why the use of strcmp[for example 2] is discouraged.
[1] https://linux.die.net/man/3/strncmp
[2] https://stackoverflow.com/questions/24353504/whats-wrong-with-strcmp
Hello! If no one is working on this bug at the moment, I would like to work on it.
|
|
Reporter | |
Comment 6•5 years ago
|
||
Please go ahead. Once you have submitted a patch for this, we will assign the bug to you. Thanks!
Currently, I have an Artifact build on my machine. Will I be able to fix this bug on it? Also which chat room is best suited to ask questions regarding this bug?
|
|
Reporter | |
Comment 8•5 years ago
|
||
The best resource for a mentored bug is typically the mentor that's assigned to this bug. I do not believe that an artifact build will be sufficient here, since the changes will need to occur in C++ code which doesn't get recompiled during artifact builds. Once you have a full build, I encourage you to read the documentation referenced in comment 4 and see if that's enough to fix the bug. Thank you for looking into this!
|
|
||
Updated•5 years ago
|
Alright! I will start working on this bug as soon as my full build is ready. Thank you for helping me out!
|
Assignee | |
Comment 10•5 years ago
|
||
Hi, I am ready with a patch :). I would like to test it locally first. How can I do so?
|
|
Reporter | |
Comment 11•5 years ago
|
||
I'm setting a need-info for :nalexander, who is the mentor on this bug. However, I will say that this is rather difficult to "test" because we are not looking for a functional change. For this kind of patch, the correctness will need to be determined during code review. If you feel that your patch is ready for a first look, you can submit to Phabricator as explained here:
|
|
Assignee | |
Comment 12•5 years ago
|
||
|
||
Updated•5 years ago
|
|
||
Comment 13•5 years ago
|
||
(In reply to Vandan from comment #10)
Hi, I am ready with a patch :). I would like to test it locally first. How can I do so?
Hi Vandan! Testing locally will be about ensuring that this builds (in as many environments as you can test) and that Firefox appears to work correctly. We'll run the series through try to get many more build platforms (and run some test jobs), which will also verify that we haven't introduced any new compile-time warnings.
In parallel, my review should catch issues. I see your patch, so expect some feedback shortly. Welcome aboard!
|
|
Assignee | |
Comment 14•5 years ago
|
||
Hey Nick, I have submitted a new patch for reviewing. Can you please tell me if there are any further corrections to it? Thank you!
|
|
||
Comment 15•5 years ago
|
||
(In reply to Vandan from comment #14)
Hey Nick, I have submitted a new patch for reviewing. Can you please tell me if there are any further corrections to it? Thank you!
Vandan -- sorry for the delay! I think my day will be full today but I should get you feedback tomorrow. Sorry again! (Leaving NI so I don't miss this.)
|
|
Assignee | |
Comment 16•5 years ago
|
||
Hey Nick, I have submitted a new patch. Can you please review it?
|
|
||
Comment 17•5 years ago
|
||
(In reply to Vandan from comment #16)
Hey Nick, I have submitted a new patch. Can you please review it?
Clearing NI: this was reviewed and we elected not to pursue this approach. Thanks for taking it on, Vandan, and sorry for the disappointing outcome.
Description
•