1057642 - PointTyped::x being CoordTyped breaks printf("%f", point.x)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

Mason and I discovered an unfortunate regression caused by bug 923512: 

  CSSPoint p;  // or any other PointTyped or IntPointTyped
  ...
  printf_stderr("(%f, %f)\n", p.x, p.y);  // Undefined behaviour!

Since PointTyped::x and PointTyped::y are now of type CoordTyped, passing them as arguments to printf silently invokes undefined behaviour (since the C++ spec says that passing any object of class type as an argument for an ellipsis parameter results in undefined behaviour).

The undefined behaviour here is not theoretical, either: Mason has seen ridiculous outputs such as (33718362177536.000000, 0.000000), which went away when using

  printf_stderr("(%f, %f)\n", p.x.value, p.y.value);

instead.

Unfortunately, I don't see any way to make the first example either Just Work, or give a compiler error. (If anyone does, please let me know!)

Therefore, it's not immediately clear to me what to do about this. Generally the options that I can think of are:

  (1) Provide a way to print points that doesn't involve printing
      their coordinates individually, such as

        printf_stderr("%s", p.ToString());

      Tell people (via code comments, documentation, and a dev-platform
      post) that they have to either do this, or use '.value' to unwrap
      the CoordTyped objects (or use a type-safe logging mechanism
      such as that in gfx/2d/Logging.h).

      Fix all debugging code currently in the tree to do so.

  (2) Back out the portion of the bug 923512 patch that changed
      PointTyped::x and PointTyped::y to be CoordTyped, leaving
      them as floats.

I would prefer (1), because (2) would go against the grain of slowly making everything strongly-typed. 

If no one argues strongly for (2), I will implement (1).

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This also explains the mysterious crashes I was seeing on my hamachi the other day when I enabled APZC framemetrics logging. I narrowed it down to printing float values but then I went digging in printf_stderr code before giving up :/

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> Unfortunately, I don't see any way to make the first example either Just
> Work, or give a compiler error. (If anyone does, please let me know!)

Turns out there is a way to make the first example give a compiler error, at least on platforms that supports the __atrribute__(format(...)) and where we build with -Werror: by adding said attribute to printf-like functions like printf_stderr. There is a work-in-progress effort to do this in bug 965022, which I'm going to pick up.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Fix some print sites

I don't know if this helps at all but I've been fixing some call sites as I run into them. Here's the ones I've found so far.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8479082 [details] [diff] [review]
Fix some print sites

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> I don't know if this helps at all but I've been fixing some call sites as I
> run into them. Here's the ones I've found so far.

It does - thanks! My plan was to fix them together with other misuses of printf-like functions in bug 965022, but as that's still a work in progress, I think it makes sense to not leave these known issues in the tree until bug 965022 lands.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f888ce4572

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3f888ce4572

Comment 7

[Botond Ballo [:botond]]

Attachment: Do formatting of points in one place

This patch does away with the .x.value, .y.value ugliness in existing places that format points, by using operator<< / ToString() to format points instead (like we did in bug 961289, part 3b for rects, and are doing in bug 1058919 for regions and matrices).

I didn't change APZCCallbackHandler::Handle[Single|Double|Long]Tap because that needs to format the point in a specific (different) way.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8480267 [details] [diff] [review]
Do formatting of points in one place

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

I feel like there must a joke somewhere about logging systems being one of the hardest things in computer science... :p

::: gfx/2d/BasePoint.h
@@ +80,5 @@
>      return *static_cast<Sub*>(this);
>    }
>  
> +  friend std::ostream& operator<<(std::ostream& stream, const Sub& aPoint) {
> +    return stream << '(' << aPoint.x << ',' << aPoint.y << ')';

Can we modify gfx/2d/Logging.h to use this rather than duplicating the code?

Comment 9

[Botond Ballo [:botond]]

Attachment: Do formatting of points in one place

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> I feel like there must a joke somewhere about logging systems being one of
> the hardest things in computer science... :p

I'll take it under advisement for my next T-shirt :p

> ::: gfx/2d/BasePoint.h
> @@ +80,5 @@
> >      return *static_cast<Sub*>(this);
> >    }
> >  
> > +  friend std::ostream& operator<<(std::ostream& stream, const Sub& aPoint) {
> > +    return stream << '(' << aPoint.x << ',' << aPoint.y << ')';
> 
> Can we modify gfx/2d/Logging.h to use this rather than duplicating the code?

Definitely! Updated, carrying r+.

Comment 10

[Botond Ballo [:botond]]

Attachment: Do formatting of points in one place

Fixed a compiler error, carrying r+.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=66a6416d14fa

Comment 11

[Botond Ballo [:botond]]

Attachment: Revert the type of [Int]PointTyped::[x|y] to be primitive

Solving this issue the proper way in bug 965022 has run into some roadblocks, and I don't have the time to see that through right now.

Yet, even with the mitigating patches that have landed so far, the current situation has very high footgun-potential, so we need to do something.

Therefore, I'm going to back out the part of bug 923512 that made the type of [Int]PointTyped::[x|y] a user-defined type. The attached patch does so.

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> Therefore, I'm going to back out the part of bug 923512 that made the type
> of [Int]PointTyped::[x|y] a user-defined type. The attached patch does so.

Note: my plan is to un-revert this once bug 965022 is fixed properly (whenever that may be).

Comment 13

[Botond Ballo [:botond]]

Try push for backout patch: https://tbpl.mozilla.org/?tree=Try&rev=4358caac0c74

Comment 14

[Botond Ballo [:botond]]

Landed the "Do formatting of points in one place" and "Revert the type of [Int]PointTyped::[x|y] to be primitive" patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fac9e4a3738
https://hg.mozilla.org/integration/mozilla-inbound/rev/b54f8eca9cb9

These patches constitute a fix for this bug, so I'm removing the leave-open flag, and the dependency on bug 965022. I will file a follow-up, depending on bug 965022, for reinstating the type of [Int]PointTyped::[x|y] to be [Int]CoordTyped.

Comment 15

[Botond Ballo [:botond]]

This landed on central:

http://hg.mozilla.org/mozilla-central/rev/6fac9e4a3738
http://hg.mozilla.org/mozilla-central/rev/b54f8eca9cb9

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)
> These patches constitute a fix for this bug, so I'm removing the leave-open
> flag, and the dependency on bug 965022. I will file a follow-up, depending
> on bug 965022, for reinstating the type of [Int]PointTyped::[x|y] to be
> [Int]CoordTyped.

Filed bug 1060421 as a follow-up.