Add MOZ_FORMAT_PRINTF to AppendPrintf and nsPrintfCString

RESOLVED FIXED in Firefox 54

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: botond, Assigned: tromey)

Tracking

(Blocks: 8 bugs)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(36 attachments, 2 obsolete attachments)

58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
58 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
59 bytes, text/x-review-board-request
froydnj
: review+
Details | Review
(Reporter)

Description

3 years ago
This is a follow-up to bug 965022.

See comments 31-33, and patches 4.5, 5, and 6, on that bug.
(Reporter)

Updated

3 years ago
Blocks: 1060421
Component: Graphics: Layers → XPCOM
(Assignee)

Comment 1

3 years ago
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.
(Reporter)

Comment 2

3 years ago
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)?
(Assignee)

Comment 3

3 years ago
(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.
(Reporter)

Comment 4

3 years ago
(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
(Assignee)

Comment 5

3 years ago
(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 :(
(Assignee)

Comment 6

3 years ago
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.
(Reporter)

Updated

a year ago
See Also: → bug 1296122
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1296122
(Assignee)

Comment 8

10 months ago
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.
(Assignee)

Comment 9

10 months ago
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.
(Assignee)

Comment 10

10 months ago
This code is a bit funny:

https://dxr.mozilla.org/mozilla-central/source/js/ipc/JavaScriptLogging.h#75
(Assignee)

Comment 11

10 months ago
Might as well admit it.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
(Assignee)

Comment 12

10 months ago
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.
(Assignee)

Comment 13

10 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 45

10 months ago
Well that was hilarious.
(Assignee)

Comment 46

10 months ago
webrtc has some -Wformat issues but IIUC we import this from upstream.
So I'm just going to disable the warning there.
(Assignee)

Comment 47

10 months ago
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.
(Assignee)

Comment 48

10 months ago
Only ~90 patches to get it to pass try!  But I will rebase them into sanity.
(Assignee)

Comment 49

10 months ago
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.

Updated

10 months ago
Depends on: 1324987
(Assignee)

Comment 50

10 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 83

10 months ago
mozreview-review
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 84

10 months ago
mozreview-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 85

10 months ago
mozreview-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 86

10 months ago
mozreview-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 87

10 months ago
mozreview-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+
(Assignee)

Comment 88

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 89

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 90

10 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 120

10 months ago
mozreview-review-reply
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 121

10 months ago
mozreview-review
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 122

10 months ago
mozreview-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 123

10 months ago
mozreview-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 124

10 months ago
mozreview-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 125

10 months ago
mozreview-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+
(Assignee)

Comment 126

10 months ago
mozreview-review-reply
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
(Assignee)

Comment 127

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 128

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 129

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 130

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 131

10 months ago
mozreview-review-reply
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 132

10 months ago
mozreview-review
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 133

10 months ago
mozreview-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 134

10 months ago
mozreview-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 135

10 months ago
mozreview-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 136

10 months ago
mozreview-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 137

10 months ago
mozreview-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 138

10 months ago
mozreview-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 139

10 months ago
mozreview-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 140

10 months ago
mozreview-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 141

10 months ago
mozreview-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 142

10 months ago
mozreview-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 143

10 months ago
mozreview-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 144

10 months ago
mozreview-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 145

10 months ago
mozreview-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 146

10 months ago
mozreview-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 147

10 months ago
mozreview-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 148

10 months ago
mozreview-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 149

10 months ago
mozreview-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+
(Assignee)

Comment 150

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 151

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 152

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 153

10 months ago
mozreview-review-reply
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 154

10 months ago
mozreview-review
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.
(Assignee)

Comment 156

10 months ago
(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.
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1251303
(Assignee)

Comment 158

9 months ago
(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.
(Assignee)

Comment 160

9 months ago
(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 161

9 months ago
mozreview-review
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 162

9 months ago
mozreview-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+
(Assignee)

Comment 163

9 months ago
mozreview-review-reply
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.
(Assignee)

Comment 164

9 months ago
mozreview-review-reply
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.
(Assignee)

Comment 165

9 months ago
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.
(Assignee)

Comment 166

9 months ago
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.
(Assignee)

Comment 167

9 months ago
Err, I should say, it *only* has a char16_t interface.
GCC at least won't do any checking here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 204

9 months ago
mozreview-review
Comment on attachment 8819577 [details]
Bug 1060419 - remove SprintfState typedef;

https://reviewboard.mozilla.org/r/99278/#review106028
Attachment #8819577 - Flags: review?(ttromey)
(Assignee)

Comment 205

9 months ago
mozreview-review
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)
(Assignee)

Comment 206

9 months ago
mozreview-review
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)
(Assignee)

Comment 207

9 months ago
Still trying to figure out how to re-r? some of those patches...
(Reporter)

Comment 208

9 months ago
(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 209

9 months ago
mozreview-review
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 210

9 months ago
mozreview-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 211

9 months ago
mozreview-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+
(Assignee)

Updated

9 months ago
Attachment #8819577 - Flags: review?(ttromey)
Attachment #8819577 - Flags: review?(nfroyd)
Attachment #8819577 - Flags: review+
(Assignee)

Updated

9 months ago
Attachment #8819601 - Flags: review?(ttromey)
Attachment #8819601 - Flags: review?(nfroyd)
Attachment #8819601 - Flags: review+
(Assignee)

Updated

9 months ago
Attachment #8819578 - Flags: review?(ttromey)
Attachment #8819578 - Flags: review?(nfroyd)
Attachment #8819578 - Flags: review+
(Assignee)

Comment 212

9 months ago
(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.
(Assignee)

Updated

9 months ago
Blocks: 1331760
(Assignee)

Updated

9 months ago
Blocks: 1331762
(Assignee)

Updated

9 months ago
Blocks: 1331766
(Assignee)

Comment 213

9 months ago
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.
(Assignee)

Comment 214

9 months ago
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?
(Assignee)

Comment 217

9 months ago
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.
(Assignee)

Comment 218

9 months ago
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 219

9 months ago
mozreview-review
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.
(Assignee)

Comment 223

9 months ago
(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 225

9 months ago
mozreview-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 226

9 months ago
mozreview-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+
(Assignee)

Comment 227

9 months ago
(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.
(Assignee)

Comment 228

9 months ago
(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.
(Assignee)

Comment 229

9 months ago
mozreview-review-reply
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.
(Assignee)

Comment 230

9 months ago
mozreview-review-reply
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.
(Assignee)

Comment 231

9 months ago
mozreview-review-reply
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.
(Assignee)

Comment 232

9 months ago
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)
(Assignee)

Comment 233

9 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 272

9 months ago
(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.
(Assignee)

Comment 273

9 months ago
LOL I really made it much worse today.

Comment 274

9 months ago
mozreview-review
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)
(Assignee)

Comment 275

9 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 314

9 months ago
mozreview-review
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+
(Assignee)

Comment 315

9 months ago
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+
(Assignee)

Comment 318

9 months ago
(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.
(Assignee)

Comment 319

9 months ago
mozreview-review-reply
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.
(Assignee)

Comment 320

9 months ago
mozreview-review-reply
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!
(Assignee)

Comment 321

9 months ago
mozreview-review-reply
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");
(Assignee)

Comment 322

9 months ago
mozreview-review-reply
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.
(Assignee)

Updated

9 months ago
Blocks: 1334276
(Assignee)

Updated

9 months ago
Blocks: 1334278
(Assignee)

Updated

9 months ago
Blocks: 1334279
(Assignee)

Updated

9 months ago
Blocks: 1334283
(Assignee)

Updated

9 months ago
Blocks: 1334286
(Assignee)

Updated

9 months ago
Blocks: 1334289
(Assignee)

Updated

9 months ago
Blocks: 1334296
(Assignee)

Updated

9 months ago
Blocks: 1334298
(Assignee)

Updated

9 months ago
Blocks: 1334301
(Assignee)

Updated

9 months ago
Blocks: 1334302
(Assignee)

Updated

9 months ago
Blocks: 1334304
(Assignee)

Updated

9 months ago
Blocks: 1334305
(Assignee)

Updated

9 months ago
Blocks: 1334307
(Assignee)

Updated

9 months ago
Blocks: 1334311
(Assignee)

Updated

9 months ago
Blocks: 1334313
(Assignee)

Updated

9 months ago
Blocks: 1334318
(Assignee)

Comment 323

9 months ago
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
(Assignee)

Comment 324

9 months ago
(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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)