798068 - nsContentUtils::GetViewportInfo() returns bonkers minZoom/maxZoom values on m.cnn.com

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Load m.cnn.com
 (2) Examine ViewportInfo.minZoom/maxZoom

I've seen values (bug 795657) like

(min = -6.98217e-07, max = 2.07784e+08)
(min = 1.74498e+08, max = 5.35786e-315)

The platform interface shouldn't return garbage data or it's going to force all clients to work around it.  Instead, we should ensure these values are clamped to sane min/max values (prefs?) and don't force clients to check.

These kinds of bugs are a great way to introduce security holes.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Doug, want to take a look?  If you're swamped with school, Matt can you help?

Is fennec seeing this?

Comment 2

[Matt Brubeck (:mbrubeck)]

We should be clamping those values to the range [0.0, 10.0] here:
http://hg.mozilla.org/mozilla-central/file/58f3ccaa02b8/content/base/src/nsContentUtils.cpp#l5153
http://hg.mozilla.org/mozilla-central/file/58f3ccaa02b8/content/base/src/nsContentUtils.cpp#l5168

(It would be a good idea to also enforce minZoom <= maxZoom.)

After that, the only modification to the values is multiplying them by the device pixel ratio, which ought to be in the 1.0 to 2.0 range on normal hardware:
http://hg.mozilla.org/mozilla-central/file/58f3ccaa02b8/content/base/src/nsContentUtils.cpp#l5199

Fennec is not currently using GetViewportInfo.  (The Fennec patches bounced because of intermittent reftest failures.)

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What's happening here is that for m.cnn.com,

I/Gecko   (16981): [Viewport][nsContentUtils]     WAP/Mobile/WML site, bailing

so we early-return from nsContentUtils::GetViewportInfo() with a bunch of uninitialized values in the returned ViewportInfo.  In one instance, we tried to set the CSS viewport to 1e9 x 2e7, which gecko did not like at all.

I don't know what the intention is here or in

    // If the docType specifies that we are on a site optimized for mobile,
    // then we want to return specially crafted defaults for the viewport info.

where those "specially crafted defaults" are supposed to come from, but at the very least this function should return a failure code instead of garbage.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Remove WAP/Mobile/WML/handheldFriendly code until we can return meaningful initialized values

Filed bug 798183 on implementing the missing logic here.

Comment 5

[Scott Johnson (:jwir3)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Created attachment 668291 [details] [diff] [review]
> Remove WAP/Mobile/WML/handheldFriendly code until we can return meaningful
> initialized values
> 
> Filed bug 798183 on implementing the missing logic here.

Rather than just removing it, though, which will likely break font-inflation reftests, why don't we just make it return _something_ consistent by default?

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 668291 [details] [diff] [review]
Remove WAP/Mobile/WML/handheldFriendly code until we can return meaningful initialized values

I agree with jwir3; we should simply add the remaining default values to the initializers at the head of the function:

width = aDisplayWidth
height = aDisplayHeight
minZoom = kViewportMinScale
maxZoom = kViewportMaxScale

Comment 7

[Scott Johnson (:jwir3)]

I can take this. I'll write a patch that consistently applies constraints to all these values.

Comment 8

[Scott Johnson (:jwir3)]

Attachment: b798068

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 668474 [details] [diff] [review]
b798068

Please request a review from a DOM peer also.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 668474 [details] [diff] [review]
b798068

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

::: content/base/public/nsContentUtils.h
@@ +1570,5 @@
> +   * in order to always return sane minimum/maximum values.
> +   *
> +   * @return A constrained viewport data structure.
> +   */
> +  static ViewportInfo ConstrainViewportValues(ViewportInfo& aViewInfo);

It's weird that this returns the same ViewportInfo that was passed in... As a caller would I need to do something like this?

  ViewportInfo constrained = ViewportInfo(mViewInfo);
  mViewInfo = constrained;

The comment implies that I should ("@return A constrained viewport data structure" doesn't say anything about returning *the same* structure). The only hint that I shouldn't is that the function takes a non-const ref.

Beyond the comments, though, I think it would be much more obvious that the function does an in-place modification if it returned void. Can we do that instead?

::: content/base/src/nsContentUtils.cpp
@@ +255,5 @@
> +static const uint32_t  kViewportMinWidth = 200;
> +static const uint32_t  kViewportMaxWidth = 10000;
> +static const uint32_t  kViewportMinHeight = 223;
> +static const uint32_t  kViewportMaxHeight = 10000;
> +static const int32_t   kViewportDefaultScreenWidth = 980;

Why did you reformat all of these to have two spaces between type and name? I'd revert.

Comment 11

[Scott Johnson (:jwir3)]

(In reply to ben turner [:bent] from comment #10)

> Beyond the comments, though, I think it would be much more obvious that the
> function does an in-place modification if it returned void. Can we do that
> instead?
> 

I can change that. Do you think it would be more obvious if I returned a new ViewportInfo struct from the constraining function, or just have it return void and change it in place?

> Why did you reformat all of these to have two spaces between type and name?
> I'd revert.

Sure, I can do that. I only did it because I changed the type of the top preferences to "double" instead of float, and I thought it looked nicer if the variable names were lined up, but I can certainly revert that.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Scott Johnson (:jwir3) from comment #11)
> I can change that. Do you think it would be more obvious if I returned a new
> ViewportInfo struct from the constraining function, or just have it return
> void and change it in place?

I would leave it with the in-place modification. If callers want copies they can make them themselves.

> Sure, I can do that. I only did it because I changed the type of the top
> preferences to "double" instead of float, and I thought it looked nicer if
> the variable names were lined up, but I can certainly revert that.

I'm fine with them all lining up, but before you had this:

 - static const uint32_t kViewportMinWidth = 200;

and now you have this:

 + static const uint32_t  kViewportMinWidth = 200;

You should only need to reformat the double lines.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Scott Johnson (:jwir3) from comment #5)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > Created attachment 668291 [details] [diff] [review]
> > Remove WAP/Mobile/WML/handheldFriendly code until we can return meaningful
> > initialized values
> > 
> > Filed bug 798183 on implementing the missing logic here.
> 
> Rather than just removing it, though, which will likely break font-inflation
> reftests, why don't we just make it return _something_ consistent by default?

How could the font inflation reftests possibly work?  If we'd hit this bug in the field, it'd be sg:crit.

I feel like there's a big piece of information I'm missing ...

Comment 14

[Scott Johnson (:jwir3)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> How could the font inflation reftests possibly work?  If we'd hit this bug
> in the field, it'd be sg:crit.
> 
> I feel like there's a big piece of information I'm missing ...

Well, what I mean is that this GetViewportInfo function was created, initially, so we could better detect sites that are optimized for mobile when performing font inflation. It didn't matter what the values that came back were if we detected "handheldFriendly" or "WAP/Mobile/WML", just that if those existed, then it was assumed that the site was a mobile version of the site. We then disabled font inflation. 

The code was copied over, almost verbatim from javascript and translated into C++. But, the B2G stuff is the first time we're having to use this code "for real", rather than just checking to see if the <meta name="viewport"> element exists on the page.

Comment 15

[Scott Johnson (:jwir3)]

Attachment: b798068 (v2)

New patch that addresses review comments.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, OK.  So something was lost in the translation to C++?

mbrubeck, this is a possible (although unlikely) hypothesis for the talos bustage for fennec.

Comment 17

[Matt Brubeck (:mbrubeck)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #16)
> Oh, OK.  So something was lost in the translation to C++?

Right; in the original JS code these values would be "undefined" or NaN if not explicitly set (rather than a random value from uninitialized memory), and the code that consumed the values checked for that.

> mbrubeck, this is a possible (although unlikely) hypothesis for the talos
> bustage for fennec.

The Talos bustage was already fixed, though there are still some intermittent reftest failures that this might affect.  (However, I doubt most of those reftests used handheldFriendly or mobile doctypes.)

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 668597 [details] [diff] [review]
b798068 (v2)

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

r=me with this:

::: content/base/src/nsContentUtils.cpp
@@ +5154,5 @@
>          if ((docId.Find("WAP") != -1) ||
>              (docId.Find("Mobile") != -1) ||
>              (docId.Find("WML") != -1))
>          {
> +

Nit: Nix this extra line.

Comment 20

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9456e53577df

Comment 21

[Phil Ringnalda (:philor)]

Outbounded in https://hg.mozilla.org/integration/mozilla-inbound/rev/9e582b26b6b1 - "test_meta_viewport6.html | maximum scale defaults to the absolute maximum - got 10, expected 15"

Comment 22

[Matt Brubeck (:mbrubeck)]

(In reply to Phil Ringnalda (:philor) from comment #21)
> "test_meta_viewport6.html | maximum scale defaults to the absolute maximum -
> got 10, expected 15"

ConstrainViewportValues need to be adjusted to take GetDevicePixelsPerMetaViewportPixel into account (or we need to multiply/device the return values by the pixel ratio after constraining them, instead of before).

Comment 23

[Scott Johnson (:jwir3)]

Attachment: b798068 (v3)

Comment 24

[Scott Johnson (:jwir3)]

Inbound, with fixes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07ff446d1bd5

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 669213 [details] [diff] [review]
b798068 (v3)

[Approval Request Comment]

This is blocking-basecamp+ so should get automatic aurora approval.

The code affected by this patch only runs in b2g (currently).

Comment 26

[Alex Keybl [:akeybl]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> This is blocking-basecamp+ so should get automatic aurora approval.

If it's near-zero risk to desktop/mobile, a=blocking-basecamp (explicit approval not required). Otherwise, please provide a risk assessment here.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, this is zero risk to Firefox and Firefox-android.  The code affected here only runs in service of b2g at the moment.

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/07ff446d1bd5

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 669213 [details] [diff] [review]
b798068 (v3)

No need to get explicit approval in that case. Land with a=blocking-basecamp.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, forgot to clear the flag.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/690a3827c465