Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 12 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 1 • 12 years ago

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

Is fennec seeing this?

	Matt Brubeck (:mbrubeck)
	Comment 2 • 12 years ago

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.)

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 3 • 12 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 4 • 12 years ago

Filed bug 798183 on implementing the missing logic here.

	Scott Johnson (:jwir3) Assignee
	Comment 5 • 12 years ago

(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?

	Matt Brubeck (:mbrubeck)
	Comment 6 • 12 years ago

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

	Scott Johnson (:jwir3) Assignee
	Comment 7 • 12 years ago

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

	Matt Brubeck (:mbrubeck)
	Comment 9 • 12 years ago

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

Please request a review from a DOM peer also.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 10 • 12 years ago

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.

	Scott Johnson (:jwir3) Assignee
	Comment 11 • 12 years ago

(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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 12 • 12 years ago

(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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 13 • 12 years ago

(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 ...

	Scott Johnson (:jwir3) Assignee
	Comment 14 • 12 years ago

(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.

	Scott Johnson (:jwir3) Assignee
	Comment 15 • 12 years ago

New patch that addresses review comments.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 16 • 12 years ago

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.

	Matt Brubeck (:mbrubeck)
	Comment 17 • 12 years ago

(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.)

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 18 • 12 years ago

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.

	Scott Johnson (:jwir3) Assignee
	Comment 20 • 12 years ago

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

	Phil Ringnalda (:philor)
	Comment 21 • 12 years ago

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"

	Matt Brubeck (:mbrubeck)
	Comment 22 • 12 years ago

(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).

	Scott Johnson (:jwir3) Assignee
	Comment 24 • 12 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 25 • 12 years ago

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).

	Alex Keybl [:akeybl]
	Comment 26 • 12 years ago

(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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 27 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 28 • 12 years ago

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

	Alex Keybl [:akeybl]
	Comment 29 • 12 years ago

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

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Comment 30 • 12 years ago

Sorry, forgot to clear the flag.

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 12 years ago

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