remove PR_snprintf calls in miscellaneous directories

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: froydnj, Assigned: palmieri.igor, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

3 years ago
Steps:

1. Find all calls to PR_snprintf in:

image/
xpfe/
tools/
widget/
accessible/
browser/
toolkit/

(All of these directories only have a handful of calls each, so it wasn't worth filing separate bugs on each of them.)

2. Add #include "mozilla/Snprintf.h" to the file(s).
3. Replace PR_snprintf(X, sizeof(X), ...) calls with snprintf_literal(X, ...).
4. Replace other PR_snprintf calls with snprintf.
5. Remove any #include "prprf.h" lines.
Created attachment 8651456 [details] [diff] [review]
1197331.patch
Attachment #8651456 - Flags: review?(nfroyd)
Hi Nathan,

I attached a patch for this bug, let me know if is OK or needs changes.

Thanks
Assignee: nobody → gioyik
(Reporter)

Comment 3

3 years ago
Comment on attachment 8651456 [details] [diff] [review]
1197331.patch

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

Thanks for the patch!  Just a few minor fixes.

::: accessible/atk/AccessibleWrap.cpp
@@ +439,1 @@
>                  interfacesBits);

Please fixup the indentation of this line.

::: browser/components/shell/nsGNOMEShellService.cpp
@@ +545,5 @@
>    uint16_t red = COLOR_8_TO_16_BIT((aColor >> 16) & 0xff);
>    uint16_t green = COLOR_8_TO_16_BIT((aColor >> 8) & 0xff);
>    uint16_t blue = COLOR_8_TO_16_BIT(aColor & 0xff);
>  
> +  snprintf_literal(buf, "#%04x%04x%04x", red, green, blue);

This should be a call to |snprintf|, not snprintf_literal.

::: nsprpub/tools/httpget.c
@@ +17,5 @@
>  #include "prio.h"
>  #include "prnetdb.h"
>  #include "prlog.h"
>  #include "prerror.h"
> +#include "mozilla/Snprintf.h"

Please revert all changes to this file.  PR_snprintf is a part of NSPR and as such, it's OK that this file continue to call PR_snprintf.
Attachment #8651456 - Flags: review?(nfroyd) → feedback+

Comment 4

3 years ago
Hi, I would like to work on this bug as it seems to be an easy one. Can I take this up if Giovanny isn't working on it anymore?
Flags: needinfo?(gioyik)

Comment 5

3 years ago
Created attachment 8674033 [details] [diff] [review]
1197331.patch

Have attached a patch for the fixes. Please let me know if it needs any change. 

Regards,
Aniket
Attachment #8674033 - Flags: review?(nfroyd)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8674033 [details] [diff] [review]
1197331.patch

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

Looks good, except for one minor problem below.

::: accessible/atk/AccessibleWrap.cpp
@@ +437,5 @@
>      static gchar namePrefix[] = "MaiAtkType";   /* size = 10 */
>      static gchar name[MAI_ATK_TYPE_NAME_LEN + 1];
>  
> +    snprintf_literal(name, MAI_ATK_TYPE_NAME_LEN, "%s%x", namePrefix,
> +                     interfacesBits);

I don't think this compiles, because you left in the length argument (MAI_ATK_TYPE_NAME_LEN), but changed the call to use snprinf_literal, which doesn't take a length argument.  Removing the MAI_ATK_TYPE_NAME_LEN is the right thing to do here.
Attachment #8674033 - Flags: review?(nfroyd) → feedback+

Updated

3 years ago
Assignee: gioyik → vyas45

Comment 7

3 years ago
Created attachment 8674506 [details] [diff] [review]
1197331.patch

Oops my bad, attached the wrong patch. Updated the patch and the commit message. Please let me know if it needs any changes.
Attachment #8651456 - Attachment is obsolete: true
Attachment #8674033 - Attachment is obsolete: true
Attachment #8674506 - Flags: review?(nfroyd)
(Reporter)

Comment 8

3 years ago
Comment on attachment 8674506 [details] [diff] [review]
1197331.patch

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

Thank you!
Attachment #8674506 - Flags: review?(nfroyd) → review+

Comment 9

3 years ago
Hey Nathan, 

    Do you want me to send the patch for testing ? 

Thanks,
Aniket
Flags: needinfo?(nfroyd)
(Reporter)

Comment 10

3 years ago
If you have try access, please go ahead!
Flags: needinfo?(nfroyd)

Comment 11

3 years ago
Thanks Nathan, a followup question for you, what should be the ideal try "computed syntax" ?

I was looking at the wiki (https://wiki.mozilla.org/ReleaseEngineering/TryServer), and it suggests not running all the tests. So wanted to know what I should be running ? 

Regards,
Aniket
Flags: needinfo?(nfroyd)
(Reporter)

Comment 12

3 years ago
(In reply to Aniket Vyas from comment #11)
> I was looking at the wiki
> (https://wiki.mozilla.org/ReleaseEngineering/TryServer), and it suggests not
> running all the tests. So wanted to know what I should be running ? 

My usual default is:

-b do -p all -u all[x64] -t none

which builds on all platforms, but only runs tests on x86-64 Linux.  You can experiment with different options at:

http://trychooser.pub.build.mozilla.org/

(see especially the "Restrict tests to platforms" checkboxes in the lower part of the first column.)  I would definitely build on all platforms, regardless of what tests you decide to run.
Flags: needinfo?(nfroyd)

Comment 13

3 years ago
Thanks a lot Nathan. 

Also, could you please vouch for me at : https://bugzilla.mozilla.org/show_bug.cgi?id=1217619

This is for try server access privileges. 

Regards,
Aniket
Flags: needinfo?(nfroyd)
(Reporter)

Comment 14

3 years ago
(In reply to Aniket Vyas from comment #13)
> Also, could you please vouch for me at :
> https://bugzilla.mozilla.org/show_bug.cgi?id=1217619
> 
> This is for try server access privileges. 

Sure, done.  In the future, note that you only have to needinfo in one bug. ;)
Flags: needinfo?(nfroyd)

Comment 15

3 years ago
Thanks a lot Nathan ! Sure will see to that :)
Aniket, did you ever push this to the tryserver as requested?
Flags: needinfo?(gioyik) → needinfo?(vyas45)

Comment 17

3 years ago
Hi Josh, 

    Sorry but couldn't get it to it, and I am afraid would need some help with it.
Flags: needinfo?(vyas45)
If you would like to continue working on this please let us know (and ask questions about anything that isn't working!), or let us know if we should unassign you.
Flags: needinfo?(vyas45)

Comment 19

3 years ago
On this particular bug, I think the turnaround would be quicker if I am unassigned as this month looks quite occupied with my day job.
Flags: needinfo?(vyas45)

Updated

3 years ago
Assignee: vyas45 → nobody

Comment 20

3 years ago
Hi
  I would like to work on this bug , could you assign it to me?

Updated

3 years ago
Assignee: nobody → chaitanya7991

Comment 21

3 years ago
Created attachment 8727358 [details] [diff] [review]
Made changes as per required. Please let me know if any changes are to be made.Thanks!
Attachment #8727358 - Flags: review?(nfroyd)
(Reporter)

Updated

3 years ago
Attachment #8727358 - Attachment is patch: true
(Reporter)

Comment 22

3 years ago
Comment on attachment 8727358 [details] [diff] [review]
Made changes as per required. Please let me know if any changes are to be made.Thanks!

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

Thanks for the patch; I'm not sure what you had to change as the previous version of the patch was approved and it just had to be tested. :)

r=me with the following change below:

::: accessible/atk/AccessibleWrap.cpp
@@ +437,5 @@
>      static gchar namePrefix[] = "MaiAtkType";   /* size = 10 */
>      static gchar name[MAI_ATK_TYPE_NAME_LEN + 1];
>  
> +    snprintf_literal(name,"%s%x", namePrefix,
> +                     interfacesBits);

It looks like you wrapped |interfacesBits| onto the next line, even though previous versions had placed it on the same line as the |snprintf_literal| call.  Please put it back on the same line as the |snprintf_literal| call.
Attachment #8727358 - Flags: review?(nfroyd) → review+

Comment 23

3 years ago
Created attachment 8728806 [details] [diff] [review]
Made changes on comment 22. Please review.
Attachment #8727358 - Attachment is obsolete: true
Attachment #8728806 - Flags: review?(nfroyd)
(Reporter)

Updated

3 years ago
Attachment #8728806 - Attachment is patch: true
(Reporter)

Comment 24

3 years ago
Comment on attachment 8728806 [details] [diff] [review]
Made changes on comment 22. Please review.

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

This is the exact same patch as the one you posted in comment 21.  Did you forget to refresh your patch queue or commit the changes?
Attachment #8728806 - Flags: review?(nfroyd)

Comment 25

3 years ago
Created attachment 8729076 [details]
Sorry for that. I hope this works.
Attachment #8728806 - Attachment is obsolete: true
Attachment #8729076 - Flags: review?(nfroyd)
(Reporter)

Comment 26

3 years ago
Comment on attachment 8729076 [details]
Sorry for that. I hope this works.

This does not appear to be a patch, but an HTML file?
Attachment #8729076 - Flags: review?(nfroyd)

Comment 27

3 years ago
Created attachment 8729441 [details] [diff] [review]
I'm sorry for that. I hope this solves the bug.
Attachment #8729076 - Attachment is obsolete: true
Attachment #8729441 - Flags: review?(nfroyd)
(Reporter)

Comment 28

3 years ago
Comment on attachment 8729441 [details] [diff] [review]
I'm sorry for that. I hope this solves the bug.

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

Thank you!
Attachment #8729441 - Flags: review?(nfroyd) → review+

Comment 29

3 years ago
(In reply to Nathan Froyd [:froydnj] from comment #28)
> Comment on attachment 8729441 [details] [diff] [review]
> I'm sorry for that. I hope this solves the bug.
> 
> Review of attachment 8729441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you!

Can we get this landed?
Keywords: checkin-needed
does not apply cleanly:

applying attachment.cgi?id=8674506
patching file xpfe/appshell/nsXULWindow.cpp
Hunk #2 FAILED at 1503
Hunk #3 FAILED at 1554
2 out of 3 hunks FAILED -- saving rejects to file xpfe/appshell/nsXULWindow.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh attachment.cgi?id=8674506
Flags: needinfo?(chaitanya7991)
Keywords: checkin-needed

Comment 31

3 years ago
Created attachment 8745045 [details] [diff] [review]
Made a new patch for the bug. Please review.
Attachment #8729441 - Attachment is obsolete: true
Attachment #8745045 - Flags: review?(nfroyd)
(Reporter)

Comment 32

3 years ago
Comment on attachment 8745045 [details] [diff] [review]
Made a new patch for the bug. Please review.

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

Thanks for updating the patch!
Attachment #8745045 - Flags: review?(nfroyd) → review+
(Reporter)

Updated

3 years ago
Attachment #8674506 - Attachment is obsolete: true

Comment 33

3 years ago
Hi 
I am a newbie and want to ccontribute.Can somebody assign me to the bug and help me in the process?
Nathan, looks like this patch was forgotten.
Flags: needinfo?(nfroyd)

Comment 35

3 years ago
I think the patch looks good. Please let me know if any changes are to be made.
(Reporter)

Comment 36

3 years ago
The patch no longer applies.  chaitanya?
Flags: needinfo?(nfroyd)

Comment 37

3 years ago
Does the bug still exists? I popped out all the existing patches and updated my local tree. Now I see that the changes are already there in my local tree.
It looks like the changes are not present in mozilla-central: https://hg.mozilla.org/mozilla-central/file/tip/accessible/atk/AccessibleWrap.cpp#l439 . Maybe you accidentally committed them?

Comment 39

2 years ago
(In reply to Josh Matthews [:jdm] from comment #38)
> It looks like the changes are not present in mozilla-central:
> https://hg.mozilla.org/mozilla-central/file/tip/accessible/atk/
> AccessibleWrap.cpp#l439 . Maybe you accidentally committed them?


I just cloned the Mozilla-Central and have not committed any change. In my local tree, the changes already seem to be implemented.
Flags: needinfo?(chaitanya7991)

Comment 40

2 years ago
(In reply to chaithanya from comment #39)
> (In reply to Josh Matthews [:jdm] from comment #38)
> > It looks like the changes are not present in mozilla-central:
> > https://hg.mozilla.org/mozilla-central/file/tip/accessible/atk/
> > AccessibleWrap.cpp#l439 . Maybe you accidentally committed them?
> 
> 
> I just cloned the Mozilla-Central and have not committed any change. In my
> local tree, the changes already seem to be implemented.
(Reporter)

Comment 41

2 years ago
(In reply to chaithanya from comment #39)
> (In reply to Josh Matthews [:jdm] from comment #38)
> > It looks like the changes are not present in mozilla-central:
> > https://hg.mozilla.org/mozilla-central/file/tip/accessible/atk/
> > AccessibleWrap.cpp#l439 . Maybe you accidentally committed them?
> 
> 
> I just cloned the Mozilla-Central and have not committed any change. In my
> local tree, the changes already seem to be implemented.

grepping through a checkout of mozilla-central shows that the PR_snprintf calls are still there in a number of places that are touched by this patch:

./browser/components/shell/nsGNOMEShellService.cpp:519:  PR_snprintf(buf, 14, "#%04x%04x%04x", red, green, blue);
./toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:339:    PR_snprintf(buf, sizeof(buf), "%u", val & 0xff);
./image/decoders/icon/nsIconURI.cpp:97:    PR_snprintf(buf, sizeof(buf), "%d", mSize);
./xpfe/appshell/nsXULWindow.cpp:1603:      PR_snprintf(sizeBuf, sizeof(sizeBuf), "%d",
./xpfe/appshell/nsXULWindow.cpp:1612:      PR_snprintf(sizeBuf, sizeof(sizeBuf), "%d",
./xpfe/appshell/nsXULWindow.cpp:1624:      PR_snprintf(sizeBuf, sizeof(sizeBuf), "%d",
./xpfe/appshell/nsXULWindow.cpp:1633:      PR_snprintf(sizeBuf, sizeof(sizeBuf), "%d",
./xpfe/appshell/nsXULWindow.cpp:1663:        PR_snprintf(sizeBuf, sizeof(sizeBuf), "%lu", (unsigned long)zLevel);
./tools/profiler/core/ProfilerMarkers.cpp:177:  PR_snprintf(buffer, bufferSize, "%p", mLayer);
./widget/xremoteclient/XRemoteClient.cpp:296:    PR_snprintf(pidstr, sizeof(pidstr), "pid%d@", getpid());

So I believe this patch has not yet landed.
Flags: needinfo?(chaitanya7991)
(Assignee)

Comment 42

2 years ago
I am highly willing to work on this. Can I produce another patch and post here?
(Reporter)

Comment 43

2 years ago
(In reply to Igor from comment #42)
> I am highly willing to work on this. Can I produce another patch and post
> here?

Yes please!
Flags: needinfo?(chaitanya7991)
(Assignee)

Comment 44

2 years ago
Created attachment 8777306 [details] [diff] [review]
bug-1197331.patch

I am adding a patch for this bug too. If there is anything that I can do to help further, please let me know.
Attachment #8777306 - Flags: review?(nfroyd)
(Reporter)

Comment 45

2 years ago
Comment on attachment 8777306 [details] [diff] [review]
bug-1197331.patch

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

Looks good, just two small things to fix!

::: accessible/atk/AccessibleWrap.cpp
@@ +436,5 @@
>      static gchar namePrefix[] = "MaiAtkType";   /* size = 10 */
>      static gchar name[MAI_ATK_TYPE_NAME_LEN + 1];
>  
> +    snprintf_literal(name, "%s%x", namePrefix,
> +                     interfacesBits);

Nit: interfacesBits can fit on the line above.

::: xpfe/appshell/nsXULWindow.cpp
@@ +1656,5 @@
>        uint32_t zLevel;
>        nsCOMPtr<nsIWindowMediator> mediator(do_GetService(NS_WINDOWMEDIATOR_CONTRACTID));
>        if (mediator) {
>          mediator->GetZLevel(this, &zLevel);
> +        snprintf_literal(sizeBuf, "%" PRIu32, static_cast<uint32_t>(zLevel));

zLevel is already a uint32_t, there's no need for the static_cast.
Attachment #8777306 - Flags: review?(nfroyd) → review+
(Reporter)

Updated

2 years ago
Attachment #8745045 - Attachment is obsolete: true
(Assignee)

Comment 46

2 years ago
Created attachment 8778006 [details] [diff] [review]
bug-1197331.patch - revised

Thanks for the feedback!
I attached a new patch with the corrections. Please let me know if there is anything else that I could do to help.
Attachment #8777306 - Attachment is obsolete: true
Attachment #8778006 - Flags: review?(nfroyd)
(Reporter)

Comment 47

2 years ago
Comment on attachment 8778006 [details] [diff] [review]
bug-1197331.patch - revised

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

Looks great!
Attachment #8778006 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 48

2 years ago
Thanks,

Now what would be the next steps? Can you push it to the TryServer and run the tests?
Flags: needinfo?(nfroyd)
Assignee: chaitanya7991 → palmieri.igor
(Assignee)

Comment 50

2 years ago
Thanks!

The errors for the Windows tests seems to be related to the test itself:

> 09:58:49    FATAL - Automation Error: Can't checkout https://hg.mozilla.org/build/tools!
> 09:58:49    FATAL - Caught exception: repo checkout failed!

As for the Android error, can someone help me to get more info?

> 10:22:34     INFO -  Notification center failed: Install the python dbus module to get a notification when the build finishes.
> 10:22:34    ERROR - Return code: 2
> 10:22:34  WARNING - setting return code to 2
> 10:22:34    FATAL - 'mach build' did not run successfully. Please check log for errors.
> 10:22:34    FATAL - Running post_fatal callback...
> 10:22:34    FATAL - Exiting -1
Those are all "tier 2" (that is, less important), so I'm just going to request a checkin here.
Keywords: checkin-needed

Comment 52

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4321017e1f83
remove PR_snprintf calls in miscellaneous directories. r=nfroyd
Keywords: checkin-needed

Comment 53

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4321017e1f83
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 54

2 years ago
I am looking for new bugs to get more involved in the project. Feel free to suggest me any other bugs or any task to help.
 
Thanks!
You need to log in before you can comment on or make changes to this bug.