Closed Bug 1197311 Opened 9 years ago Closed 8 years ago

remove PR_snprintf calls in dom/

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: froydnj, Assigned: sakshivaid95, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 16 obsolete files)

63.22 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Steps:

1. Find all calls to PR_snprintf in dom/.
2. Add #include "mozilla/Snprintf.h" to the file.
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.

It's worth noting that the calls in dom/system/gonk/NetworkUtils.cpp should be rewritten using snprintf_literal, rather than snprintf.
Am I OK to work on this as my first Bug?
(In reply to jmtrik from comment #1)
> Am I OK to work on this as my first Bug?

Yes!
That's great. Can you assign it to me?
Assignee: nobody → jmtrik
Attached patch 1197311.patch (obsolete) — Splinter Review
Partial changes for Bug. There are still more changes to be made.
Attachment #8651955 - Attachment description: <strike>1197311.patch</strike> → 1197311.patch
I have made some of the changes (see the attached patch). I have a few questions about the remaining files that I need to change.

I have thes three files left to change.

/dom/plugins/base/nsPluginHost.cpp
/dom/media/webrtc/TRCCertificate.cpp
/dom/system/gonk/NetworkUtils.cpp

I will list the issues I have for each file.
-----------

/dom/plugins/base/nsPluginHost.cpp

The only occurance of PR_snprintf is on line 3771. I don't think we can change this as l != sizeof(p). Do you agree?

-
	uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32;
    newBufferLen = dataLen + l;
    if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen)))
      return NS_ERROR_OUT_OF_MEMORY;
    headersLen = PR_snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF);
	
-

----------

/dom/media/webrtc/TRCCertificate.cpp
Similar story here. On line 98 we have
-
 PR_snprintf(&buf[i * 2 + 3], 2, "%.2x", randomName[i]);
 -
This should clearly not be changed. Again, do you agree?

---------

/dom/system/gonk/NetworkUtils.cpp
There seem to be inconsistencies with the value being passed as the second argument here. Is this for a good reason or can all of the occurances of lines like the following be changed

PR_snprintf(key, PROPERTY_KEY_MAX - 1, .....
PR_snprintf(gCurrentCommand.command, MAX_COMMAND_SIZE - 1,...
PR_snprintf(command, MAX_COMMAND_SIZE - 1,....
PR_snprintf(command, sizeof command,....              

My worry is the last two seem to be inconsistent.
(In reply to jmtrik from comment #5)
> I have made some of the changes (see the attached patch). I have a few
> questions about the remaining files that I need to change.

Great!  It's also worth noting that I forgot a step, which is to ensure the format characters correspond to the kind of data they're formatting.  This is mostly ensuring that %ld, %lu, %lld, %llu, and similar get changed to use the standard formatting sequences.

> /dom/plugins/base/nsPluginHost.cpp
> 
> The only occurance of PR_snprintf is on line 3771. I don't think we can
> change this as l != sizeof(p). Do you agree?

You can change it to call snprintf instead of PR_snprintf.

> /dom/media/webrtc/TRCCertificate.cpp
> Similar story here. On line 98 we have
> -
>  PR_snprintf(&buf[i * 2 + 3], 2, "%.2x", randomName[i]);
>  -
> This should clearly not be changed. Again, do you agree?

Please change it to call snprintf instead.

> /dom/system/gonk/NetworkUtils.cpp
> There seem to be inconsistencies with the value being passed as the second
> argument here. Is this for a good reason or can all of the occurances of
> lines like the following be changed
> 
> PR_snprintf(key, PROPERTY_KEY_MAX - 1, .....
> PR_snprintf(gCurrentCommand.command, MAX_COMMAND_SIZE - 1,...
> PR_snprintf(command, MAX_COMMAND_SIZE - 1,....
> PR_snprintf(command, sizeof command,....              

You should be able to change all of these to call snprintf_literal.  It's not clear to me why the -1 was subtracted in these cases, but it's not necessary.
OK thanks Nathan. That all makes sense. I'll finish making those changes.
Attached patch 1197311.patch (obsolete) — Splinter Review
Nathan, could you check this patch?

Also, do you want strings like this changing to cross-platform macros?

LOG("AppleVDADecoder[%s] status %d flags %d retainCount %ld",
Attachment #8651955 - Attachment is obsolete: true
(In reply to jmtrik from comment #8)
> Nathan, could you check this patch?

I apologize for not responding to this!  I'll look at the patch.

> Also, do you want strings like this changing to cross-platform macros?
> 
> LOG("AppleVDADecoder[%s] status %d flags %d retainCount %ld",

I'm not sure what you mean here; could you elaborate?
Flags: needinfo?(jmtrik)
Made all the changes and fixed the indentation in the patch.
Assignee: jmtrik → allamsetty.anup
Attachment #8652050 - Attachment is obsolete: true
Attachment #8688031 - Flags: review?(nfroyd)
Comment on attachment 8688031 [details] [diff] [review]
made the necessary changes as requested in the bug.

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

Thanks for fixing up the patch!  I noted a number of issues below.

I think all the MAX_COMMAND_SIZE - 1 snprintf calls could really be snprintf_literal, but I'm unsure of whether snprintf on Firefox OS would do the correct thing there.

::: dom/base/nsDocument.cpp
@@ +2944,5 @@
>    PRExplodedTime prtime;
>    PR_ExplodeTime(aTime, PR_LocalTimeParameters, &prtime);
>    // "MM/DD/YYYY hh:mm:ss"
>    char formatedTime[24];
> +  if (snprintf_literal(formatedTime, "%02ld/%02ld/%04hd %02ld:%02ld:%02ld",

The "%ld" format strings should be changed to "%d", and the "%hd" format strings should be changed to "%d" with the corresponding field (prtime.tm_year) changed to be cast to |int|.

::: dom/base/nsGlobalWindow.cpp
@@ +1770,5 @@
>      nsAutoCString uri;
>      if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) {
>        tmp->mDoc->GetDocumentURI()->GetSpec(uri);
>      }
> +    snprintf_literal(name, "nsGlobalWindow #%llu %s %s", tmp->mWindowID,

The %llu here should be changed to use "%" PRIu64.

::: dom/base/nsXMLContentSerializer.cpp
@@ +604,5 @@
>  nsXMLContentSerializer::GenerateNewPrefix(nsAString& aPrefix)
>  {
>    aPrefix.Assign('a');
>    char buf[128];
> +  snprintf_literal(buf,, "%d", mPrefixIndex++);

The argument list here contains an extra comma.

::: dom/events/DOMEventTargetHelper.cpp
@@ +30,5 @@
>      nsAutoString uri;
>      if (tmp->mOwnerWindow && tmp->mOwnerWindow->GetExtantDoc()) {
>        tmp->mOwnerWindow->GetExtantDoc()->GetDocumentURI(uri);
>      }
> +    snprintf(name, "DOMEventTargetHelper %s",

This should be snprintf_literal.

::: dom/media/MediaManager.cpp
@@ +2507,5 @@
>    uint64_t outerID = outer->WindowID();
>  
>    // Notify the UI that this window no longer has gUM active
>    char windowBuffer[32];
> +  snprintf_literal(windowBuffer, "%llu", outerID);

The format string here should use "%" PRIu64.

::: dom/plugins/base/nsPluginHost.cpp
@@ +3758,5 @@
>      uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32;
>      newBufferLen = dataLen + l;
>      if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen)))
>        return NS_ERROR_OUT_OF_MEMORY;
> +    headersLen = snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF);

The "%ld" in this format string should be "%d".

::: dom/system/gonk/NetworkUtils.cpp
@@ +652,5 @@
>                                   CommandCallback aCallback,
>                                   NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "nat disable %s %s 0", 

Nit: Please don't add trailing whitespace.

@@ +663,5 @@
>                                    CommandCallback aCallback,
>                                    NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "nat enable %s %s 0", 

Nit: Please don't add trailing whitespace.

@@ +749,5 @@
>                              CommandCallback aCallback,
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, 

Nit: please don't add trailing whitespace.

@@ +761,5 @@
>                                    NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
>  
> +  snprintf(command, MAX_COMMAND_SIZE - 1, 

Nit: please don't add trailing whitespace.

::: dom/wifi/WifiUtils.cpp
@@ +367,5 @@
>    int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) {
>      char command[COMMAND_SIZE];
>      if (!strcmp(iface, "p2p0")) {
>        // Commands for p2p0 interface don't need prefix
> +      snprintf(command, COMMAND_SIZE, "%s", cmd);

This could be snprintf_literal.

@@ +372,3 @@
>      }
>      else {
> +      snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd);

This could be snprintf_literal.
Attachment #8688031 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #11)
> Comment on attachment 8688031 [details] [diff] [review]
> made the necessary changes as requested in the bug.
> 
> Review of attachment 8688031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for fixing up the patch!  I noted a number of issues below.

Okay, I will fix the issues and will update the patch.

> 
> I think all the MAX_COMMAND_SIZE - 1 snprintf calls could really be
> snprintf_literal, but I'm unsure of whether snprintf on Firefox OS would do
> the correct thing there.

Should I "NEEDINFO dohlbert" here for this?
Attached patch updated patch (obsolete) — Splinter Review
Made all the changes as requested in comment 11 and updated the patch.

Should we needinfo dohlbert here for this bug? because he might be helpful for the doubts related to snprintf_literal
Attachment #8688031 - Attachment is obsolete: true
Attachment #8688270 - Flags: review?(nfroyd)
Attachment #8688270 - Flags: feedback?(nfroyd)
Comment on attachment 8688270 [details] [diff] [review]
updated patch

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

Thanks for the patch.

For future patches you only need to set the review flag; I typically set the feedback flag to f+ to indicate that I think the patch is going in the correct general direction, even though it doesn't pass review.

::: dom/base/nsGlobalWindow.cpp
@@ +1770,5 @@
>      nsAutoCString uri;
>      if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) {
>        tmp->mDoc->GetDocumentURI()->GetSpec(uri);
>      }
> +    snprintf_literal(name, "nsGlobalWindow #"%" PRIu64 %s %s", tmp->mWindowID,

This format string doesn't compile because (writing some spaces in here):

"nsGlobalWindow #" % " PRIu64 %s %s"

is trying to apply the |%| operator to two strings, which isn't going to work.

It also looks like this file may need to include mozilla/IntegerPrintfMacros.h.

::: dom/wifi/WifiUtils.cpp
@@ +372,3 @@
>      }
>      else {
> +      snprintf_literal(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd);

These snprintf_literal calls can't be correct, because COMMAND_SIZE isn't a format string.  Please fix them.
Attachment #8688270 - Flags: review?(nfroyd)
Attachment #8688270 - Flags: feedback?(nfroyd)
Attachment #8688270 - Flags: feedback+
Attached file Bug Error (obsolete) —
Hi,
I have followed all instructions provided and I am receiving the following errors provided in the attachment. Can you please give me an idea of why this is happening. Thanks
(In reply to Salah from comment #15)
> Created attachment 8690422 [details]
> Bug Error
> 
> Hi,
> I have followed all instructions provided and I am receiving the following
> errors provided in the attachment. Can you please give me an idea of why
> this is happening. Thanks

Hi Salah, 

I am working on this bug, will be updating the patch soon, and will fix all the errors.
Attached patch bug1197311.patch (obsolete) — Splinter Review
I have updated the patch as you requested in comment 14, but still the build is failing, don't know what the problem is.

Can you please help me out?
Attachment #8688270 - Attachment is obsolete: true
Attachment #8690422 - Attachment is obsolete: true
Attachment #8690537 - Flags: review?(nfroyd)
Comment on attachment 8690537 [details] [diff] [review]
bug1197311.patch

(In reply to Anup Kumar from comment #17)
> I have updated the patch as you requested in comment 14, but still the build
> is failing, don't know what the problem is.
> 
> Can you please help me out?

It's difficult to help you figure out what the problem is if you don't provide details of what the build failure is.
Attachment #8690537 - Flags: review?(nfroyd)
Here is the build error as you requested. 

It is producing the error at "snprintf_literal(name, "nsGlobalWindow #"%"PRIu64 %s %s", tmp->mWindowID,"
Attachment #8690666 - Flags: review?(nfroyd)
Build error with the patch "bug1197311.patch" applied.
Attachment #8690666 - Attachment is obsolete: true
Attachment #8690666 - Flags: review?(nfroyd)
(In reply to Anup Kumar from comment #19)
> Created attachment 8690666 [details]
> build error while trying to run with the latest patch applied
> 
> Here is the build error as you requested. 
> 
> It is producing the error at "snprintf_literal(name, "nsGlobalWindow
> #"%"PRIu64 %s %s", tmp->mWindowID,"

I addressed this in comment 14.  The '%' character should be inside a string, not outside it.

Your other build error:

3:15.88 /home/reznord/mozilla/dom/media/MediaManager.cpp:2508:34: warning: invalid suffix on literal; C++11 requires a space between literal and identifier [-Wliteral-suffix]
 3:15.88    snprintf_literal(windowBuffer, "%"PRIu64, outerID);

means that you need to insert a space between |PRIu64| and any strings you're concatenating it with: |"%" PRIu64|, and you'll likely need to do the same for the nsGlobalWindow.cpp modification, too.
Attached patch Fixed all the errors in dom/ (obsolete) — Splinter Review
Fixed all the errors from the previous comments and tested the patch in my local tree by building it. The build is successful for me.

Please tell me if any more changes are requires for this patch.
Attachment #8690537 - Attachment is obsolete: true
Attachment #8690668 - Attachment is obsolete: true
Flags: needinfo?(jmtrik)
Attachment #8691762 - Flags: review?(nfroyd)
Comment on attachment 8691762 [details] [diff] [review]
Fixed all the errors in dom/

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

Thanks for fixing the build errors, though I'm a bit curious why the compiler didn't complain about the snprintf_literal calls pointed out below.

::: dom/base/nsGlobalWindow.cpp
@@ +1771,5 @@
>      nsAutoCString uri;
>      if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) {
>        tmp->mDoc->GetDocumentURI()->GetSpec(uri);
>      }
> +    snprintf_literal(name, "nsGlobalWindow # %PRIu64 %s %s", tmp->mWindowID,

This compiles, but it doesn't do the right thing at runtime, since the format directive is now %P...

::: dom/system/gonk/NetworkUtils.cpp
@@ +372,5 @@
>   */
>  static void getIFProperties(const char* ifname, IFProperties& prop)
>  {
>    char key[PROPERTY_KEY_MAX];
> +  snprintf_literal(key, PROPERTY_KEY_MAX - 1, "net.%s.gw", ifname);

This snprintf_literal call should be calling snprintf instead.  There are a lot of calls like this in the remainder of the patch; I'm not going to point out each and every one.  Please fix them all.
Attachment #8691762 - Flags: review?(nfroyd) → feedback+
Status: NEW → ASSIGNED
Attached patch Fixed all the errors in dom/ (obsolete) — Splinter Review
Made the changes requested in comment 23.
Attachment #8691762 - Attachment is obsolete: true
Attachment #8703581 - Flags: review?(nfroyd)
Comment on attachment 8703581 [details] [diff] [review]
Fixed all the errors in dom/

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

Thanks for the updates.  Still some minor things to fix, and one or two significant ones, which I'll ni? folks about.

::: dom/base/nsGlobalWindow.cpp
@@ +19,5 @@
>  #include "nsIDOMStorageManager.h"
>  #include "mozilla/dom/DOMStorage.h"
>  #include "mozilla/dom/StorageEvent.h"
>  #include "mozilla/dom/StorageEventBinding.h"
> +#include "mozilla/IntegerPrintfMacros.h"f

I guess this doesn't compile?

@@ +1787,5 @@
>      nsAutoCString uri;
>      if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) {
>        tmp->mDoc->GetDocumentURI()->GetSpec(uri);
>      }
> +    snprintf_literal(name, "nsGlobalWindow # %" PRIu64" %s %s", tmp->mWindowID,

Please insert a space between PRIu64 and the subsequent string.

::: dom/plugins/base/nsPluginHost.cpp
@@ +3756,5 @@
>      uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32;
>      newBufferLen = dataLen + l;
>      if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen)))
>        return NS_ERROR_OUT_OF_MEMORY;
> +    headersLen = snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF);

Please change the %ld here to use %u.

::: dom/system/gonk/NetworkUtils.cpp
@@ +713,5 @@
>                              CommandCallback aCallback,
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s %lld", GET_CHAR(mIfname), LLONG_MAX);

Please change the %lld to use PRId64, and the LLONG_MAX to INT64_MAX.

@@ +733,5 @@
>                              CommandCallback aCallback,
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld", GET_CHAR(mIfname), GET_FIELD(mThreshold));

Please change this %lld to use %d.  It looks like mThreshold was silently changed in bug 1209654 to be a 64-bit integer without updating this format string, so I think changing the field access to int32_t(GET_FIELD(mThreshold)) is correct...

@@ +754,5 @@
>                                    NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
>  
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %ld", GET_FIELD(mThreshold));

Likewise here.

@@ +2039,5 @@
>      // Set default froute from system properties.
>      char key[PROPERTY_KEY_MAX];
>      char gateway[PROPERTY_KEY_MAX];
>  
> +    snprintf_literal(key, sizeof key - 1, "net.%s.gw", autoIfname.get());

This snprintf_literal call can't be correct, please fix it.

::: dom/wifi/WifiUtils.cpp
@@ +368,5 @@
>    int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) {
>      char command[COMMAND_SIZE];
>      if (!strcmp(iface, "p2p0")) {
>        // Commands for p2p0 interface don't need prefix
> +      snprintf(command, COMMAND_SIZE, "%s", cmd);

This could be snprintf_literal, yes?

@@ +373,3 @@
>      }
>      else {
> +      snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd);

Same here.
Attachment #8703581 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #25)
> @@ +733,5 @@
> >                              CommandCallback aCallback,
> >                              NetworkResultOptions& aResult)
> >  {
> >    char command[MAX_COMMAND_SIZE];
> > +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld", GET_CHAR(mIfname), GET_FIELD(mThreshold));
> 
> Please change this %lld to use %d.  It looks like mThreshold was silently
> changed in bug 1209654 to be a 64-bit integer without updating this format
> string, so I think changing the field access to
> int32_t(GET_FIELD(mThreshold)) is correct...
> 
> @@ +754,5 @@
> >                                    NetworkResultOptions& aResult)
> >  {
> >    char command[MAX_COMMAND_SIZE];
> >  
> > +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %ld", GET_FIELD(mThreshold));
> 
> Likewise here.

Albert, Ethan, your names are associated with bug 1177236, which added these particular commands.  What's the right resolution now that mThreshold is a 64-bit integer?
Flags: needinfo?(ettseng)
Flags: needinfo?(alberto.crespell)
(In reply to Nathan Froyd [:froydnj] from comment #25)
> Comment on attachment 8703581 [details] [diff] [review]
> Fixed all the errors in dom/
> 
> Review of attachment 8703581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the updates.  Still some minor things to fix, and one or two
> significant ones, which I'll ni? folks about.
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +19,5 @@
> >  #include "nsIDOMStorageManager.h"
> >  #include "mozilla/dom/DOMStorage.h"
> >  #include "mozilla/dom/StorageEvent.h"
> >  #include "mozilla/dom/StorageEventBinding.h"
> > +#include "mozilla/IntegerPrintfMacros.h"f
> 
> I guess this doesn't compile?
> 
> @@ +1787,5 @@
> >      nsAutoCString uri;
> >      if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) {
> >        tmp->mDoc->GetDocumentURI()->GetSpec(uri);
> >      }
> > +    snprintf_literal(name, "nsGlobalWindow # %" PRIu64" %s %s", tmp->mWindowID,
> 
> Please insert a space between PRIu64 and the subsequent string.
> 
> ::: dom/plugins/base/nsPluginHost.cpp
> @@ +3756,5 @@
> >      uint32_t l = sizeof(ContentLenHeader) + sizeof(CRLFCRLF) + 32;
> >      newBufferLen = dataLen + l;
> >      if (!(*outPostData = p = (char*)moz_xmalloc(newBufferLen)))
> >        return NS_ERROR_OUT_OF_MEMORY;
> > +    headersLen = snprintf(p, l,"%s: %ld%s", ContentLenHeader, dataLen, CRLFCRLF);
> 
> Please change the %ld here to use %u.
> 
> ::: dom/system/gonk/NetworkUtils.cpp
> @@ +713,5 @@
> >                              CommandCallback aCallback,
> >                              NetworkResultOptions& aResult)
> >  {
> >    char command[MAX_COMMAND_SIZE];
> > +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s %lld", GET_CHAR(mIfname), LLONG_MAX);
> 
> Please change the %lld to use PRId64, and the LLONG_MAX to INT64_MAX.
> 
> @@ +733,5 @@
> >                              CommandCallback aCallback,
> >                              NetworkResultOptions& aResult)
> >  {
> >    char command[MAX_COMMAND_SIZE];
> > +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld", GET_CHAR(mIfname), GET_FIELD(mThreshold));
> 
> Please change this %lld to use %d.  It looks like mThreshold was silently
> changed in bug 1209654 to be a 64-bit integer without updating this format
> string, so I think changing the field access to
> int32_t(GET_FIELD(mThreshold)) is correct...
> 
> @@ +754,5 @@
> >                                    NetworkResultOptions& aResult)
> >  {
> >    char command[MAX_COMMAND_SIZE];
> >  
> > +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %ld", GET_FIELD(mThreshold));
> 
> Likewise here.
> 
> @@ +2039,5 @@
> >      // Set default froute from system properties.
> >      char key[PROPERTY_KEY_MAX];
> >      char gateway[PROPERTY_KEY_MAX];
> >  
> > +    snprintf_literal(key, sizeof key - 1, "net.%s.gw", autoIfname.get());
> 
> This snprintf_literal call can't be correct, please fix it.
> 
> ::: dom/wifi/WifiUtils.cpp
> @@ +368,5 @@
> >    int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) {
> >      char command[COMMAND_SIZE];
> >      if (!strcmp(iface, "p2p0")) {
> >        // Commands for p2p0 interface don't need prefix
> > +      snprintf(command, COMMAND_SIZE, "%s", cmd);
> 
> This could be snprintf_literal, yes?

I don't think so. I think this could be |snprintf| itself. If this call is to be |snprintf_literal|, then all the remaining calls which are of the same format must also use |snprintf|, which would be against the comment 23.

> 
> @@ +373,3 @@
> >      }
> >      else {
> > +      snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd);
> 
> Same here.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #26)
> Albert, Ethan, your names are associated with bug 1177236, which added these
> particular commands.  What's the right resolution now that mThreshold is a
> 64-bit integer?

Yes. mThreshold is defined as long long integer now.
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkUtils.h#194

It was a long integer initially but caused an overflow problem, which was resolved by bug 1209654.
But the patch missed one place to modify, which was addressed by bug 1225549 (and one more missing point is addressed in this bug).

We should use %lld for mThreshold instead of %ld.
Flags: needinfo?(ettseng)
Flags: needinfo?(alberto.crespell)
(In reply to Anup Kumar [:Anupkumar] from comment #27)
> (In reply to Nathan Froyd [:froydnj] from comment #25)
> > ::: dom/wifi/WifiUtils.cpp
> > @@ +368,5 @@
> > >    int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) {
> > >      char command[COMMAND_SIZE];
> > >      if (!strcmp(iface, "p2p0")) {
> > >        // Commands for p2p0 interface don't need prefix
> > > +      snprintf(command, COMMAND_SIZE, "%s", cmd);
> > 
> > This could be snprintf_literal, yes?
> 
> I don't think so. I think this could be |snprintf| itself. If this call is
> to be |snprintf_literal|, then all the remaining calls which are of the same
> format must also use |snprintf|, which would be against the comment 23.

No.  The problem that comment 23 was pointing out was that:

  snprintf_literal(key, PROPERTY_KEY_MAX - 1, "net.%s.gw", ifname);

doesn't compile, and assuming you want to keep the PROPERTY_KEY_MAX - 1, it should be:

  snprintf(key, PROPERTY_KEY_MAX - 1, "net.%s.gw", ifname);

Alternatively, as comment 6 suggests, you could make this:

  snprintf_literal(key, "net.%s.gw", ifname);

So the review comment stands, and really meant that you could write:

  snprintf_literal(command, "%s", cmd);
Flags: needinfo?(nfroyd)
Attached patch fixed all the errors (obsolete) — Splinter Review
made the changes as requested in above comments.
Attachment #8703581 - Attachment is obsolete: true
Attachment #8704729 - Flags: review?(nfroyd)
Comment on attachment 8704729 [details] [diff] [review]
fixed all the errors

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +734,5 @@
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %d",
> +           GET_CHAR(mIfname), int32_t(GET_FIELD(mThreshold)));

mThreshold is long long. This should be %lld.

@@ +755,5 @@
>                                    NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
>  
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %d", int32_t(GET_FIELD(mThreshold)));

Same here.
Comment on attachment 8704729 [details] [diff] [review]
fixed all the errors

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

Thanks for fixing things up.  A few more comments, and please address Ethan's comments as well.

::: dom/base/nsGlobalWindow.cpp
@@ +1787,5 @@
>      nsAutoCString uri;
>      if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) {
>        tmp->mDoc->GetDocumentURI()->GetSpec(uri);
>      }
> +    snprintf_literal(name, "nsGlobalWindow # % " PRIu64" %s %s", tmp->mWindowID,

You missed the space insertion requested in comment 28.

::: dom/system/gonk/NetworkUtils.cpp
@@ +713,5 @@
>                              CommandCallback aCallback,
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s % PRId64", GET_CHAR(mIfname), INT64_MAX);

Please insert a space between PRId64 and the following '"' character.

::: dom/wifi/WifiUtils.cpp
@@ +368,5 @@
>    int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) {
>      char command[COMMAND_SIZE];
>      if (!strcmp(iface, "p2p0")) {
>        // Commands for p2p0 interface don't need prefix
> +      snprintf_literal(command, COMMAND_SIZE, "%s", cmd);

This snprintf_literal call doesn't compile.

@@ +373,3 @@
>      }
>      else {
> +      snprintf_literal(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd);

Neither does this one.
Attachment #8704729 - Flags: review?(nfroyd) → feedback+
Re-assigning the bug to another contributor since I am getting busy for a while. (don't want the bug to be staged)
Assignee: allamsetty.anup → sakshivaid95
Comment on attachment 8704729 [details] [diff] [review]
fixed all the errors

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

::: dom/media/webm/WebMDemuxer.cpp
@@ +121,1 @@
>    PR_vsnprintf(msg+strlen(msg), sizeof(msg)-strlen(msg), aFormat, args);

This doesn't compile since we are removing "prprf.h" from this file. I crossed checked it with the "mozilla/Snprintf.h" header file and there |vsnprintf()| is declared. 

So, I think this needs to be changed to |vsnprintf()|. 

What do you say froydnj?
Attachment #8704729 - Flags: feedback+ → feedback?(nfroyd)
(In reply to Anup Kumar [:Anupkumar] from comment #34)
> ::: dom/media/webm/WebMDemuxer.cpp
> @@ +121,1 @@
> >    PR_vsnprintf(msg+strlen(msg), sizeof(msg)-strlen(msg), aFormat, args);
> 
> This doesn't compile since we are removing "prprf.h" from this file. I
> crossed checked it with the "mozilla/Snprintf.h" header file and there
> |vsnprintf()| is declared. 
> 
> So, I think this needs to be changed to |vsnprintf()|. 
> 
> What do you say froydnj?

I think the right thing here is to leave the prprf.h include and leave the PR_vsnprintf call unchanged; according to the comment in Snprintf.h, we can't use vsnprintf on Windows due to an MSVC bug.
Attachment #8704729 - Flags: feedback?(nfroyd) → feedback+
Please review.
Attachment #8707523 - Flags: review?(nfroyd)
Attachment #8707523 - Flags: review?(allamsetty.anup)
Comment on attachment 8707523 [details] [diff] [review]
Made changes as per the previous comments. bug1197311.patch

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

Looks good to me.
Attachment #8707523 - Flags: review?(allamsetty.anup) → review+
Comment on attachment 8707523 [details] [diff] [review]
Made changes as per the previous comments. bug1197311.patch

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

Thanks for fixing things up!  I noticed a few changes that hadn't been made, as well as some things I missed earlier.

::: dom/base/nsGlobalWindow.cpp
@@ +1785,5 @@
>      nsAutoCString uri;
>      if (tmp->mDoc && tmp->mDoc->GetDocumentURI()) {
>        tmp->mDoc->GetDocumentURI()->GetSpec(uri);
>      }
> +    snprintf_literal(name, "nsGlobalWindow # % " PRIu64 " %s %s", tmp->mWindowID,

Sorry for missing this in the last review, but you don't want any space between the first '%' and the closing '"'; the way it's written now, snprintf won't work correctly because "% " isn't a format directive.

::: dom/system/gonk/NetworkUtils.cpp
@@ +734,5 @@
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setinterfacealert %s %lld",
> +           GET_CHAR(mIfname), int32_t(GET_FIELD(mThreshold)));

Per Ethan's comment 28, please remove the int32_t cast.

@@ +755,5 @@
>                                    NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
>  
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setglobalalert %lld", int32_t(GET_FIELD(mThreshold)));

Likewise here.

@@ +1181,5 @@
>  
>    for (uint32_t i = 0; i < length; i++) {
>      NS_ConvertUTF16toUTF8 autoDns(dnses[i]);
>  
> +    int ret = snprintf_literal(command + written, sizeof(command) - written, " %s", autoDns.get());

Sorry for not catching this earlier; this should be an snprintf call, not an snprintf_literal call.

::: dom/wifi/WifiUtils.cpp
@@ +368,5 @@
>    int32_t do_wifi_command(const char* iface, const char* cmd, char* buf, size_t* len) {
>      char command[COMMAND_SIZE];
>      if (!strcmp(iface, "p2p0")) {
>        // Commands for p2p0 interface don't need prefix
> +      snprintf(command, COMMAND_SIZE, "%s", cmd);

Per comment 29, please change this to use snprintf_literal instead.  Remember to remove the COMMAND_SIZE argument.

@@ +373,3 @@
>      }
>      else {
> +      snprintf(command, COMMAND_SIZE, "IFNAME=%s %s", iface, cmd);

Likewise this as well.
Attachment #8707523 - Flags: review?(nfroyd) → feedback+
Please review.
Attachment #8707523 - Attachment is obsolete: true
Attachment #8708246 - Flags: review?(nfroyd)
Attachment #8708246 - Flags: review?(allamsetty.anup)
Comment on attachment 8708246 [details] [diff] [review]
Made changes according to the comment 38

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

Looks good, thank you!
Attachment #8708246 - Flags: review?(nfroyd)
Attachment #8708246 - Flags: review?(allamsetty.anup)
Attachment #8708246 - Flags: review+
Thank you :)
Your most recent patch doesn't seem to apply cleanly to current m-c.  Could you please rebase it and upload a new version?  Thanks!
Flags: needinfo?(sakshivaid95)
Changes made as per comment 42.
Please review.
Attachment #8708246 - Attachment is obsolete: true
Flags: needinfo?(sakshivaid95)
Attachment #8713698 - Flags: review?(nfroyd)
Attachment #8713698 - Flags: review?(allamsetty.anup)
Comment on attachment 8713698 [details] [diff] [review]
Removed PR_snprintf calls in dom/

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

Thanks for updating this!  It applies just fine locally, but a try push reveals a few problems:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e9ad55c7bf&exclusion_profile=false

::: dom/system/gonk/NetworkUtils.cpp
@@ +378,2 @@
>    Property::Get(key, prop.gateway, "");
>    PR_snprintf(key, Property::KEY_MAX_LENGTH - 1, "net.%s.dns1", ifname);

Looks like you should fix this...

@@ +378,4 @@
>    Property::Get(key, prop.gateway, "");
>    PR_snprintf(key, Property::KEY_MAX_LENGTH - 1, "net.%s.dns1", ifname);
>    Property::Get(key, prop.dns1, "");
>    PR_snprintf(key, Property::KEY_MAX_LENGTH - 1, "net.%s.dns2", ifname);

...and this, too.  Not sure how those snuck through all this time!

@@ +714,5 @@
>                              CommandCallback aCallback,
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s % PRId64 ", GET_CHAR(mIfname), INT64_MAX);

This bit of "% PRId64 " doesn't look right.

@@ +1444,5 @@
>                            CommandCallback aCallback,
>                            NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "interface setmtu %s %d",

The compiler says this %d should be %ld instead.

@@ +1833,5 @@
>      for (uint32_t i = 0; i < length; i++) {
>        NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
>  
>        char dns_prop_key[Property::VALUE_MAX_LENGTH];
> +      snprintf(dns_prop_key, sizeof dns_prop_key, "net.dns%d", i+1);

There's a PR_snprintf call just below here that needs changing.

Also, this call can use snprintf_literal.
Attachment #8713698 - Flags: review?(nfroyd) → feedback+
Attached patch Made changes as per comment 44. (obsolete) — Splinter Review
Please review.
Attachment #8713698 - Attachment is obsolete: true
Attachment #8713698 - Flags: review?(allamsetty.anup)
Attachment #8714000 - Flags: review?(nfroyd)
Attachment #8714000 - Attachment is patch: true
Comment on attachment 8714000 [details] [diff] [review]
Made changes as per comment 44.

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

In addition to fixing the bits below, please fix the commit message so that it just reads:

Bug 1197311 - remove PR_snprintf calls in dom/ r=froydnj

There's no need to have it say the same thing three different times.

::: dom/system/gonk/NetworkUtils.cpp
@@ +714,5 @@
>                              CommandCallback aCallback,
>                              NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  snprintf(command, MAX_COMMAND_SIZE - 1, "bandwidth setiquota %s % PRId64", GET_CHAR(mIfname), INT64_MAX);

Did you forget to fix the PRId64 bit here?

@@ +1833,5 @@
>      for (uint32_t i = 0; i < length; i++) {
>        NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
>  
>        char dns_prop_key[Property::VALUE_MAX_LENGTH];
> +      snprintf_literal(dns_prop_key, sizeof dns_prop_key, "net.dns%d", i+1);

When I said "this call can use snprintf_literal", I meant to change the call and to change the arguments, since snprintf_literal and snprintf have different argument lists.  Please fix this.
Attachment #8714000 - Flags: review?(nfroyd) → feedback+
Attached patch Made changes as per comment 46. (obsolete) — Splinter Review
Please review.
Attachment #8714000 - Attachment is obsolete: true
Attachment #8715366 - Flags: review?(nfroyd)
Comment on attachment 8715366 [details] [diff] [review]
Made changes as per comment 46.

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

This patch looks good, thank you!

But it also needs an update to apply cleanly to current mozilla-central.  Could you do that, please?
Attachment #8715366 - Flags: review?(nfroyd) → review+
Sorry for the late reply. 
Please review.
Attachment #8715366 - Attachment is obsolete: true
Attachment #8719839 - Flags: review?(nfroyd)
Attachment #8704729 - Attachment is obsolete: true
Comment on attachment 8719839 [details] [diff] [review]
Made changes as per the previous comment.

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

Thanks for updating it.  A try run looks like it's in good shape.
Attachment #8719839 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/deadb414ee23
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.