1197306 - remove PR_snprintf calls in ipc/

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nathan Froyd [:froydnj]]

Steps:

1. Find all calls to PR_snprintf in ipc/.
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: 1197306.patch

Comment 2

[Giovanny Gongora [:gioyik]]

Hi Nathan,

I created a patch for this, could you review it and tell me if is OK? I am assigning this to myself to keep it on my dashboard and not loose it with all the bugzilla email I receive.

Thanks

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8651349 [details] [diff] [review]
1197306.patch

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

Thanks for the patch!  Sorry for forgetting one other step, which is to ensure the format string characters match up.  I'll detail the changes necessary below.

::: ipc/chromium/src/base/logging.cc
@@ -3,5 @@
>  // found in the LICENSE file.
>  
>  #include "base/logging.h"
>  #include "prmem.h"
> -#include "prprf.h"

Thanks for cleaning this up, too!

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +618,1 @@
>  	      "%ld", base::Process::Current().pid());

In this case, we'll want to change the string to "%d".

::: ipc/glue/MessageChannel.cpp
@@ +1692,1 @@
>                      "(msgtype=0x%lX,name=%s) %s",

Likewise here, this should be 0x%X.

@@ +1738,1 @@
>                  "(msgtype=0x%lX,name=%s) %s",

Likewise here, this should be 0x%X.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #3)
> Comment on attachment 8651349 [details] [diff] [review]
> 1197306.patch
> 
> Review of attachment 8651349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch!  Sorry for forgetting one other step, which is to
> ensure the format string characters match up.  I'll detail the changes
> necessary below.
> 
> ::: ipc/chromium/src/base/logging.cc
> @@ -3,5 @@
> >  // found in the LICENSE file.
> >  
> >  #include "base/logging.h"
> >  #include "prmem.h"
> > -#include "prprf.h"
> 
> Thanks for cleaning this up, too!

Sorry, I realized that this #include actually can't be removed, because logging.cc uses PR_vsprintf_append, which comes from prprf.h.  I should have been more explicit to only remove prprf.h includes from files that were changed. :)

Comment 5

[Giovanny Gongora [:gioyik]]

Yes, that happened in other patches I submitted too. So, I am leaving those files with `#include "prprf.h"` only, and modifying the ones who has `#include "prprf.h"` and `PR_snprintf`

Comment 6

[Giovanny Gongora [:gioyik]]

Attachment: 1197306.patch

Comment 7

[Giovanny Gongora [:gioyik]]

Nathan I attached a new patch. Let me know if this is OK.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8651445 [details] [diff] [review]
1197306.patch

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

Thanks for revising the patch!  I forgot to mention that the indentation on calls should be fixed, so I've noted those below.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +614,5 @@
>    // send the child the PID so that it can open a ProcessHandle back to us.
>    // probably don't want to do this in the long run
>    char pidstring[32];
> +  snprintf_literal(pidstring,
> +	      "%d", base::Process::Current().pid());

Please indent this line so that the "%d" lines up with |pidstring| on the line above.

::: ipc/glue/MessageChannel.cpp
@@ +1693,1 @@
>                      aMsg->type(), aMsg->name(), errorMsg);

Likewise for these two lines.

@@ +1739,1 @@
>                  aMsg.type(), aMsg.name(), errorMsg);

Likewise for these two lines.

Comment 9

[Anup Kumar [:reznord]]

Attachment: changes made as requested in comment 8.

Hi Nathan,

Here are the changes to the patch which you requested in comment 8, I hope these changes are good enough, if not please tell me what are the corrections to be made.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8672578 [details] [diff] [review]
changes made as requested in comment 8.

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

Thanks for revising the patch!  A couple of minor details below.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +614,5 @@
>    // send the child the PID so that it can open a ProcessHandle back to us.
>    // probably don't want to do this in the long run
>    char pidstring[32];
> +  snprintf_literal(pidstring,
> +       "%d", base::Process::Current().pid());

The indentation here still needs to be fixed, but as the line is now shorter, we can place the call and all its parameters on a single line.

::: ipc/glue/MessageChannel.cpp
@@ +1694,5 @@
>  
>      if (aMsg) {
>          char reason[512];
> +        snprintf_literal(reason, "(msgtype=0x%lX,name=%s) %s",
> +             aMsg->type(), aMsg->name(), errorMsg);

This line needs to be lined up underneath |reason| on the above line:

snprintf_literal(reason, ...
                 aMsg->type(), ...);

@@ +1739,5 @@
>      }
>  
>      char reason[512];
> +    snpritnf_literal(reason, "(msgtype=0x%lX,name=%s) %s",
> +         aMsg.type(), aMsg.name(), errorMsg);

Same alignment issues here.

Comment 11

[chaithanya]

Attachment: Made changes to function calls in ./ipc

Made necessary changes as mentioned in Comment 10

Comment 12

[chaithanya]

Attachment: Made final changes as per Comment 10

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8687920 [details] [diff] [review]
Made final changes as per Comment 10

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

Thanks for the patch!  There's at least one change to be made from comment 10, and it looks like some of the format string changes from earlier patches need to be redone.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +618,1 @@
>  	             "%ld", base::Process::Current().pid());

All the arguments to this call need to be on one line, as per comment 10.

The format string here should not be "%ld", it should be "%d", as per comment 3.

::: ipc/glue/MessageChannel.cpp
@@ +1692,5 @@
>  
>      if (aMsg) {
>          char reason[512];
> +        snprintf_literal(reason, "(msgtype=0x%lX,name=%s) %s",
> +                         aMsg->type(), aMsg->name(), errorMsg);

Likewise here from comment 3, this should be 0x%X.

@@ +1737,5 @@
>      }
>  
>      char reason[512];
> +    snprintf_literal(reason,"(msgtype=0x%lX,name=%s) %s",
> +                     aMsg.type(), aMsg.name(), errorMsg);

Likewise here from comment 3, this should be 0x%X.

Comment 14

[chaithanya]

Attachment: Changes have been made as per comment 13. In case of any more modifications please let me know

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8688016 [details] [diff] [review]
Changes have been made as per comment 13. In case of any more modifications please let me know

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

Thanks for the quick update!  One minor nit below:

::: ipc/glue/MessageChannel.cpp
@@ +1736,5 @@
>          return false;
>      }
>  
>      char reason[512];
> +    snprintf_literal(reason,"(msgtype=0x%lX,name=%s) %s",

This format string should use 0x%X now.

Comment 16

[chaithanya]

Attachment: Made comments as per comment15. Please let me know if there are any more changes to be made

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8688255 [details] [diff] [review]
Made comments as per comment15. Please let me know if there are any more changes to be made

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

The changes in this patch look good, but I can't actually apply the patch because of the problem below.  Can you please upload a new patch with that problem corrected?

::: ipc/glue/GeckoChildProcessHost.cpp
@@ -618,1 @@
>  

I think you based this patch off the wrong revision or something?  This bit of code doesn't even compile prior to your patch.

Comment 18

[chaithanya]

Attachment: Made changes as by comment17. Please let me know if there are any more changes to make

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8688573 [details] [diff] [review]
Made changes as by comment17. Please let me know if there are any more changes to make

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

This patch file also appears to be corrupt, and it doesn't appear to fix the original issue.

Comment 20

[chaithanya]

Attachment: New patch submitted as per comment19. Hope this is good enough,if in case of any changes please let me know

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8688618 [details] [diff] [review]
New patch submitted as per comment19. Hope this is good enough,if in case of any changes please let me know

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

::: ipc/glue/MessageChannel.cpp
@@ +1693,5 @@
>  
>      if (aMsg) {
>          char reason[512];
> +        snprintf(reason,"(msgtype=0x%X,name=%s) %s",
> +                 aMsg->type(), aMsg->name(), errorMsg);

This doesn't compile, because the call should be to snprintf_literal, not snprintf.

@@ +1738,5 @@
>      }
>  
>      char reason[512];
> +    snprintf(reason,"(msgtype=0x%X,name=%s) %s",
> +             aMsg.type(), aMsg.name(), errorMsg);

This doesn't compile either, for the same reasons.

Comment 22

[chaithanya]

Attachment: Made changes as by comment21.Hope the reqirements are met.Please let me know if any changes are to be made

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8689164 [details] [diff] [review]
Made changes as by comment21.Hope the reqirements are met.Please let me know if any changes are to be made

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

Thanks for working through the problems here!

Comment 24

[Andrew McCreight [:mccr8]]

Nathan, it looks like maybe this is ready to land? Well, pending a try run or something.

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> Nathan, it looks like maybe this is ready to land? Well, pending a try run
> or something.

Looks like it compiles OK on linux64 debug locally.  Try run:

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

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4537485ef2c2

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4537485ef2c2