1197331 - remove PR_snprintf calls in miscellaneous directories

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Giovanny Gongora [:gioyik]]

Attachment: 1197331.patch

Comment 2

[Giovanny Gongora [:gioyik]]

Hi Nathan,

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

Thanks

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Param Singh]

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?

Comment 5

[Aniket Vyas]

Attachment: 1197331.patch

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

Regards,
Aniket

Comment 6

[Nathan Froyd [:froydnj]]

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.

Comment 7

[Aniket Vyas]

Attachment: 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.

Comment 8

[Nathan Froyd [:froydnj]]

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

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

Thank you!

Comment 9

[Aniket Vyas]

Hey Nathan, 

    Do you want me to send the patch for testing ? 

Thanks,
Aniket

Comment 10

[Nathan Froyd [:froydnj]]

If you have try access, please go ahead!

Comment 11

[Aniket Vyas]

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

Comment 12

[Nathan Froyd [:froydnj]]

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

Comment 13

[Aniket Vyas]

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

Comment 14

[Nathan Froyd [:froydnj]]

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

Comment 15

[Aniket Vyas]

Thanks a lot Nathan ! Sure will see to that :)

Comment 16

[Josh Matthews [:jdm]]

Aniket, did you ever push this to the tryserver as requested?

Comment 17

[Aniket Vyas]

Hi Josh, 

    Sorry but couldn't get it to it, and I am afraid would need some help with it.

Comment 18

[Josh Matthews [:jdm]]

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.

Comment 19

[Aniket Vyas]

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.

Comment 20

[chaithanya]

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

Comment 21

[chaithanya]

Attachment: Made changes as per required. Please let me know if any changes are to be made.Thanks!

Comment 22

[Nathan Froyd [:froydnj]]

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.

Comment 23

[chaithanya]

Attachment: Made changes on comment 22. Please review.

Comment 24

[Nathan Froyd [:froydnj]]

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?

Comment 25

[chaithanya]

Attachment: Sorry for that. I hope this works.

Comment 26

[Nathan Froyd [:froydnj]]

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

This does not appear to be a patch, but an HTML file?

Comment 27

[chaithanya]

Attachment: I'm sorry for that. I hope this solves the bug.

Comment 28

[Nathan Froyd [:froydnj]]

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!

Comment 29

[chaithanya]

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

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

[chaithanya]

Attachment: Made a new patch for the bug. Please review.

Comment 32

[Nathan Froyd [:froydnj]]

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!

Comment 33

[Vivek Jain]

Hi 
I am a newbie and want to ccontribute.Can somebody assign me to the bug and help me in the process?

Comment 34

[Josh Matthews [:jdm]]

Nathan, looks like this patch was forgotten.

Comment 35

[chaithanya]

I think the patch looks good. Please let me know if any changes are to be made.

Comment 36

[Nathan Froyd [:froydnj]]

The patch no longer applies.  chaitanya?

Comment 37

[chaithanya]

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.

Comment 38

[Josh Matthews [:jdm]]

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

[chaithanya]

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

Comment 40

[chaithanya]

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

Comment 41

[Nathan Froyd [:froydnj]]

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

Comment 42

[Igor]

I am highly willing to work on this. Can I produce another patch and post here?

Comment 43

[Nathan Froyd [:froydnj]]

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

Comment 44

[Igor]

Attachment: 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.

Comment 45

[Nathan Froyd [:froydnj]]

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.

Comment 46

[Igor]

Attachment: 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.

Comment 47

[Nathan Froyd [:froydnj]]

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

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

Looks great!

Comment 48

[Igor]

Thanks,

Now what would be the next steps? Can you push it to the TryServer and run the tests?

Comment 49

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57fa51ead7ac

Comment 50

[Igor]

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

Comment 51

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Those are all "tier 2" (that is, less important), so I'm just going to request a checkin here.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4321017e1f83
remove PR_snprintf calls in miscellaneous directories. r=nfroyd

Comment 53

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4321017e1f83

Comment 54

[Igor]

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!