Closed Bug 1060419 Opened 10 years ago Closed 7 years ago

Add MOZ_FORMAT_PRINTF to AppendPrintf and nsPrintfCString

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: botond, Assigned: tromey)

References

(Blocks 5 open bugs)

Details

Attachments

(36 files, 2 obsolete files)

58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
This is a follow-up to bug 965022.

See comments 31-33, and patches 4.5, 5, and 6, on that bug.
Blocks: 1060421
Component: Graphics: Layers → XPCOM
I found a number of other places that could use this treatment.
(And see also bug 553032, which is specifically about js/).

I've got about 16 patches including the ones referenced by this bug.
There's actually one more to be done -- PR_LogPrint, PR_snprintf,
etc, can be converted.  However, this yields a large number
of errors, so it may take a while.

It's also worth noting that not everything can be marked up that
we would like to mark up.  JS extends printf with "%hs" (at least);
and nsTextFormatter.h takes a char16_t* as a format argument, which
gcc doesn't recognize.

I'll upload and test my patches sometime soon.
How do you propose to solve the problem of the nspr printf functions treating %l differently from standard printf (and thus putting the attribute on them, which expects the standard printf interface, triggering bogus errors)?
(In reply to Botond Ballo [:botond] from comment #2)
> How do you propose to solve the problem of the nspr printf functions
> treating %l differently from standard printf (and thus putting the attribute
> on them, which expects the standard printf interface, triggering bogus
> errors)?

I have a variant of your patch from bug 965022 in my series; and
I'll need to fix all those PR_LogPrint, etc, errors mentioned in comment #1.
But I'm curious why you ask -- is there a problem you foresee?

It seems to me that non-standard treatment for %l is actively confusing,
so it's best to change nspr and update the tree to follow.  Maybe this
is overly ambitious and/or misguided somehow.  I'd like to know.

This does find real bugs among all the logging fixes and minor %whatever nits.
E.g., in your nsPrintfCString patch, there is a fix for a real bug in
APZCCallbackHandler.cpp.
(In reply to Tom Tromey :tromey from comment #3)
> I have a variant of your patch from bug 965022 in my series; and
> I'll need to fix all those PR_LogPrint, etc, errors mentioned in comment #1.
> But I'm curious why you ask -- is there a problem you foresee?

I got the impression, from e.g. [1], that making a backwards-incompatible change to nspr is difficult.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1035925#c5
(In reply to Botond Ballo [:botond] from comment #4)
> (In reply to Tom Tromey :tromey from comment #3)
> > I have a variant of your patch from bug 965022 in my series; and
> > I'll need to fix all those PR_LogPrint, etc, errors mentioned in comment #1.
> > But I'm curious why you ask -- is there a problem you foresee?
> 
> I got the impression, from e.g. [1], that making a backwards-incompatible
> change to nspr is difficult.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1035925#c5

Wow, I had no idea.  That seems very unfortunate to me, both for nspr
and for firefox.

If nspr is truly frozen like that then the only options available I see for
this bug are:

* Migrate all PR_* printf-like uses to use something more standard; or
* Write our own compiler plugin to check format strings.

The latter is maybe more practical.  It would let us solve the %hs issue
for JS as well.

A couple of the patches I have are salvageable, as they are wrappers
for vfprintf or the like.  Most are not though :(
Hah, and I just saw that JS_smprintf and friends are a copy-and-pasted
version of prprf.c, only with different changes and extensions.
See Also: → 1296122
I started working on this a bit again.

Bug 553032 made the JS printf standard-like, so my new plan is to move this to
mozglue, rename the functions, then go through the tree and update calls to PR_mumble
to call the new functions, fixing up the warnings along the way.

I'm happy to push my current series to github if anybody cares to see it.
I think it's going to take a while to finish.

FWIW it turns out that there are a number of misuses of *printf APIs, though so far
I think I've only seen them in logging code.
One fun thing I learned is that an enum class enumerator doesn't (AFAICT) implicitly convert to its
base type when passed through varargs.  So currently the series has many changes like:

-        LOG(("OBJLC [%p]: OpenChannel returned failure (%u)", this, rv));
+        LOG(("OBJLC [%p]: OpenChannel returned failure (%" PRIu32 ")",
+             this, static_cast<uint32_t>(rv)));

Gross!  But perhaps required.
Might as well admit it.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
I'm probably not going to fix up the weirdness in JavaScriptLogging.
I also wasn't planning to convert the various PR_fprintf calls.
The current series is already quite large...
I can file follow-up bugs if that's desirable.
I'm going to attach my current series for safekeeping and for anyone who is curious.
It builds locally (Linux); I'm also going to push it through try to see what else complains.
I'm planning to hold off requesting review until that is clean.

It will need at least a rebase regardless; and probably sheriff support to land.

I tried to make each patch as small as possible for simpler review.  Some of them are
quite large anyway; for example changing the logging API resulted in a lot of fallout.

Each point in the series builds locally -- that is, a patch may depend on older patches,
but not on newer ones.
Well that was hilarious.
webrtc has some -Wformat issues but IIUC we import this from upstream.
So I'm just going to disable the warning there.
This spot: https://dxr.mozilla.org/mozilla-central/rev/863c2b61bd27bb6099104933134d3be7c052551a/xpcom/io/nsPipe3.cpp#888
... really wants %td, but our printf-like doesn't implement this, and I think neither does MSVC.
So I'm going to cast that to int64_t.
Only ~90 patches to get it to pass try!  But I will rebase them into sanity.
libcubeb needed -Wno-format; see https://github.com/kinetiknz/cubeb/issues/203 for follow-up.
I'll file a follow-up bug here to re-enable -Wformat for libcubeb, after the next import is done.
Depends on: 1324987
I'm going to attach the patches for review now.
There is still some lingering problem on Windows; I haven't investigated that yet.
As mentioned previously, this is going to require a rebase anyhow, and also some
sheriff support while landing.  I think it's ready for review, though, as the overall
shape has been stable while I've been fixing the various minor problems.
Comment on attachment 8821232 [details]
Bug 1060419 - convert one nsPrintfCString to MOZ_LOG,

https://reviewboard.mozilla.org/r/100552/#review101328
Attachment #8821232 - Flags: review?(nfroyd) → review+
Comment on attachment 8819582 [details]
Bug 1060419 - make MediaEngineWebRTC.h use SprintfLiteral,

https://reviewboard.mozilla.org/r/99288/#review101354
Attachment #8819582 - Flags: review?(nfroyd) → review+
Comment on attachment 8819585 [details]
Bug 1060419 - make XPC_Log_print use VsprintfLiteral,

https://reviewboard.mozilla.org/r/99294/#review101358

I think that converting `%x` to `%p` will be relatively non-controversial.

::: js/xpconnect/src/XPCLog.cpp:60
(Diff revision 2)
>  {
>      va_list ap;
>      char line[LINE_LEN];
>  
>      va_start(ap, fmt);
> -    PR_vsnprintf(line, sizeof(line)-1, fmt, ap);
> +    SprintfLiteral(line, fmt, ap);

Surely this should be VsprintfLiteral?

::: js/xpconnect/src/XPCWrappedJS.cpp:700
(Diff revision 2)
>  
>  NS_IMETHODIMP
>  nsXPCWrappedJS::DebugDump(int16_t depth)
>  {
>  #ifdef DEBUG
> -    XPC_LOG_ALWAYS(("nsXPCWrappedJS @ %x with mRefCnt = %d", this, mRefCnt.get()));
> +    XPC_LOG_ALWAYS(("nsXPCWrappedJS @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get()));

This doesn't seem right: `mRefCnt` ought to be an integer-valued thing, not a pointer-sized integer value.  I think `%u` is the right format code here?

::: js/xpconnect/src/XPCWrappedJSClass.cpp:1416
(Diff revision 2)
>  NS_IMETHODIMP
>  nsXPCWrappedJSClass::DebugDump(int16_t depth)
>  {
>  #ifdef DEBUG
>      depth-- ;
> -    XPC_LOG_ALWAYS(("nsXPCWrappedJSClass @ %x with mRefCnt = %d", this, mRefCnt.get()));
> +    XPC_LOG_ALWAYS(("nsXPCWrappedJSClass @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get()));

Same issue with the refcount here.

::: js/xpconnect/src/XPCWrappedNative.cpp:2151
(Diff revision 2)
>  
>  NS_IMETHODIMP XPCWrappedNative::DebugDump(int16_t depth)
>  {
>  #ifdef DEBUG
>      depth-- ;
> -    XPC_LOG_ALWAYS(("XPCWrappedNative @ %x with mRefCnt = %d", this, mRefCnt.get()));
> +    XPC_LOG_ALWAYS(("XPCWrappedNative @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get()));

Same comment here on the refcount.

::: js/xpconnect/src/nsXPConnect.cpp:857
(Diff revision 2)
>  NS_IMETHODIMP
>  nsXPConnect::DebugDump(int16_t depth)
>  {
>  #ifdef DEBUG
>      depth-- ;
> -    XPC_LOG_ALWAYS(("nsXPConnect @ %x with mRefCnt = %d", this, mRefCnt.get()));
> +    XPC_LOG_ALWAYS(("nsXPConnect @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get()));

Here with the refcount as well.
Attachment #8819585 - Flags: review?(nfroyd) → review+
Comment on attachment 8819595 [details]
Bug 1060419 - make WebGLContextUtils.cpp use VsprintfLiteral,

https://reviewboard.mozilla.org/r/99314/#review101360

::: dom/canvas/WebGLContext.cpp:763
(Diff revision 2)
>                            " entry: ";
>              reason.info.Append(reason.key);
>  
>              out_failReasons->push_back(reason);
>  
> -            GenerateWarning(reason.info.BeginReading());
> +            GenerateWarning("%s", reason.info.BeginReading());

The only good thing about this is that I think `reason.info` might have been entirely under our control before...

::: dom/canvas/WebGLContextUtils.cpp:79
(Diff revision 2)
>          return;
>  
>      mAlreadyGeneratedWarnings++;
>  
>      char buf[1024];
> -    PR_vsnprintf(buf, 1024, fmt, ap);
> +    SprintfLiteral(buf, fmt, ap);

Surely this should be VsprintfLiteral?
Attachment #8819595 - Flags: review?(nfroyd) → review+
Comment on attachment 8819598 [details]
Bug 1060419 - make nsFrame.cpp use VsprintfLiteral,

https://reviewboard.mozilla.org/r/99320/#review101362
Attachment #8819598 - Flags: review?(nfroyd) → review+
Comment on attachment 8819585 [details]
Bug 1060419 - make XPC_Log_print use VsprintfLiteral,

https://reviewboard.mozilla.org/r/99294/#review101358

> Surely this should be VsprintfLiteral?

Yes, sorry about that.  Fixed locally.

> This doesn't seem right: `mRefCnt` ought to be an integer-valued thing, not a pointer-sized integer value.  I think `%u` is the right format code here?

I think mRefCnt is declared here:

https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#296

It is an nsrefcnt, which is defined as a typedef of MozRefCountType:

https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h#252

That in turn is a uintptr_t:

https://dxr.mozilla.org/mozilla-central/source/mfbt/RefCountType.h#22

At least, this was my logic when I made this change.  I'm not always confident that I'm finding the correct definitions in the tree.
Comment on attachment 8819595 [details]
Bug 1060419 - make WebGLContextUtils.cpp use VsprintfLiteral,

https://reviewboard.mozilla.org/r/99314/#review101360

> The only good thing about this is that I think `reason.info` might have been entirely under our control before...

There are handful of changes along these lines; I figured they are obviously safe (even more obviously so than before) and not very expensive (considering that the others I recall are in logging anyhow).

> Surely this should be VsprintfLiteral?

Yes, sorry about this.  Fixed locally.
Comment on attachment 8819580 [details]
Bug 1060419 - remove unneeded includes of prprf.h,

https://reviewboard.mozilla.org/r/99284/#review101374

::: netwerk/test/TestFileInput2.cpp:92
(Diff revision 2)
>      double mean = mTotalTime / mCount;
>      double variance = fabs(mSquares / mCount - mean * mean);
>      double stddev = sqrt(variance);
>      uint32_t imean = (uint32_t)mean;
>      uint32_t istddev = (uint32_t)stddev;
> -    return PR_smprintf("%d +/- %d ms", 
> +    return mozilla::Smprintf("%d +/- %d ms", 

Oops, this stuff wasn't meant to be in this patch.  I will put it into another patch in the next revision.
Comment on attachment 8819580 [details]
Bug 1060419 - remove unneeded includes of prprf.h,

https://reviewboard.mozilla.org/r/99284/#review101374

> Oops, this stuff wasn't meant to be in this patch.  I will put it into another patch in the next revision.

I did this.
Comment on attachment 8819579 [details]
Bug 1060419 - remove unused members from SprintfState,

https://reviewboard.mozilla.org/r/99282/#review102648
Attachment #8819579 - Flags: review?(nfroyd) → review+
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

https://reviewboard.mozilla.org/r/99278/#review102652

I think this is OK.  Some notes below.

::: js/src/jscntxt.cpp:336
(Diff revision 2)
>      JSErrorReport report;
>  
>      if (checkReportFlags(cx, &flags))
>          return true;
>  
> -    UniqueChars message(JS_vsmprintf(format, ap));
> +    UniqueChars message(mozilla::Vsprintf(format, ap));

I wonder if we shouldn't change the documentation on `Smprintf` and friends, since this sort of change is kind of abusing the knowledge that `mozilla::SmprintfFree` is really just `free` under the hood.

::: js/src/jscntxt.cpp:403
(Diff revision 2)
>      if (JSREPORT_IS_WARNING(report->flags) && !reportWarnings)
>          return false;
>  
>      char* prefix = nullptr;
>      if (report->filename)
> -        prefix = JS_smprintf("%s:", report->filename);
> +        prefix = mozilla::Smprintf("%s:", report->filename);

All these calls to `Smprintf` result in a memory leak, right?  I don't see any calls to `JS_smprintf_free` in this function, or any changes from that to `mozilla::SmprintfFree`.  Possibly worth auditing other callsites..

Pre-existing issue, so not critical to fix here, I suppose.

::: js/src/jsprf.h:66
(Diff revision 2)
>      MOZ_FORMAT_PRINTF(2, 3);
>  
>  /*
>  ** va_list forms of the above.
>  */
> -extern JS_PUBLIC_API(char*) JS_vsmprintf(const char* fmt, va_list ap);
> +extern MFBT_API char* Vsprintf(const char* fmt, va_list ap);

Shouldn't this be `Vsmprintf`, for continuity with `Smprintf`, above, and for consistency with the previous `JS_vsmprintf`?
Attachment #8819577 - Flags: review?(nfroyd) → review+
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

https://reviewboard.mozilla.org/r/99280/#review102660

I've tried to limit myself to things that matter because of the move, and not things that should be cleaned up in this grotty code.  Feel free to disagree.

::: mozglue/misc/Printf.h:7
(Diff revision 2)
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_Printf_h

Can you put a single-line comment before this include guard so the file displays nicely in DXR etc.?

::: mozglue/misc/Printf.h:11
(Diff revision 2)
> +
> +#ifndef mozilla_Printf_h
> +#define mozilla_Printf_h
> +
> +/*
> +** API for PR printf like routines. Supports the following formats

I don't know that it's necessary to do here, but it'd be nice to document *why* this header and associated baggage exist.  Possible reasons that come to mind:

- Bugs in existing `printf` implementations.
- Incomplete `printf` implementations.
- The ability to control where the output goes (malloc'ing returned strings and such), though this might be less important now that we could write a portable `asprintf`?
- Guarantees on behavior (e.g. no allocation under the hood).

It's possible, I suppose, that the need for this is just a relic of earlier times, and we could do everything with standard code and some wrappers?

You don't have to add that documentation for this bug; a follow-up would be fine.

::: mozglue/misc/Printf.h:26
(Diff revision 2)
> +**      %f - float
> +**      %g - float

Would be good to document that these are formatted with the platform's native `printf`, and so may vary from platform to platform.

::: mozglue/misc/Printf.h:59
(Diff revision 2)
> +** the malloc'd buffer. sprintf will append data to the end of last,
> +** growing it as necessary using realloc. If last is nullptr, SprintfAppend
> +** will allocate the initial string. The return value is the new value of
> +** last for subsequent calls, or nullptr if there is a malloc failure.
> +*/
> +extern MFBT_API char* SprintfAppend(char* last, const char* fmt, ...)

Feels like once we have `PrintfTarget` from other patches, the `*Append` variants can go away as a followup bug, perhaps?

::: mozglue/misc/Printf.h:65
(Diff revision 2)
> +    MOZ_FORMAT_PRINTF(2, 3);
> +
> +/*
> +** va_list forms of the above.
> +*/
> +extern MFBT_API char* Vsprintf(const char* fmt, va_list ap);

Comments on calling this `Vsmprintf` from a previous patch apply here too.

::: mozglue/misc/Printf.cpp:280
(Diff revision 2)
> +    SprintfLiteral(fout, fin, d);
> +
> +    return (*ss->stuff)(ss, fout, strlen(fout));

Can you file a followup so we can use the return value of `SprintfLiteral` here rather than calling `strlen`?

::: mozglue/misc/Printf.cpp:357
(Diff revision 2)
> +    if (!nas.growByUninitialized(number))
> +        return false;

WDYT about putting some sort of hard, smallish cap on `number`, so that we just return false here if `number` is too big because somebody passed in a screwy format string?  I guess the allocation would fail here for really large numbers, but for medium-size numbers that we don't really want to bother testing, a cap would give us some peace of mind.

I think C has a recommended limit of 20 numbered arguments, which seems reasonable, and is implemented elsewhere (e.g. `nsTextFormatter`).

::: mozglue/misc/Printf.cpp:865
(Diff revision 2)
> +    /* Copy data */
> +    while (len) {
> +        --len;
> +        *ss->cur++ = *sp++;
> +    }

I guess they didn't have `memcpy` in the good ol' days.
Attachment #8819578 - Flags: review?(nfroyd) → review+
Comment on attachment 8819601 [details]
Bug 1060419 - add PrintfTarget class to jsprf.h,

https://reviewboard.mozilla.org/r/99326/#review102680

I think this is OK.  I would have liked being able to do this without virtual functions, but everything else is less convenient and less C++-y.  We'll live, I guess.

::: mozglue/misc/Printf.h:81
(Diff revision 3)
> +    bool MFBT_API print(const char* format, ...) MOZ_FORMAT_PRINTF(2, 3);
> +
> +    /* The Vprintf-like interface.  */
> +    bool MFBT_API vprint(const char* format, va_list);
> +
> +    bool emit(const char* sp, size_t len) {

Ugh, there's no good way to make this "private, but reasonably callable from the guts of the printf code" is there?  I guess you'd have to make all the little helper functions, `cvt_f` and whatnot, private methods of `PrintfTarget`...actually, that's not a bad idea, given that this is basically what you have to do with C-like syntax already.  WDYT about changing that, so we don't have to expose this function?
Attachment #8819601 - Flags: review?(nfroyd) → review+
Comment on attachment 8819602 [details]
Bug 1060419 - make nsDebugImpl.cpp use PrintfTarget,

https://reviewboard.mozilla.org/r/99328/#review102682

::: xpcom/base/nsDebugImpl.cpp:346
(Diff revision 3)
> -    PRINT_TO_NONPID_BUFFER("line %d", aLine);
> +    nonPIDBuf.print("line %d", aLine);
>    }
> -#undef PRINT_TO_NONPID_BUFFER
>  
>    // Print "[PID]" or "[Desc PID]" at the beginning of the message.
> -#define PRINT_TO_BUFFER(...) PR_sxprintf(StuffFixedBuffer, &buf, __VA_ARGS__)
> +  buf.print("[");

The lone use of `PR_sxprintf` in the tree.  Good riddance.
Attachment #8819602 - Flags: review?(nfroyd) → review+
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

https://reviewboard.mozilla.org/r/99278/#review102652

> I wonder if we shouldn't change the documentation on `Smprintf` and friends, since this sort of change is kind of abusing the knowledge that `mozilla::SmprintfFree` is really just `free` under the hood.

Yeah.  This is why I was talking about removing JS_smprintf_free and replacing it with plain free -- the tree already isn't consistent about which variant of free it calls.

> All these calls to `Smprintf` result in a memory leak, right?  I don't see any calls to `JS_smprintf_free` in this function, or any changes from that to `mozilla::SmprintfFree`.  Possibly worth auditing other callsites..
> 
> Pre-existing issue, so not critical to fix here, I suppose.

This code uses JS_free rather than JS_smprintf_free.  E.g., https://dxr.mozilla.org/mozilla-central/rev/f13abb8ba9f366c9f32a3146245adf642528becd/js/src/jscntxt.cpp#465
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

https://reviewboard.mozilla.org/r/99280/#review102660

> Can you file a followup so we can use the return value of `SprintfLiteral` here rather than calling `strlen`?

This is small enough that I've just tacked on an additional patch to the series.
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

https://reviewboard.mozilla.org/r/99280/#review102660

> Feels like once we have `PrintfTarget` from other patches, the `*Append` variants can go away as a followup bug, perhaps?

There are ~30 calls to SprintfAppend, so I think keeping it around as a convenience function seems reasonable.  VsprintfAppend only has 2 callers, so I suppose it could go away.
Comment on attachment 8819601 [details]
Bug 1060419 - add PrintfTarget class to jsprf.h,

https://reviewboard.mozilla.org/r/99326/#review102680

> Ugh, there's no good way to make this "private, but reasonably callable from the guts of the printf code" is there?  I guess you'd have to make all the little helper functions, `cvt_f` and whatnot, private methods of `PrintfTarget`...actually, that's not a bad idea, given that this is basically what you have to do with C-like syntax already.  WDYT about changing that, so we don't have to expose this function?

Good idea, I will do that.
Comment on attachment 8819601 [details]
Bug 1060419 - add PrintfTarget class to jsprf.h,

https://reviewboard.mozilla.org/r/99326/#review102680

> Good idea, I will do that.

Ok, fixed locally.
Comment on attachment 8819585 [details]
Bug 1060419 - make XPC_Log_print use VsprintfLiteral,

https://reviewboard.mozilla.org/r/99294/#review101358

> I think mRefCnt is declared here:
> 
> https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#296
> 
> It is an nsrefcnt, which is defined as a typedef of MozRefCountType:
> 
> https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h#252
> 
> That in turn is a uintptr_t:
> 
> https://dxr.mozilla.org/mozilla-central/source/mfbt/RefCountType.h#22
> 
> At least, this was my logic when I made this change.  I'm not always confident that I'm finding the correct definitions in the tree.

We discussed this and agreed that this part is correct as-is.
Comment on attachment 8819580 [details]
Bug 1060419 - remove unneeded includes of prprf.h,

https://reviewboard.mozilla.org/r/99284/#review103062

Gonna assume this compiles appropriately.
Attachment #8819580 - Flags: review?(nfroyd) → review+
Comment on attachment 8819581 [details]
Bug 1060419 - make nsChromeRegistry.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99286/#review103064

It's really too bad that we can't printf to 16-bit characters straightaway here.  `nsTextFormatter` can, though...maybe after this lands this custom `printf` code and `nsTextFormatter` could be merged somehow.
Attachment #8819581 - Flags: review?(nfroyd) → review+
Comment on attachment 8819583 [details]
Bug 1060419 - make nsPluginsDirWin.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99290/#review103068

Using `%ld` for the 16-bit value returned from `HIWORD` is kind of dodgy, but thanks to C, I guess this works!
Attachment #8819583 - Flags: review?(nfroyd) → review+
Comment on attachment 8819584 [details]
Bug 1060419 - make ipc/chromium use Printf.h,

https://reviewboard.mozilla.org/r/99292/#review103070
Attachment #8819584 - Flags: review?(nfroyd) → review+
Comment on attachment 8819586 [details]
Bug 1060419 - make nsCookieService.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99296/#review103074
Attachment #8819586 - Flags: review?(nfroyd) → review+
Comment on attachment 8819587 [details]
Bug 1060419 - make nsHttpHandler.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99298/#review103078

::: netwerk/protocol/http/nsHttpHandler.cpp:869
(Diff revision 3)
>  #endif
> -        char *buf = PR_smprintf(format,
> +        char *buf = mozilla::Smprintf(format,
> -                                info.dwMajorVersion,
> +                               info.dwMajorVersion,
> -                                info.dwMinorVersion);
> +                               info.dwMinorVersion);
>          if (buf) {
>              mOscpu = buf;

Ah, C++, making code look like a memory hazard.
Attachment #8819587 - Flags: review?(nfroyd) → review+
Comment on attachment 8819588 [details]
Bug 1060419 - make mozStorageConnection.cpp and mozStorageStatement.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99300/#review103080
Attachment #8819588 - Flags: review?(nfroyd) → review+
Comment on attachment 8819589 [details]
Bug 1060419 - make nsExceptionHandler.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99302/#review103082

::: toolkit/crashreporter/nsExceptionHandler.cpp:3505
(Diff revision 3)
>  #if defined(XP_WIN)
> -  PR_Free(childCrashNotifyPipe);
> +  mozilla::SmprintfFree(childCrashNotifyPipe);

Hm, I guess this should be `#if defined(XP_MACOSX)` too, since we allocate `childCrashNotifyPipe` on OS X too?  File a followup for that?
Attachment #8819589 - Flags: review?(nfroyd) → review+
Comment on attachment 8819590 [details]
Bug 1060419 - make nsProfileLock.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99304/#review103084
Attachment #8819590 - Flags: review?(nfroyd) → review+
Comment on attachment 8819592 [details]
Bug 1060419 - make ManifestParser use Printf.h,

https://reviewboard.mozilla.org/r/99308/#review103094
Attachment #8819592 - Flags: review?(nfroyd) → review+
Comment on attachment 8819593 [details]
Bug 1060419 - make TestPipes.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99310/#review103096
Attachment #8819593 - Flags: review?(nfroyd) → review+
Comment on attachment 8819594 [details]
Bug 1060419 - make nsEnvironment.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99312/#review103098
Attachment #8819594 - Flags: review?(nfroyd) → review+
Comment on attachment 8819596 [details]
Bug 1060419 - make nsAppRunner.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99316/#review103100
Attachment #8819596 - Flags: review?(nfroyd) → review+
Comment on attachment 8819597 [details]
Bug 1060419 - make nsWebBrowserPersist.cpp use Printf.h,

https://reviewboard.mozilla.org/r/99318/#review103102
Attachment #8819597 - Flags: review?(nfroyd) → review+
Comment on attachment 8819599 [details]
Bug 1060419 - make xpcom/glue/nsDebug.h use Printf.h,

https://reviewboard.mozilla.org/r/99322/#review103104
Attachment #8819599 - Flags: review?(nfroyd) → review+
Comment on attachment 8819600 [details]
Bug 1060419 - make SharedMemoryBasic_mach.mm use Printf.h,

https://reviewboard.mozilla.org/r/99324/#review103106
Attachment #8819600 - Flags: review?(nfroyd) → review+
Comment on attachment 8819604 [details]
Bug 1060419 - fix comment in IntegerPrintfMacros.h to reflect Printf.h change,

https://reviewboard.mozilla.org/r/99332/#review103108
Attachment #8819604 - Flags: review?(nfroyd) → review+
Comment on attachment 8821615 [details]
Bug 1060419 - make netwerk/test/TestFileInput2.cpp use Printf.h;

https://reviewboard.mozilla.org/r/100848/#review103110
Attachment #8821615 - Flags: review?(nfroyd) → review+
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

https://reviewboard.mozilla.org/r/99278/#review102652

> Shouldn't this be `Vsmprintf`, for continuity with `Smprintf`, above, and for consistency with the previous `JS_vsmprintf`?

Good idea.  I've changed this locally.
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

https://reviewboard.mozilla.org/r/99280/#review102660

> I don't know that it's necessary to do here, but it'd be nice to document *why* this header and associated baggage exist.  Possible reasons that come to mind:
> 
> - Bugs in existing `printf` implementations.
> - Incomplete `printf` implementations.
> - The ability to control where the output goes (malloc'ing returned strings and such), though this might be less important now that we could write a portable `asprintf`?
> - Guarantees on behavior (e.g. no allocation under the hood).
> 
> It's possible, I suppose, that the need for this is just a relic of earlier times, and we could do everything with standard code and some wrappers?
> 
> You don't have to add that documentation for this bug; a follow-up would be fine.

I'm adding another patch for this comment and also the one before the include guard.
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

https://reviewboard.mozilla.org/r/99280/#review102660

> WDYT about putting some sort of hard, smallish cap on `number`, so that we just return false here if `number` is too big because somebody passed in a screwy format string?  I guess the allocation would fail here for really large numbers, but for medium-size numbers that we don't really want to bother testing, a cap would give us some peace of mind.
> 
> I think C has a recommended limit of 20 numbered arguments, which seems reasonable, and is implemented elsewhere (e.g. `nsTextFormatter`).

Sure, I used MOZ_RELEASE_ASSERT, hopefully that's correct.
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

https://reviewboard.mozilla.org/r/99278/#review102652

> Yeah.  This is why I was talking about removing JS_smprintf_free and replacing it with plain free -- the tree already isn't consistent about which variant of free it calls.

I was somewhat mistaken here.  It's true that the tree is not consistent about using JS_smprintf_free.  However, UniqueChars does use js_free:

https://dxr.mozilla.org/mozilla-central/rev/a14094edbad78fc1d16e8d4c57902537cf286fd1/js/public/Utility.h#496

... so while some code is violating the abstraction here, it isn't actually incorrect.

The various uses of values coming from JS_smprintf spread out enough that I am thinking it would be simplest to just continue using js_realloc and js_free in the new Printf.cpp.  I'm not fond of this, but I'm not completely confident that I could correctly write a patch to change all the users of these values.

I'd appreciate your input on this as well.
Comment on attachment 8819605 [details]
Bug 1060419 - convert mozglue/linker to use the mfbt-provided printf format defines,

https://reviewboard.mozilla.org/r/99334/#review103482
Attachment #8819605 - Flags: review?(nfroyd) → review+
(In reply to Tom Tromey :tromey from comment #153)
> The various uses of values coming from JS_smprintf spread out enough that I
> am thinking it would be simplest to just continue using js_realloc and
> js_free in the new Printf.cpp.  I'm not fond of this, but I'm not completely
> confident that I could correctly write a patch to change all the users of
> these values.

Won't this cause a circular dependency?  JS depends on mozglue, which in turns depends on JS to provide js_free.

(In reply to Tom Tromey :tromey from comment #152)
> > I think C has a recommended limit of 20 numbered arguments, which seems reasonable, and is implemented elsewhere (e.g. `nsTextFormatter`).
> 
> Sure, I used MOZ_RELEASE_ASSERT, hopefully that's correct.

That's perfect, thank you.
(In reply to Nathan Froyd [:froydnj] from comment #155)

> Won't this cause a circular dependency?  JS depends on mozglue, which in
> turns depends on JS to provide js_free.

Yeah :(
Another idea would be to give the various printf functions an allocator policy
template parameter.
(In reply to Tom Tromey :tromey from comment #156)

> Another idea would be to give the various printf functions an allocator
> policy
> template parameter.

My current idea is to do this and also move the PrintfTarget patch much earlier
in the series.
(In reply to Tom Tromey :tromey from comment #158)
> (In reply to Tom Tromey :tromey from comment #156)
> 
> > Another idea would be to give the various printf functions an allocator
> > policy
> > template parameter.
> 
> My current idea is to do this and also move the PrintfTarget patch much
> earlier
> in the series.

Does that mean a lot of the printf implementation is going to move into a header file?  That does not seem so great.

I do think there's going to be some amount of templating involved here or eventually (e.g. output character type, details about how you want floats formatted if we ever want to unify with nsTextFormatter, etc.), I'm just trying to get a sense of how everything would fit together.
(In reply to Nathan Froyd [:froydnj] from comment #159)

> Does that mean a lot of the printf implementation is going to move into a
> header file?

No, I think just SprintfState will move to a header and become a template class.
I'll clean it up a bit to be more normal (constructor etc).
In the end the constructor, SprintfState::append, and the (much shorter)
implementation of Smprintf et all will move into the header.
The bulk of the printf functionality will remain in Printf.cpp.

If you like I can whip up a patch showing what this looks like.
Comment on attachment 8819606 [details]
Bug 1060419 - make security/certverifier/ExtendedValidation.cpp use Sprintf.h,

https://reviewboard.mozilla.org/r/99336/#review104942
Attachment #8819606 - Flags: review?(nfroyd) → review+
Comment on attachment 8819603 [details]
Bug 1060419 - make AppendPrintf and nsPrintfCString use Printf.h,

https://reviewboard.mozilla.org/r/99330/#review104944

This looks good, thank you.

::: dom/base/DOMException.cpp:556
(Diff revision 3)
>  
>    static const char defaultMsg[] = "<no message>";
>    static const char defaultLocation[] = "<unknown>";
>    static const char defaultName[] = "<unknown>";
>    static const char format[] =
> -    "[Exception... \"%s\"  code: \"%d\" nsresult: \"0x%x (%s)\"  location: \"%s\"]";
> +    "[Exception... \"%s\"  code: \"%d\" nsresult: \"0x%" PRIu32 " (%s)\"  location: \"%s\"]";

This looks like it should be `PRIx32` for consistency with the previous code.

::: xpcom/string/string-template-def-char.h:15
(Diff revision 3)
>  #define nsTAString_IncompatibleCharT        nsAString
>  #define nsTString_CharT                     nsCString
>  #define nsTFixedString_CharT                nsFixedCString
>  #define nsTAutoString_CharT                 nsAutoCString
>  #define nsTSubstring_CharT                  nsACString
> +#define PrintfAppend_CharT                  PrintfAppend_nsACString

You have modified the string code.  Level up!
Attachment #8819603 - Flags: review?(nfroyd) → review+
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

https://reviewboard.mozilla.org/r/99278/#review102652

> I was somewhat mistaken here.  It's true that the tree is not consistent about using JS_smprintf_free.  However, UniqueChars does use js_free:
> 
> https://dxr.mozilla.org/mozilla-central/rev/a14094edbad78fc1d16e8d4c57902537cf286fd1/js/public/Utility.h#496
> 
> ... so while some code is violating the abstraction here, it isn't actually incorrect.
> 
> The various uses of values coming from JS_smprintf spread out enough that I am thinking it would be simplest to just continue using js_realloc and js_free in the new Printf.cpp.  I'm not fond of this, but I'm not completely confident that I could correctly write a patch to change all the users of these values.
> 
> I'd appreciate your input on this as well.

The new series templatizes the functions as discussed.  It's also much simpler since I've left the JS_* variants as wrapper functions.

> This code uses JS_free rather than JS_smprintf_free.  E.g., https://dxr.mozilla.org/mozilla-central/rev/f13abb8ba9f366c9f32a3146245adf642528becd/js/src/jscntxt.cpp#465

Dropping this one since I think the code is ok.
Comment on attachment 8819603 [details]
Bug 1060419 - make AppendPrintf and nsPrintfCString use Printf.h,

https://reviewboard.mozilla.org/r/99330/#review104944

> This looks like it should be `PRIx32` for consistency with the previous code.

Thanks, I've fixed this locally.
I've rearranged the patch series to deal with the free/js_free problem.
The new series differs from the old:

* I kept jsprf.h and the various JS_*printf functions.  This greatly simplifies a couple
  of patches, since there's no mass renaming and no need to change all the #includes.
  This is done so that the JS implementation can continue using js_malloc and js_free.

* I've added an allocator policy to Smprintf et al.  I stopped short of making these deal
  exclusively with unique pointers.

I've made some other smaller changes, like marking a couple of "append" methods "override".

I'll request re-review on the patches that have changed substantially.

I'm still tweaking the series to build properly :)
Once that is done the next steps are a clean try run and then, ugh, rebasing and fixing up
the fallout.
Today I discovered another fork of the NSPR printf code:

https://dxr.mozilla.org/mozilla-central/rev/de67fccc4c64a49f261aea29141357b94c7b3b9c/xpcom/glue/nsTextFormatter.cpp#10

... hilariously discovered because making SprintfState more visible caused a name clash.

This one actually has users of its char16_t interface, so it's harder to fix.
I will most likely just rename the offending struct and file a followup bug for this.
Err, I should say, it *only* has a char16_t interface.
GCC at least won't do any checking here.
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

https://reviewboard.mozilla.org/r/99278/#review106028
Attachment #8819577 - Flags: review?(ttromey)
Comment on attachment 8819601 [details]
Bug 1060419 - add PrintfTarget class to jsprf.h,

https://reviewboard.mozilla.org/r/99326/#review106030
Attachment #8819601 - Flags: review?(ttromey)
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

https://reviewboard.mozilla.org/r/99280/#review106034
Attachment #8819578 - Flags: review?(ttromey)
Still trying to figure out how to re-r? some of those patches...
(In reply to Tom Tromey :tromey from comment #207)
> Still trying to figure out how to re-r? some of those patches...

Bug 1195661 is relevant (short answer: you can't, except perhaps by manually setting the flags in buzilla :/)
Comment on attachment 8827592 [details]
Bug 1060419 - use result of SprintfLiteral in Printf.cpp;

https://reviewboard.mozilla.org/r/105222/#review106064

The patch is OK, but are you landing this bit on its own or in combination with a previous patch that touches this code?  The commit message for this patch "use SprintfLiteral in Printf.cpp" isn't really accurate, and if it's going to land as its own patch, it would be nice if it was changed.
Attachment #8827592 - Flags: review?(nfroyd) → review+
Comment on attachment 8827590 [details]
Bug 1060419 - introduce mozilla::Smprintf and friends;

https://reviewboard.mozilla.org/r/105218/#review106066

Is this getting squashed somewhere, or standing on its own?

::: js/src/jsprf.h:45
(Diff revision 1)
>  /*
>  ** sprintf into a malloc'd buffer. Return a pointer to the malloc'd
> -** buffer on success, nullptr on failure. Call "JS_smprintf_free" to release
> +** buffer on success, nullptr on failure. Call "SmprintfFree" to release
>  ** the memory returned.
>  */
> -extern JS_PUBLIC_API(char*) JS_smprintf(const char* fmt, ...)
> +extern MFBT_API char* Smprintf(const char* fmt, ...)

These are `MFBT_API` partly because it makes them easy to cut-and-paste later and partly because `JS_PUBLIC_API` on symbols in `mozilla::` doesn't make a lot of sense, correct?
Attachment #8827590 - Flags: review?(nfroyd) → review+
Comment on attachment 8827593 [details]
Bug 1060419 - update comments in Printf.h;

https://reviewboard.mozilla.org/r/105224/#review106068

Thank you for these!
Attachment #8827593 - Flags: review?(nfroyd) → review+
Attachment #8819577 - Flags: review?(ttromey)
Attachment #8819577 - Flags: review?(nfroyd)
Attachment #8819577 - Flags: review+
Attachment #8819601 - Flags: review?(ttromey)
Attachment #8819601 - Flags: review?(nfroyd)
Attachment #8819601 - Flags: review+
Attachment #8819578 - Flags: review?(ttromey)
Attachment #8819578 - Flags: review?(nfroyd)
Attachment #8819578 - Flags: review+
(In reply to Nathan Froyd [:froydnj] from comment #209)
> Comment on attachment 8827592 [details]
> Bug 1060419 - use SprintfLiteral in Printf.cpp;
> 
> https://reviewboard.mozilla.org/r/105222/#review106064
> 
> The patch is OK, but are you landing this bit on its own or in combination
> with a previous patch that touches this code?

On its own.  I don't propose to squash any patches in this series but land them
as-is.

> The commit message for this
> patch "use SprintfLiteral in Printf.cpp" isn't really accurate, and if it's
> going to land as its own patch, it would be nice if it was changed.

I've changed it to "use result of SprintfLiteral in Printf.cpp", per our discussion on irc.

> These are `MFBT_API` partly because it makes them easy to cut-and-paste later and partly because `JS_PUBLIC_API` on symbols in `mozilla::` doesn't make a lot of sense, correct?

Yeah.  Though a later patch strips MFBT_API in some spots, when the functions are converted
to templates, because leaving the marker there in that case broke the Windows build.
Blocks: 1331760
Blocks: 1331762
Blocks: 1331766
TIL that there are multiple spots in-tree using MOZ_LOG with the %S format spec.
This is implemented in NSPR (for Windows only) but not in the JS fork, which was
the starting point for this series.

Some of these calls incorrectly assume that wchar_t is the same as char16_t.
So far this seems limited to Windows, though, so maybe it's ok(-ish).

But there are also bugs like:

https://dxr.mozilla.org/mozilla-central/rev/80eac484366ad881c6a10bf81e8d9b8f7a676c75/toolkit/components/filewatcher/NativeFileWatcherWin.cpp#515

I imagine this would cause a crash.

"%ls" is also in use.
A real try run points out some crashers and other badness, but not really as awful as I
might have been anticipating.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3e69a70c5b0784896fc325bbb1c18bd0dc73e91

Also, given bug 1331349, I wonder if this bug should depend on bug 1330608; that is, wait until
mingw64 cross builds are in automation, to avoid breaking those.
(In reply to Tom Tromey :tromey from comment #214)
> Also, given bug 1331349, I wonder if this bug should depend on bug 1330608;
> that is, wait until
> mingw64 cross builds are in automation, to avoid breaking those.

I don't know how much time bug 1330608 is going to take, and if it winds up being O(weeks), that's just going to make landing this more painful.  I am inclined to put the burden of fixing mingw64 format string problems on the mingw64 folks at the moment, but I am open to arguments otherwise.  (Folks who maintain ports out of tree are used to the occasional breakage.)
Also, we probably have a *better* chance at fixing bug 1331349 properly once we get everything going through one set of format codes?
I've copied over the NSPR implementation of %S and generalized it to handle %ls.
NSPR uses WideCharToMultiByte, so this is all #if defined(XP_WIN).
I thought perhaps we could use wcstombs_s (to avoid #if hell), but it's surprisingly
hard to find out if these two are actually equivalent.
Today I searched for "char.*, \.\.\." and found a reasonably large number of other printf wrappers
in the tree.  Filtering out the ones that seemed to be in imported code, and also probably missing
a few due to the volume, I came up with a future to-do list:

* TODO browser/app/nsBrowserApp.cpp:Output could be marked
  same with b2g/app/nsBrowserApp
* TODO ./dom/plugins/base/android/android_npapi.h
* TODO dom/media/gmp/widevine-adapter/WidevineUtils.cpp
* TODO gecko/widget/cocoa/nsCocoaDebugUtils.h
* TODO widget/windows/WinUtils.cpp
* TODO tools/profiler/public/GeckoProfilerImpl.h
* TODO memory/replace/dmd/DMD.h
* TODO toolkit/mozapps/update/common/updatelogging.h
* TODO layout/painting/nsCSSRenderingBorders.h
* TODO ipc/hal/DaemonSocketPDUHelpers.h
* TODO mozglue/build/WindowsDllBlocklist.cpp
* TODO logging::MsgBegin in accessible/base/Logging.cpp
* TODO js/xpconnect/src/XPCDebug.cpp
* TODO js/src/vm/Printer.cpp could be more efficient by not allocating
* TODO js/src/devtools/vprof/vprof.cpp

There are a couple of other follow-up bugs to file as well.

And, I found out that the WebGLContextUtils patch was incomplete -- there were some wrappers that
weren't marked -- and this update found a few more incorrect printfs.
Comment on attachment 8827591 [details]
Bug 1060419 - templatize SprintfState and move to jsprf.h;

https://reviewboard.mozilla.org/r/105220/#review106062

Sorry, I reviewed this the other day and apparently forgot to hit "Publish!"  One small note below.

::: js/src/jsprf.h:87
(Diff revision 1)
> +template<typename AllocPolicy>
> +class MOZ_STACK_CLASS SprintfState : public mozilla::PrintfTarget

I think the usual style when templating over `AllocPolicy` is to have the class privately inherit from the provided `AllocPolicy`.  This style takes advantage of the empty base class optimization that (I think) all of our compilers employ.  Maybe it has to be protected inheritance for this class, though, if other states are intended to inherit from this class?
Attachment #8827591 - Flags: review?(nfroyd) → review+
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

Manually marking this r+, since I think it's already marked in mozreview.
Attachment #8819577 - Flags: review?(nfroyd) → review+
Comment on attachment 8819601 [details]
Bug 1060419 - add PrintfTarget class to jsprf.h,

This is great, thank you!
Attachment #8819601 - Flags: review?(nfroyd) → review+
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

I'm confused here: the interdiff says that we're now including jsprf.h instead of mozilla/Printf.h?  And that, I assume, declares things in the mozilla:: namespace?  That does not sit well with me, in addition to being contrary to what the commit message says.
(In reply to Nathan Froyd [:froydnj] from comment #222)
> Comment on attachment 8819578 [details]
> Bug 1060419 - move Smprintf et al to mozglue,
> 
> I'm confused here: the interdiff says that we're now including jsprf.h
> instead of mozilla/Printf.h?  And that, I assume, declares things in the
> mozilla:: namespace?  That does not sit well with me, in addition to being
> contrary to what the commit message says.

The interdiff is confusing in this case.

The original patch series completely removed jsprf.{h,cpp} and updated all #includes
to mozilla/Printf.h.

However, then we discovered the alloc policy problem, necessitating a big change.
This new patch series keeps jsprf.{h,cpp}, but now these files just hold specializations
of the generic facilities provided by mozglue.

Since jsprf.h still exists, there was no need to change all those #includes.  So, I backed
out that part of the original series.  This backout is what is showing up in the interdiff.

This series does temporarily declare some mozilla:: things in jsprf.h.  I did it this way to
minimize churn in the patches.  In particular, mozilla::Smprintf and some other things are
introduced in patch #3, but then they are all moved to mozglue in patch #7 (this patch).

I think the commit message here is correct...?  Smprintf and various other things from jsprf.h
are moved to mozglue here.  A few things are left behind, namely the existing JS_* functions.

I guess one way I could have done this differently would have been to introduce new things directly
in mozglue and only then change jsprf.h.  I can make this change if you'd like.
Comment on attachment 8819578 [details]
Bug 1060419 - move Smprintf et al to mozglue,

OK.  I trust that everything will get to the right place eventually. :)
Attachment #8819578 - Flags: review?(nfroyd) → review+
Comment on attachment 8819591 [details]
Bug 1060419 - make log_print use Printf.h,

https://reviewboard.mozilla.org/r/99306/#review103090

Lots of comments, but this is generally good stuff.

::: dom/base/nsObjectLoadingContent.cpp:2430
(Diff revision 3)
>      break;
>      case eType_Loading:
>        // If our type remains Loading, we need a channel to proceed
>        rv = OpenChannel();
>        if (NS_FAILED(rv)) {
> -        LOG(("OBJLC [%p]: OpenChannel returned failure (%u)", this, rv));
> +        LOG(("OBJLC [%p]: OpenChannel returned failure (%" PRIx32 ")",

Should this be `PRIu32` for consistency with the previous `%u` format?  I think `PRIx32` is probably more readable for `nsresult`, but since we're going for minimum behavior changes here...

::: dom/base/nsObjectLoadingContent.cpp:2708
(Diff revision 3)
>  nsObjectLoadingContent::NotifyStateChanged(ObjectType aOldType,
>                                             EventStates aOldState,
>                                             bool aSync,
>                                             bool aNotify)
>  {
> -  LOG(("OBJLC [%p]: Notifying about state change: (%u, %llx) -> (%u, %llx)"
> +  LOG(("OBJLC [%p]: Notifying about state change: (%u, %" PRIu64 ") -> (%u, %" PRIu64 ")"

These should be `PRIx64` for consistency with the old code, right?

::: dom/events/IMEStateManager.cpp:929
(Diff revision 3)
>  {
>    MOZ_LOG(sISMLog, LogLevel::Info,
>      ("SetInputContextForChildProcess(aTabParent=0x%p, "
>       "aInputContext={ mIMEState={ mEnabled=%s, mOpen=%s }, "
>       "mHTMLInputType=\"%s\", mHTMLInputInputmode=\"%s\", mActionHint=\"%s\" }, "
> -     "aAction={ mCause=%s, mAction=%s }, aTabParent=0x%p), sPresContext=0x%p, "
> +     "aAction={ mCause=%s, mAction=%s }), sPresContext=0x%p, "

I assume this got deleted due to format checking?  Yay for finding real bugs!

::: dom/html/TextTrackManager.cpp:270
(Diff revision 4)
>    nsTArray<RefPtr<TextTrackCue> > activeCues;
>    mTextTracks->GetShowingCues(activeCues);
>  
>    if (activeCues.Length() > 0) {
>      WEBVTT_LOG("UpdateCueDisplay ProcessCues");
> -    WEBVTT_LOGV("UpdateCueDisplay activeCues.Length() %d",activeCues.Length());
> +    WEBVTT_LOGV("UpdateCueDisplay activeCues.Length() %" PRIuSIZE,activeCues.Length());

Nit: can you add a space after `PRIuSIZE` here to make things a little more readable?

::: dom/media/MediaDecoderStateMachine.cpp:2573
(Diff revision 4)
>    bool skipToNextKeyFrame = NeedToSkipToNextKeyframe();
>  
>    media::TimeUnit currentTime = media::TimeUnit::FromMicroseconds(GetMediaTime());
>  
> -  SAMPLE_LOG("Queueing video task - queued=%i, decoder-queued=%o, skip=%i, time=%lld",
> +  SAMPLE_LOG("Queueing video task - queued=%" PRIuSIZE
> +             ", decoder-queued=%" PRIoSIZE ", skip=%i, time=%" PRId64,

I am slightly skeptical that somebody actually *wanted* octal notation here, but OK...

What do you think about changing this to `PRIdSIZE`, on the assumption that somebody fat-fingered this?  I guess we could ni? somebody familiar with this code...

::: dom/media/eme/MediaKeySession.cpp:163
(Diff revision 4)
>      for (const CDMCaps::KeyStatus& status : keyStatuses) {
>        message.Append(nsPrintfCString(" (%s,%s)", ToBase64(status.mId).get(),
>          MediaKeyStatusValues::strings[static_cast<IntegerType>(status.mStatus)].value));
>      }
>      message.Append(" }");
> -    EME_LOG(message.get());
> +    EME_LOG("%s", message.get());

Maybe comment: "// Use %s so we aren't exposing random strings to printf interpolation."?

::: dom/media/encoder/MediaEncoder.cpp:194
(Diff revision 4)
>      LOG(LogLevel::Error, ("Can not find any encoder to record this media stream"));
>      return nullptr;
>    }
>    LOG(LogLevel::Debug, ("Create encoder result:a[%d] v[%d] w[%d] mimeType = %s.",
>                        audioEncoder != nullptr, videoEncoder != nullptr,
> -                      writer != nullptr, mimeType.get()));
> +                      writer != nullptr, NS_ConvertUTF16toUTF8(mimeType).get()));

That's a nice catch.

::: dom/media/platforms/apple/AppleVTDecoder.cpp:405
(Diff revision 4)
>  nsresult
>  AppleVTDecoder::WaitForAsynchronousFrames()
>  {
>    OSStatus rv = VTDecompressionSessionWaitForAsynchronousFrames(mSession);
>    if (rv != noErr) {
> -    LOG("AppleVTDecoder: Error %d waiting for asynchronous frames", rv);
> +    LOG("AppleVTDecoder: Error %ul waiting for asynchronous frames", static_cast<int>(rv));

`%u` is correct for an `int` argument?  That seems weird.  And why the extra `l` after `%u`?

::: dom/media/platforms/omx/OmxDataDecoder.cpp:435
(Diff revision 4)
> -  LOG("NotifyError %d (%d) at %s", aOmxError, aError.Code(), aLine);
> +  LOG("NotifyError %d (%" PRId32 ") at %s", static_cast<int>(aOmxError),
> +      static_cast<uint32_t>(aError.Code()), aLine);

We have `PRId32` for a `uint32_t` argument?  That seems incorrect.

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:149
(Diff revision 4)
>  void
>  MediaEngineCameraVideoSource::LogConstraints(
>      const NormalizedConstraintSet& aConstraints)
>  {
>    auto& c = aConstraints;
> -  LOG(((c.mWidth.mIdeal.isSome()?
> +  if (c.mWidth.mIdeal.isSome()) {

This seems unfortunate, but I guess that's what we have to live with if we have format string checking.

::: dom/presentation/PresentationService.cpp:524
(Diff revision 4)
> -  PRES_DEBUG("handle new session:url[%d], id[%s]\n",
> +  PRES_DEBUG("handle new session:url[%s], id[%s]\n",
>               NS_ConvertUTF16toUTF8(url).get(),

Nice catch.

::: dom/presentation/PresentationService.cpp:605
(Diff revision 4)
> -  PRES_DEBUG("handle termination:id[%s], receiver[%d]\n", __func__,
> -             sessionId.get(), isFromReceiver);
> +  PRES_DEBUG("%s:handle termination:id[%s], receiver[%d]\n", __func__,
> +             NS_ConvertUTF16toUTF8(sessionId).get(), isFromReceiver);

Also a nice catch.

::: dom/security/nsCSPContext.cpp:1015
(Diff revision 4)
>      // not return an error since it's really ok if reports don't go out, but
>      // it's good to log the error locally.
>  
>      if (NS_FAILED(rv)) {
>        const char16_t* params[] = { reportURIs[r].get() };
> -      CSPCONTEXTLOG(("AsyncOpen failed for report URI %s", params[0]));
> +      CSPCONTEXTLOG(("AsyncOpen failed for report URI %s", NS_ConvertUTF16toUTF8(params[0]).get()));

Another nice catch.

::: dom/security/nsCSPContext.cpp:1376
(Diff revision 4)
>        // continue the loop checking for an enforcement policy.
>        nsAutoString policy;
>        mPolicies[i]->toString(policy);
>  
>        CSPCONTEXTLOG(("nsCSPContext::GetCSPSandboxFlags, report only policy, ignoring sandbox in: %s",
> -                    policy.get()));
> +                     NS_ConvertUTF16toUTF8(policy).get()));

...and another.

::: dom/security/nsCSPUtils.cpp
(Diff revision 4)
>  nsCSPKeywordSrc::allows(enum CSPKeyword aKeyword, const nsAString& aHashOrNonce,
>                          bool aParserCreated) const
>  {
>    CSPUTILSLOG(("nsCSPKeywordSrc::allows, aKeyWord: %s, aHashOrNonce: %s, mInvalidated: %s",
>                CSP_EnumToKeyword(aKeyword),
> -              CSP_EnumToKeyword(mKeyword),

...and another.

::: gfx/thebes/gfxMacPlatformFontList.mm
(Diff revision 4)
> -                              NS_ConvertUTF16toUTF8(familyName).get(),
> -                              NS_ConvertUTF16toUTF8(key).get()));

Heh!

::: hal/fallback/FallbackThreadPriority.cpp:16
(Diff revision 4)
> -  HAL_LOG("FallbackThreadPriority - SetCurrentThreadPriority(%d)\n",
> +  HAL_LOG("FallbackThreadPriority - SetCurrentThreadPriority(%s)\n",
>            ThreadPriorityToString(aPriority));

Whoops.

::: hal/fallback/FallbackThreadPriority.cpp:24
(Diff revision 4)
> -  HAL_LOG("FallbackThreadPriority - SetThreadPriority(%d, %d)\n",
> +  HAL_LOG("FallbackThreadPriority - SetThreadPriority(%d, %s)\n",
>            aThreadId, ThreadPriorityToString(aPriority));

Whoops again.

::: netwerk/cache/nsCacheService.cpp:2193
(Diff revision 4)
>      if ((policy == nsICache::STORE_ANYWHERE) || (policy == nsICache::STORE_IN_MEMORY)) {
>          // If there is no memory device, then there is nothing to search...
>          if (mMemoryDevice) {
>              entry = mMemoryDevice->FindEntry(key, collision);
>              CACHE_LOG_DEBUG(("Searching mMemoryDevice for key %s found: 0x%p, "
> -                             "collision: %d\n", key->get(), entry, collision));
> +                             "collision: %d\n", key->get(), entry, *collision));

Nice catch.

::: netwerk/cache/nsDiskCacheDevice.cpp:478
(Diff revision 4)
>      } else if (binding && binding->mDeactivateEvent) {
>          binding->mDeactivateEvent->CancelEvent();
>          binding->mDeactivateEvent = nullptr;
>          CACHE_LOG_DEBUG(("CACHE: reusing deactivated entry %p " \
>                           "req-key=%s  entry-key=%s\n",
> -                         binding->mCacheEntry, key, binding->mCacheEntry->Key()));
> +                         binding->mCacheEntry, key->get(),

Nice catch.

::: netwerk/cache/nsDiskCacheStreams.cpp:119
(Diff revision 4)
>      *bytesRead = 0;
>  
>      if (mClosed) {
>          CACHE_LOG_DEBUG(("CACHE: nsDiskCacheInputStream::Read "
>                           "[stream=%p] stream was closed",
> -                         this, buffer, count));
> +                         this));

So great to have unused arguments caught.

::: netwerk/cache2/CacheFileIOManager.cpp:3504
(Diff revision 4)
>  CacheFileIOManager::InitIndexEntry(CacheFileHandle *aHandle,
>                                     OriginAttrsHash  aOriginAttrsHash,
>                                     bool             aAnonymous,
>                                     bool             aPinning)
>  {
> -  LOG(("CacheFileIOManager::InitIndexEntry() [handle=%p, originAttrsHash=%llx, "
> +  LOG(("CacheFileIOManager::InitIndexEntry() [handle=%p, originAttrsHash=%" PRIu64 ", "

Do we want `PRIx64` here for consistency with the original code?

::: netwerk/cache2/CacheIndex.h:258
(Diff revision 4)
>    }
>  
>    void Log() const {
>      LOG(("CacheIndexEntry::Log() [this=%p, hash=%08x%08x%08x%08x%08x, fresh=%u,"
>           " initialized=%u, removed=%u, dirty=%u, anonymous=%u, "
> -         "originAttrsHash=%llx, frecency=%u, expirationTime=%u, size=%u]",
> +         "originAttrsHash=%" PRIu64 ", frecency=%u, expirationTime=%u, size=%u]",

I think this wants to be `PRIx64` for consistency with the original code.

::: netwerk/cache2/CacheIndex.cpp:717
(Diff revision 4)
>                        OriginAttrsHash      aOriginAttrsHash,
>                        bool                 aAnonymous,
>                        bool                 aPinned)
>  {
>    LOG(("CacheIndex::InitEntry() [hash=%08x%08x%08x%08x%08x, "
> -       "originAttrsHash=%llx, anonymous=%d, pinned=%d]", LOGSHA1(aHash),
> +       "originAttrsHash=%" PRIu64 ", anonymous=%d, pinned=%d]", LOGSHA1(aHash),

I think this wants to be `PRIx64` for consistency with the original code.

::: netwerk/cache2/CacheIndex.cpp:1505
(Diff revision 4)
> -         "%08x%08x%08x%08x, expected values: originAttrsHash=%llx, "
> -         "anonymous=%d; actual values: originAttrsHash=%llx, anonymous=%d]",
> +         "%08x%08x%08x%08x, expected values: originAttrsHash=%" PRIu64 ", "
> +         "anonymous=%d; actual values: originAttrsHash=%" PRIu64 ", anonymous=%d]",

I think these want to be `PRIx64` for consistency with the original code.

::: netwerk/protocol/http/Http2Session.cpp:121
(Diff revision 4)
>    MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>  
>    static uint64_t sSerial;
>    mSerial = ++sSerial;
>  
> -  LOG3(("Http2Session::Http2Session %p serial=0x%X\n", this, mSerial));
> +  LOG3(("Http2Session::Http2Session %p serial=0x%" PRIx64 "\n", this, mSerial));

Maybe `PRIX64` for consistency with the old code?

::: rdf/base/nsRDFService.cpp:1389
(Diff revision 4)
>      entry->mLiteral = aLiteral;
>      entry->mKey = value;
>  
>      MOZ_LOG(gLog, LogLevel::Debug,
>             ("rdfserv   register-literal [%p] %s",
> -            aLiteral, (const char16_t*) value));
> +            aLiteral, NS_ConvertUTF16toUTF8(value).get()));

Nice.

::: rdf/base/nsRDFService.cpp:1409
(Diff revision 4)
>  
>      // N.B. that we _don't_ release the literal: we only held a weak
>      // reference to it in the hashtable.
>      MOZ_LOG(gLog, LogLevel::Debug,
>             ("rdfserv unregister-literal [%p] %s",
> -            aLiteral, (const char16_t*) value));
> +            aLiteral, NS_ConvertUTF16toUTF8(value).get()));

Seeing this sort of fix never gets old.

::: security/certverifier/CertVerifier.cpp:269
(Diff revision 4)
>    rv = mCTVerifier->Verify(endEntityDER, issuerPublicKeyDER,
>                             embeddedSCTs, sctsFromOCSP, sctsFromTLS, time,
>                             result);
>    if (rv != Success) {
>      MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
> -            ("SCT verification failed with fatal error %i\n", rv));
> +            ("SCT verification failed with fatal error %" PRIx32 "\n",

Should this be `PRId32` for consistency with the previous code?

::: security/manager/ssl/RootCertificateTelemetryUtils.cpp:58
(Diff revision 4)
>  
>    // Compare against list of stored hashes
>    size_t idx;
>  
>    MOZ_LOG(gPublicKeyPinningTelemetryLog, LogLevel::Debug,
> -           ("pkpinTelem: First bytes %02hx %02hx %02hx %02hx\n",
> +           ("pkpinTelem: First bytes %02x %02x %02x %02x\n",

This change is valid because things get auto-promoted to `int`s in varargs, right?

::: storage/mozStorageAsyncStatement.cpp:129
(Diff revision 4)
>    mDBConnection = aDBConnection;
>    mNativeConnection = aNativeConnection;
>    mSQLString = aSQLStatement;
>  
>    MOZ_LOG(gStorageLog, LogLevel::Debug, ("Inited async statement '%s' (0x%p)",
> -                                      mSQLString.get()));
> +                                      mSQLString.get(), this));

Whoops!

::: storage/mozStorageStatement.cpp:403
(Diff revision 4)
>      // Deactivate the warning until we have fixed the exising culprit
>      // (see bug 914070).
>      NS_WARNING(msg);
>  #endif // 0
>  
> -    MOZ_LOG(gStorageLog, LogLevel::Warning, (msg));
> +    MOZ_LOG(gStorageLog, LogLevel::Warning, ("%s", msg));

Possibly worth a comment on why we're using `%s`.

::: toolkit/components/url-classifier/Classifier.cpp:1005
(Diff revision 4)
>      applied++;
>  
>      auto updateV2 = TableUpdate::Cast<TableUpdateV2>(update);
>      if (updateV2) {
>        LOG(("Applied update to table %s:", store.TableName().get()));
>        LOG(("  %d add chunks", updateV2->AddChunks().Length()));

I am a little surprised this doesn't trigger a mismatch...

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:62
(Diff revision 4)
>      trimmed = EmptyString();
>    }
>  
>    MOZ_LOG(gUrlClassifierStreamUpdaterLog,
>            mozilla::LogLevel::Debug,
> -          (NS_ConvertUTF16toUTF8(trimmed).get()));
> +          ("%s", NS_ConvertUTF16toUTF8(trimmed).get()));

Same comment here about `%s`, perhaps?

::: uriloader/base/nsDocLoader.cpp:484
(Diff revision 4)
>      uint32_t count = 0;
>      if (mLoadGroup)
>        mLoadGroup->GetActiveCount(&count);
>  
>      MOZ_LOG(gDocLoaderLog, LogLevel::Debug,
> -           ("DocLoader:%p: OnStopRequest[%p](%s) status=%x mIsLoadingDocument=%s, %u active URLs",
> +           ("DocLoader:%p: OnStopRequest[%p](%s) status=%" PRIu32 " mIsLoadingDocument=%s, %u active URLs",

Should this be `PRIx32` for consistency with the previous code?
Attachment #8819591 - Flags: review?(nfroyd) → review+
Comment on attachment 8819576 [details]
Bug 1060419 - add -Werror=format to the warnings.configure,

https://reviewboard.mozilla.org/r/99276/#review107168

Sweet!

::: media/webrtc/moz.build:53
(Diff revision 2)
> +if CONFIG['GNU_CXX']:
> +    GYP_DIRS['trunk'].sandbox_vars['CXXFLAGS'] = ['-Wno-format']

Is this just to turn off warning spam?  We turned on `-Wno-error` above.
Attachment #8819576 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #226)

> Is this just to turn off warning spam?  We turned on `-Wno-error` above.

I couldn't remember why I added this, so I removed it and tried a build, and there weren't
even warnings related to it.  So, I'm going to drop this bit.
(In reply to Nathan Froyd [:froydnj] from comment #219)

> I think the usual style when templating over `AllocPolicy` is to have the
> class privately inherit from the provided `AllocPolicy`.  This style takes
> advantage of the empty base class optimization that (I think) all of our
> compilers employ.  Maybe it has to be protected inheritance for this class,
> though, if other states are intended to inherit from this class?

I think SprintfState can be final.  At least nothing in this series needs to derive
from it.  So, I'll do that and make this change.  Thanks for pointing this out,
deriving from the alloc policy is definitely cleaner.
Comment on attachment 8819591 [details]
Bug 1060419 - make log_print use Printf.h,

https://reviewboard.mozilla.org/r/99306/#review103090

> I assume this got deleted due to format checking?  Yay for finding real bugs!

Yes, that's right.  BTW thanks for reviewing this patch so carefully.  I know how painful that must have been.
Comment on attachment 8819591 [details]
Bug 1060419 - make log_print use Printf.h,

https://reviewboard.mozilla.org/r/99306/#review103090

> Nit: can you add a space after `PRIuSIZE` here to make things a little more readable?

I put a space after the comma instead.
Comment on attachment 8819591 [details]
Bug 1060419 - make log_print use Printf.h,

https://reviewboard.mozilla.org/r/99306/#review103090

> This change is valid because things get auto-promoted to `int`s in varargs, right?

Yes.  I think in this case data[NNN] is an unsigned char, so the usual promotion applies.
Hi.  In this bug, we're enabling printf format checking.  We found a couple of spots
in dom/media/MediaDecoderStateMachine.cpp that use the "%o" format.  These need a minor
update, and in the course of this we wondered if octal formatting was really intended here.
If so, no problem, I'll leave it as PRIoSIZE; but if you'd prefer decimal or hex, please
let me know and I will fix it up.  Thanks.
Flags: needinfo?(jwwang)
Comment on attachment 8819591 [details]
Bug 1060419 - make log_print use Printf.h,

https://reviewboard.mozilla.org/r/99306/#review103090

> I am slightly skeptical that somebody actually *wanted* octal notation here, but OK...
> 
> What do you think about changing this to `PRIdSIZE`, on the assumption that somebody fat-fingered this?  I guess we could ni? somebody familiar with this code...

I've NI'd :jww, since git log shows him as a frequent reviewer for this file.
(In reply to Tom Tromey :tromey from comment #227)
> (In reply to Nathan Froyd [:froydnj] from comment #226)
> 
> > Is this just to turn off warning spam?  We turned on `-Wno-error` above.
> 
> I couldn't remember why I added this, so I removed it and tried a build, and
> there weren't
> even warnings related to it.  So, I'm going to drop this bit.

Well, I spoke too soon.  It all builds fine locally but try doesn't like it:

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

I am going to restore this hunk.  I think the reason is that webrtc is imported code
and so it's appropriate to defer any formatting fixes to upstream.
LOL I really made it much worse today.
Comment on attachment 8819591 [details]
Bug 1060419 - make log_print use Printf.h,

https://reviewboard.mozilla.org/r/99306/#review107798

::: dom/media/MediaDecoderStateMachine.cpp:2523
(Diff revision 5)
>  MediaDecoderStateMachine::RequestAudioData()
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    MOZ_ASSERT(mState != DECODER_STATE_SEEKING);
>  
> -  SAMPLE_LOG("Queueing audio task - queued=%i, decoder-queued=%o",
> +  SAMPLE_LOG("Queueing audio task - queued=%" PRIuSIZE ", decoder-queued=%" PRIoSIZE,

s/PRIoSIZE/PRIuSIZE/

::: dom/media/MediaDecoderStateMachine.cpp:2573
(Diff revision 5)
>    bool skipToNextKeyFrame = NeedToSkipToNextKeyframe();
>  
>    media::TimeUnit currentTime = media::TimeUnit::FromMicroseconds(GetMediaTime());
>  
> -  SAMPLE_LOG("Queueing video task - queued=%i, decoder-queued=%o, skip=%i, time=%lld",
> +  SAMPLE_LOG("Queueing video task - queued=%" PRIuSIZE
> +             ", decoder-queued=%" PRIoSIZE ", skip=%i, time=%" PRId64,

s/PRIoSIZE/PRIuSIZE/
Flags: needinfo?(jwwang)
Apparently, on Windows 7, the compiler will choose the wrong constructor overload
for nsPrintfCString.  Removing this overload and changing the sole caller fixes the problem.
Compare...

bad:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e512cc0bbada17aa8c082f640c1bd594cc0cecb0
good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f88957c75dd3f59caa06dfa2f8be4fb18823ba8

We're getting close though.
Comment on attachment 8829529 [details]
Bug 1060419 - add %S and %ls support to Smprintf;

https://reviewboard.mozilla.org/r/106596/#review108366

I wish there were tests for all this, but there can be tests written as a follow-up, not necessarily by you!

::: mozglue/misc/Printf.cpp:63
(Diff revision 1)
> +#if defined(XP_WIN)
> +#define TYPE_WSTRING    12
> +#endif

Can we document this in Printf.h?  Or is it there already?

::: mozglue/misc/Printf.cpp:471
(Diff revision 1)
> +#if defined(XP_WIN)
> +            if (nas[cn].type == TYPE_LONG) {
> +                nas[cn].type = TYPE_WSTRING;
> +                break;
> +            }
> +#endif
> +            if (nas[cn].type == TYPE_INTN) {

What are these additions for?  We shouldn't have initialized data in `nas[cn].type` at this point, should we?

Oh, I see.  These are to handle `%ls`, I guess.  The size prefix handling here leaves something to be desired.

::: mozglue/misc/Printf.cpp:827
(Diff revision 1)
> +                // This should have asserted during BuildArgArray anyway.
> +                MOZ_ASSERT(0);

I don't see the matching assert in `BuildArgArray`...am I missing something?  I see assigning `TYPE_UNKNOWN` in `BuildArgArray`.  Which needs to be fixed, comments, code, or both?

::: mozglue/misc/Printf.cpp:839
(Diff revision 1)
> +                if (rv == 0) {
> +                    rv = 1;
> +                }

WideCharToMultiByte can fail in several ways; I think we only need to worry about handling `ERROR_NO_UNICODE_TRANSLATION`.  Maybe detecting that error here when we're trying to determine the length of the string, skipping the actual conversion, and simply printing `"<unicode errors in string>"`?

::: mozglue/misc/Printf.cpp:842
(Diff revision 1)
> +
> +                int rv = WideCharToMultiByte(CP_ACP, 0, u.ws, -1, NULL, 0, NULL, NULL);
> +                if (rv == 0) {
> +                    rv = 1;
> +                }
> +                char *buf = (char*)malloc(rv);

Can you use `UniqueFreePtr<char>` here?  Or even just `UniquePtr<char[]>`?
Attachment #8829529 - Flags: review?(nfroyd) → review+
Comment on attachment 8819606 [details]
Bug 1060419 - make security/certverifier/ExtendedValidation.cpp use Sprintf.h,

This patch changed a bit, per comment#275.
FWIW I think this new patch (assuming it works, I haven't pushed it through
try yet) takes a much nicer approach.
Attachment #8819606 - Flags: review+ → review?(nfroyd)
Something that occurs to me: if we're going to make MOZ_FORMAT_PRINTF support mingw over in bug 1331349, do we have to care about making this printf implementation support things like %I32d/%I64d/%Id/%Iu ?
Comment on attachment 8819606 [details]
Bug 1060419 - make security/certverifier/ExtendedValidation.cpp use Sprintf.h,

WFM.
Attachment #8819606 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #316)
> Something that occurs to me: if we're going to make MOZ_FORMAT_PRINTF
> support mingw over in bug 1331349, do we have to care about making this
> printf implementation support things like %I32d/%I64d/%Id/%Iu ?

It supports %Id and %Iu already.

I don't think %I32 or %I64 are needed, because normally code should be using
SizePrintfMacros.h, and it doesn't use these.

There's a possibility that someone will write Windows-only code that directly
uses %I32 or %I64.  I am not sure if we care to try to support it just for this case.
Currently the default case in BuildArgArray would call MOZ_ASSERT(0) for this, so
presumably they would immediately notice the problem the first time they used the code.
Comment on attachment 8829529 [details]
Bug 1060419 - add %S and %ls support to Smprintf;

https://reviewboard.mozilla.org/r/106596/#review108366

> Can we document this in Printf.h?  Or is it there already?

I will put it in the comment patch.
Comment on attachment 8829529 [details]
Bug 1060419 - add %S and %ls support to Smprintf;

https://reviewboard.mozilla.org/r/106596/#review108366

> What are these additions for?  We shouldn't have initialized data in `nas[cn].type` at this point, should we?
> 
> Oh, I see.  These are to handle `%ls`, I guess.  The size prefix handling here leaves something to be desired.

Take a look at how NumArgState stores a va_list.  Barf!
Comment on attachment 8829529 [details]
Bug 1060419 - add %S and %ls support to Smprintf;

https://reviewboard.mozilla.org/r/106596/#review108366

> I don't see the matching assert in `BuildArgArray`...am I missing something?  I see assigning `TYPE_UNKNOWN` in `BuildArgArray`.  Which needs to be fixed, comments, code, or both?

On a non-Windows build, the 'S' case in BuildArgArray falls through to the 'C' (/'E'/'G') case, which does MOZ_ASSERT(0).
But then proceeds to do more work...

... but then after the switch, there is:

        if (nas[cn].type == TYPE_UNKNOWN)
            MOZ_CRASH("Bad format string");
Comment on attachment 8829529 [details]
Bug 1060419 - add %S and %ls support to Smprintf;

https://reviewboard.mozilla.org/r/106596/#review108366

I made a note to file this as one of the many follow-up bugs.
Meanwhile there is js/src/jsapi-tests/testPrintf.cpp, which I wrote when doing the JS work.
Blocks: 1334276
Blocks: 1334278
Blocks: 1334279
Blocks: 1334283
Blocks: 1334286
Blocks: 1334289
Blocks: 1334296
Blocks: 1334298
Blocks: 1334301
Blocks: 1334302
Blocks: 1334304
Blocks: 1334305
Blocks: 1334307
Blocks: 1334311
Blocks: 1334313
Blocks: 1334318
I filed all the follow-up bugs that I think I either was asked to, or came up with myself.
I left out a few things from my to-do list in comment #218.  In particular:

gecko/widget/cocoa/nsCocoaDebugUtils.h - uses some ObjC-ism, CFStringCreateWithFormatAndArguments, not printf
widget/windows/WinUtils.cpp - uses a wchar_t format, which gcc at least doesn't check
ipc/hal/DaemonSocketPDUHelpers.h, mozglue/build/WindowsDllBlocklist.cpp, js/src/devtools/vprof/vprof.cpp
 - not clear if these are still relevant
(In reply to Nathan Froyd [:froydnj] from comment #314)
> > +                char *buf = (char*)malloc(rv);
> 
> Can you use `UniqueFreePtr<char>` here?  Or even just `UniquePtr<char[]>`?

To my great surprise, UniquePtr<char[]> did not work.
Take a look at the Windows results here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90db3328bbb6e57a29091b101510e33f3b1fbfc3

Here you can find some crashes like:

10:57:40     INFO - Thread 51 (crashed)
10:57:40     INFO -  0  ntdll.dll!RtlpLowFragHeapFree + 0x89
10:57:40     INFO -     eip = 0x770c2d94   esp = 0x145cf94c   ebp = 0x145cf980   ebx = 0x4d696e6b
10:57:40     INFO -     esi = 0x61001ff1   edi = 0x4d690000   eax = 0x61e28524   ecx = 0xc68bfd7e
10:57:40     INFO -     edx = 0x0000c68b   efl = 0x00010286
10:57:40     INFO -     Found by: given as instruction pointer in context
10:57:40     INFO -  1  ntdll.dll!RtlFreeHeap + 0x7e
10:57:40     INFO -     eip = 0x770c2ce8   esp = 0x145cf988   ebp = 0x145cf998   ebx = 0x00000001
10:57:40     INFO -     Found by: call frame info
10:57:40     INFO -  2  ucrtbase.dll!_free_base + 0x1b
10:57:40     INFO -     eip = 0x677510fb   esp = 0x145cf9a0   ebp = 0x145cf9ac   ebx = 0x145cfadc
10:57:40     INFO -     Found by: call frame info
10:57:40     INFO -  3  ucrtbase.dll!free + 0x18
10:57:40     INFO -     eip = 0x677510c8   esp = 0x145cf9b4   ebp = 0x145cf9bc
10:57:40     INFO -     Found by: call frame info
10:57:40     INFO -  4  mozglue.dll!mozilla::PrintfTarget::vprint(char const *,char *) [Printf.cpp:90db3328bbb6 : 856 + 0x16]
10:57:40     INFO -     eip = 0x67808ed2   esp = 0x145cf9c4   ebp = 0x145cfacc
10:57:40     INFO -     Found by: call frame info


I'm not sure what is going on here.  I would guess that we're seeing a malloc/free mismatch
(based on searching for "RtlpLowFragHeapFree"), but it's hard to see why that would be possible.

Using UniqueMallocPtr works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78081b9b9ebfaf074ba7678238b834676af634e3
... the only difference between these runs being this patch:

https://hg.mozilla.org/try/rev/7f90410a17d5af6d69061604c1b26d55d6845b36
Now I am going to rebase and fix up the fallout.
(In reply to Tom Tromey :tromey from comment #324)
> (In reply to Nathan Froyd [:froydnj] from comment #314)
> > > +                char *buf = (char*)malloc(rv);
> > 
> > Can you use `UniqueFreePtr<char>` here?  Or even just `UniquePtr<char[]>`?
> 
> To my great surprise, UniquePtr<char[]> did not work.
> Take a look at the Windows results here:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=90db3328bbb6e57a29091b101510e33f3b1fbfc3

Oh, that's right, there's some screwy allocator mismatch thing when you use |new| in here.  OK, malloc and UniqueFreePtr would be ideal if you can do that.
I've rebased a couple of times now without big problems.
The first rebase resulted in a couple of patches becoming obsolete, so the series is
a little shorter now.

I'm still investigating the try results.
Attachment #8821615 - Attachment is obsolete: true
Attachment #8819606 - Attachment is obsolete: true
Results are looking pretty good overall, but there are some mysterious android build failures
and dom/xslt/tests/mochitest/test_bug603159.html is failing on a couple of platforms.  Offhand
I'm not sure if these are due to the patches, will investigate more.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d069c7698db72f437d9df371bd3f03e86ffe596
The Android build failures are caused by the first patch (the build changes).
This fixes them:

diff --git a/media/webrtc/moz.build b/media/webrtc/moz.build
index b641d07..a2961b9 100644
--- a/media/webrtc/moz.build
+++ b/media/webrtc/moz.build
@@ -54,7 +54,7 @@ GYP_DIRS['trunk'].variables = gyp_vars
 GYP_DIRS['trunk'].sandbox_vars['ALLOW_COMPILER_WARNINGS'] = True
 GYP_DIRS['trunk'].sandbox_vars['FINAL_LIBRARY'] = 'webrtc'
 if CONFIG['GNU_CXX']:
-    GYP_DIRS['trunk'].sandbox_vars['CXXFLAGS'] = ['-Wno-format']
+    CXXFLAGS += ['-Wno-format']
 GYP_DIRS['trunk'].non_unified_sources += webrtc_non_unified_sources
 
 if CONFIG['MOZ_WEBRTC_SIGNALING']:
... but breaks the 32-bit linux build, apparently because it only passes -Wno-format to some subset
of the compilations.
With a change to the build patch suggested by :froydnj, and some small tweaks for the android
build and due to rebasing, the results are now looking pretty good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0b2876f307b2d4bf79a10b5144d4a4bb7ed562&selectedJob=74056367

The only one that concerns me is OS X M(4) (or Linux asan M(7)):

TEST-UNEXPECTED-FAIL | dom/xslt/tests/mochitest/test_bug603159.html | generated timestamp should be not more than 1800000 ms before 'now', but the difference was: NaN

I haven't investigated this yet but I've seen it in multiple try pushes for this series.
... FYI most of the new series is just more of the same, only rebased so the interdiffs will be
unreadable.  However in the log_print patch, the change to dom/media/systemservices/OpenSLESProvider.cpp
touched the type of a local variable and is otherwise a bit funny (because I couldn't find a reliable
definition of SLResult - it comes from Android system headers and seems different in different builds).
Also the build patch will need re-review.
(In reply to Tom Tromey :tromey from comment #398)
> The only one that concerns me is OS X M(4) (or Linux asan M(7)):
> 
> TEST-UNEXPECTED-FAIL | dom/xslt/tests/mochitest/test_bug603159.html |
> generated timestamp should be not more than 1800000 ms before 'now', but the
> difference was: NaN
> 
> I haven't investigated this yet but I've seen it in multiple try pushes for
> this series.

I think this comes down to returning some bogus result here from Date.parse:

http://dxr.mozilla.org/mozilla-central/source/dom/xslt/tests/mochitest/test_bug603159.html#41

perhaps as a result of the XSLT bit here:

http://dxr.mozilla.org/mozilla-central/source/dom/xslt/tests/mochitest/test_bug603159.html#26

we must format the date completely wrong or something.
Comment on attachment 8819576 [details]
Bug 1060419 - add -Werror=format to the warnings.configure,

This one needs re-review, since I changed it quite a bit.
Now it uses plain -Wformat and leaves turning the warnings into errors
to the specific directories.
Attachment #8819576 - Flags: review+ → review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #400)

> we must format the date completely wrong or something.

Logging shows:

2017-02-03T09:140733193388062:31.140733193388052-4294967304:140733193388032

.. whereas locally I get something like

2017-02-03T11:14:42.089-07:00
Ok, compilers don't diagnose problems here:

https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/xslt/xslt/txEXSLTFunctions.cpp#658

... and now when rebuilding with a suitable change (inlining the definition of formatstr),
I get compiler warnings and not errors, due I guess to the new version of patch #1.
So we need to revisit that somehow.
Comment on attachment 8819576 [details]
Bug 1060419 - add -Werror=format to the warnings.configure,

https://reviewboard.mozilla.org/r/99276/#review110720

Having talked with you and thought about it a little, I am -0 on removing the `-Werror=format` flags from the `moz.build` files below.  I can be persuaded otherwise, and would be interested to hear what you think about the rationale.

::: js/src/jsapi-tests/moz.build:148
(Diff revision 6)
>  ]
>  
>  OS_LIBS += CONFIG['MOZ_ZLIB_LIBS']
>  
>  if CONFIG['GNU_CXX']:
> -    CXXFLAGS += ['-Wno-shadow', '-Werror=format']
> +    CXXFLAGS += ['-Wno-shadow']

...and here

::: js/src/moz.build:798
(Diff revision 6)
>      else:
>          # Windows needs this to be linked with a static library.
>          DEFINES['FFI_BUILDING'] = True
>  
>  if CONFIG['GNU_CXX']:
> -    CXXFLAGS += ['-Wno-shadow', '-Werror=format']
> +    CXXFLAGS += ['-Wno-shadow']

I wonder if these `js/src/` changes to remove `-Werror=format` are helpful.  We'll certainly catch errors in automation, since we compiled with `--enable-warnings-as-errors` there...but having the explicit `-Werror=format` would help local developers catch problems on their builds sooner.  At least for those people who don't compile with `--enable-warnings-as-errors` because they don't trust the set of warnings to be exactly the same as automation.  We'd normally not enable explicit `-Werror=` things because the warnings can change from compiler release to compiler release, but the format warnings ought to be fairly stable in that regard, right...?

::: js/src/shell/moz.build:54
(Diff revision 6)
>      '../js.msg',
>      'ModuleLoader.js',
>  ]
>  
>  if CONFIG['GNU_CXX']:
> -    CXXFLAGS += ['-Wno-shadow', '-Werror=format']
> +    CXXFLAGS += ['-Wno-shadow']

...and here.

::: js/xpconnect/src/moz.build:69
(Diff revision 6)
>      '/layout/style',
>      '/xpcom/reflect/xptinfo',
>  ]
>  
>  if CONFIG['GNU_CXX']:
> -    CXXFLAGS += ['-Wno-shadow', '-Werror=format']
> +    CXXFLAGS += ['-Wno-shadow']

...and here.
Attachment #8819576 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #440)

> Having talked with you and thought about it a little, I am -0 on removing
> the `-Werror=format` flags from the `moz.build` files below.  I can be
> persuaded otherwise, and would be interested to hear what you think about
> the rationale.

I think I just misunderstood our conversation and thought that with a global
-Wformat, the -Werror=format flags wouldn't be needed.  However, I found out
in comment #439 that this is mistaken.  So now the question is - try harder
with the original approach, perhaps fixing up the gyp reader; or add more
uses of -Werror=format through the tree?  Both ways have problems for me: I know
nothing about gyp, but simultaneously I don't have a very good idea for where
the new errors should or should not be put.  On the whole though I suppose I'm
inclined to back to the original approach (the gyp reader one) since that one
just had issues in a single subtree.
(In reply to Tom Tromey :tromey from comment #439)
> Ok, compilers don't diagnose problems here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/xslt/xslt/txEXSLTFunctions.
> cpp#658
> 
> ... and now when rebuilding with a suitable change (inlining the definition
> of formatstr),
> I get compiler warnings and not errors, due I guess to the new version of
> patch #1.
> So we need to revisit that somehow.

Do you get compiler warnings and not errors locally?  Or on automation?  Do you only get warnings if you compile with --enable-warnings-as-errors?
(In reply to Nathan Froyd [:froydnj] from comment #442)

> Do you get compiler warnings and not errors locally?  Or on automation?  Do
> you only get warnings if you compile with --enable-warnings-as-errors?

I introduced a new error into dom/xslt and rebuilt.
I just get a warning locally; but if I use --enable-warnings-as-errors, I get an error.
I'll try automation shortly to see what happens there.
As suspected it also correctly causes an error in automation.

While I think it would be better to cause an error everywhere, I'm ok with this approach if you are,
I suppose primarily because I'd like to get this in.  I can always file another followup bug about
using -Werror=format in more places ... ?  Also I will restore the -Wformat=error bits in the first
patch.
Rebased.  Running through try now; but let me know what you think about the build
change (I added -Werror=format back where I had removed it, but didn't add it anywhere new);
and also the xslt change (part of the nsPrintfCString patch).
(In reply to Tom Tromey :tromey from comment #444)
> As suspected it also correctly causes an error in automation.
> 
> While I think it would be better to cause an error everywhere, I'm ok with
> this approach if you are,
> I suppose primarily because I'd like to get this in.

:)  I am OK with this approach, as it's what's done with the rest of our warnings; we can refine later if need be.
Still building ok after this rebase.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b165f0b296f7286c34ffa345ccce8ea642f1f2e4
Planning to try to land on Wed.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d287c298e2e
add -Werror=format to the warnings.configure, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/6f71d06ae709
remove SprintfState typedef; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e5f4fa3617fd
introduce mozilla::Smprintf and friends; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a69d8969642d
remove unused members from SprintfState, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4182949e0fbc
add PrintfTarget class to jsprf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/83b8f4166e2f
templatize SprintfState and move to jsprf.h; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/dd0cf6092b85
move Smprintf et al to mozglue, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/aa5def25fb21
add %S and %ls support to Smprintf; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/36ead380cb48
remove unneeded includes of prprf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/6369bd114199
make nsChromeRegistry.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/a279ed4f9de1
make MediaEngineWebRTC.h use SprintfLiteral, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ac79a302c430
make nsPluginsDirWin.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4d4495e4dedc
make ipc/chromium use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e93b8cb7d85d
make XPC_Log_print use VsprintfLiteral, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/0757657efd6a
make nsCookieService.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/d7cebacb1f7c
make nsHttpHandler.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/da6402c01d46
make mozStorageConnection.cpp and mozStorageStatement.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/6d9d0198ed9d
make nsExceptionHandler.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/53b80dd756bf
make nsProfileLock.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/495b8a307555
make log_print use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/bd0976eeda5f
make ManifestParser use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/90b1959687b2
make TestPipes.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/fee2788f109a
make nsEnvironment.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/86fcf8b658e4
make WebGLContextUtils.cpp use VsprintfLiteral, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/d89766e9ee60
make nsAppRunner.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/b3f9f7708dbd
make nsWebBrowserPersist.cpp use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/11812794175e
make nsFrame.cpp use VsprintfLiteral, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/c6e12df0792f
make xpcom/glue/nsDebug.h use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/2e15220e1106
make SharedMemoryBasic_mach.mm use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/eff47106a91e
make nsDebugImpl.cpp use PrintfTarget, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/f595410db18f
convert one nsPrintfCString to MOZ_LOG, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9eab13b3a19a
make AppendPrintf and nsPrintfCString use Printf.h, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/dc2d793bf277
fix comment in IntegerPrintfMacros.h to reflect Printf.h change, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e0e53d202593
convert mozglue/linker to use the mfbt-provided printf format defines, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/1f08f556c880
use result of SprintfLiteral in Printf.cpp; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ee190ce6c8ce
update comments in Printf.h; r=froydnj
https://hg.mozilla.org/mozilla-central/rev/5d287c298e2e
https://hg.mozilla.org/mozilla-central/rev/6f71d06ae709
https://hg.mozilla.org/mozilla-central/rev/e5f4fa3617fd
https://hg.mozilla.org/mozilla-central/rev/a69d8969642d
https://hg.mozilla.org/mozilla-central/rev/4182949e0fbc
https://hg.mozilla.org/mozilla-central/rev/83b8f4166e2f
https://hg.mozilla.org/mozilla-central/rev/dd0cf6092b85
https://hg.mozilla.org/mozilla-central/rev/aa5def25fb21
https://hg.mozilla.org/mozilla-central/rev/36ead380cb48
https://hg.mozilla.org/mozilla-central/rev/6369bd114199
https://hg.mozilla.org/mozilla-central/rev/a279ed4f9de1
https://hg.mozilla.org/mozilla-central/rev/ac79a302c430
https://hg.mozilla.org/mozilla-central/rev/4d4495e4dedc
https://hg.mozilla.org/mozilla-central/rev/e93b8cb7d85d
https://hg.mozilla.org/mozilla-central/rev/0757657efd6a
https://hg.mozilla.org/mozilla-central/rev/d7cebacb1f7c
https://hg.mozilla.org/mozilla-central/rev/da6402c01d46
https://hg.mozilla.org/mozilla-central/rev/6d9d0198ed9d
https://hg.mozilla.org/mozilla-central/rev/53b80dd756bf
https://hg.mozilla.org/mozilla-central/rev/495b8a307555
https://hg.mozilla.org/mozilla-central/rev/bd0976eeda5f
https://hg.mozilla.org/mozilla-central/rev/90b1959687b2
https://hg.mozilla.org/mozilla-central/rev/fee2788f109a
https://hg.mozilla.org/mozilla-central/rev/86fcf8b658e4
https://hg.mozilla.org/mozilla-central/rev/d89766e9ee60
https://hg.mozilla.org/mozilla-central/rev/b3f9f7708dbd
https://hg.mozilla.org/mozilla-central/rev/11812794175e
https://hg.mozilla.org/mozilla-central/rev/c6e12df0792f
https://hg.mozilla.org/mozilla-central/rev/2e15220e1106
https://hg.mozilla.org/mozilla-central/rev/eff47106a91e
https://hg.mozilla.org/mozilla-central/rev/f595410db18f
https://hg.mozilla.org/mozilla-central/rev/9eab13b3a19a
https://hg.mozilla.org/mozilla-central/rev/dc2d793bf277
https://hg.mozilla.org/mozilla-central/rev/e0e53d202593
https://hg.mozilla.org/mozilla-central/rev/1f08f556c880
https://hg.mozilla.org/mozilla-central/rev/ee190ce6c8ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1340760
Depends on: 1341017
Depends on: 1341074
Blocks: 1341880
We should probably audit all places we switched out |PR_vsnprintf| with |mozilla::VsnprintfLiteral|. Bug 1341017 shows how this is problematic in several ways:
  #1 - |PR_vsnprintf| returns the number of bytes written, |mozilla::VsnprintfLiteral| returns the number of bytes that *would* have been written
  #2 - |PR_vsnprintf| supports various format extensions, |mozilla::VsnprintfLiteral| uses the posix implementation of |vsnprintf|

#2 might be a non-issue if we got rid of all usages of those format extensions, but given the existence of |mozilla::Vsmprintf| I'm guessing we did not.
(In reply to Eric Rahm [:erahm] from comment #594)

> #2 might be a non-issue if we got rid of all usages of those format
> extensions, but given the existence of |mozilla::Vsmprintf| I'm guessing we
> did not.

We did get rid of them; that was a prerequisite to being able to use
the MOZ_FORMAT_PRINTF attribute.
I think the use in xpcom/base/Logging.cpp is the only problematic use of VsprintfLiteral.
Most of the remaining calls do not check the return value.  The three that do look safe to me:
https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/netwerk/sctp/datachannel/DataChannel.cpp#201
https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/tools/profiler/core/platform.cpp#1391
https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/js/src/jit/x86-shared/AssemblerBuffer-x86-shared.cpp#19
... here only the DataChannel one was touched by this bug.

I'm still looking at calls to SprintfLiteral.
(In reply to Eric Rahm [:erahm] from comment #594)
> We should probably audit all places we switched out |PR_vsnprintf| with
> |mozilla::VsnprintfLiteral|.

I took a look as well and I agree it looks like bug 1341017 was the only instance where the return value was now used incorrectly for vsnprintf.

(In reply to Tom Tromey :tromey from comment #595)
> (In reply to Eric Rahm [:erahm] from comment #594)
> 
> > #2 might be a non-issue if we got rid of all usages of those format
> > extensions, but given the existence of |mozilla::Vsmprintf| I'm guessing we
> > did not.
> 
> We did get rid of them; that was a prerequisite to being able to use
> the MOZ_FORMAT_PRINTF attribute.

Oh great! Do we check all strings for nullptr prior to formatting, or was that a non-issue? I see it's documented we do that with |mozilla::Vsmprintf| [1].

http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/mozglue/misc/Printf.h#21-24
Ok, I went through the patch series, looking for new calls to SprintfLiteral.  There were two:

https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/dom/media/webrtc/MediaEngineWebRTC.h#255
https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/mozglue/misc/Printf.cpp#279

I think those are safe.
It would make sense for someone to check my work of course.
(In reply to Eric Rahm [:erahm] from comment #597)

> Oh great! Do we check all strings for nullptr prior to formatting, or was
> that a non-issue? I see it's documented we do that with |mozilla::Vsmprintf|
> [1].

I think null pointer handling is one main reason to keep all the moz printf code
around at all... if we were confident that null pointers were never passed to %s,
it seems to me we could drop most of the code in favor of calling the system's
*printf functions. 

So, it's not an issue, it's just a feature of the current code that is kept for
compatibility and QoI reasons.
(In reply to Tom Tromey :tromey from comment #599)
> (In reply to Eric Rahm [:erahm] from comment #597)
> 
> > Oh great! Do we check all strings for nullptr prior to formatting, or was
> > that a non-issue? I see it's documented we do that with |mozilla::Vsmprintf|
> > [1].
> 
> I think null pointer handling is one main reason to keep all the moz printf
> code
> around at all... if we were confident that null pointers were never passed
> to %s,
> it seems to me we could drop most of the code in favor of calling the
> system's
> *printf functions. 
> 
> So, it's not an issue, it's just a feature of the current code that is kept
> for
> compatibility and QoI reasons.

It's in issue in the sense that replacing calls to |PR_vsnprintf| with |VsprintfLiteral| removes the null checks though, right?
Depends on: 1344809
Blocks: 1331349
Blocks: 1345609
Depends on: 1343772
Depends on: 1388681
Depends on: 1517433
You need to log in before you can comment on or make changes to this bug.