1573648 - site scaling is off, showing desktop version instead of mobile due to mis-handling trailing characters in scale values

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Ksenia Berezina [:ksenia]]

Steps to reproduce:

With Firefox Nightly on Android (68.1a1 2019-08-13)
Go to http://www.gamefront.de/

Expected:
Mobile version of the site is displayed

Actual:
Site scaling is off (showing desktop version)

From mozregression:

7:08.28 INFO: Narrowed inbound regression window from [9fb72f8a, 4cd6d838] (3 builds) to [87067f04, 4cd6d838] (2 builds) (~1 steps left)
 7:08.28 INFO: No more inbound revisions, bisection finished.
 7:08.28 INFO: Last good revision: 87067f045e1e8c957ce1e3c6b6777a525a1d3fc0
 7:08.28 INFO: First bad revision: 4cd6d838796a453f9de5821b2e146f84174badfb
 7:08.28 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=87067f045e1e8c957ce1e3c6b6777a525a1d3fc0&tochange=4cd6d838796a453f9de5821b2e146f84174badfb

Comment 1

[Hiroyuki Ikezoe (:hiro)]

The viewport content in question is "width=device-width, initial-scale=1.0/". The initial-scale value ends with '/'. It's invalid. I have no idea how Chrome handles this value. I will check it. That's being said, I'd say the site should fix the initial-scale value.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

This is my fault. The spec clearly says;

If a prefix of property-value can be converted to a number using strtod, the value will be that number. The remainder of the string is ignored.

And Chrome does end up calling this ToDoubleType with kAllowTailingJunk which means subsequent strings are ignored.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

njn, can we change that ToDouble sets NS_OK in the case where we converted the string to some extent instead of setting NS_ERROR_ILLEGAL_VALUE? It seems ToInteger returns NS_OK in such cases.

Or, can we assume that in the case where the returned double value isn't 0.0 it's a valid and parsed value in call sites?

Comment 4

[Nicholas Nethercote [inactive]]

I assume you are talking about nsTString<char>::ToDouble(nsresult*) const and nsTSubstring<T>::ToInteger(nsresult*, uint32_t) const?

njn, can we change that ToDouble sets NS_OK in the case where we converted the string to some extent instead of setting NS_ERROR_ILLEGAL_VALUE? It seems ToInteger returns NS_OK in such cases.

I don't know. It seems reasonable to make ToDouble's behaviour match ToInteger's behaviour, but there's some risk that existing code depends on the current behaviour. I suggest doing a try push and see what happens. If it doesn't cause problems on try and you decide to go forward with it, please update the header file comments for ToInteger, ToInteger64, ToDouble, and ToFloat to describe this behaviour.

You could also look through all the call sites, there aren't that many. That might give you an additional sense of whether it's safe.

If changing the behaviour does cause trouble, you could create a new function with a name like ToDoubleAllowTrailingChars().

Or, can we assume that in the case where the returned double value isn't 0.0 it's a valid and parsed value in call sites?

I recommend not doing that. It would depend on an internal detail of ToDouble that could easily change in the future.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Nicholas Nethercote [:njn] from comment #4)

I assume you are talking about nsTString<char>::ToDouble(nsresult*) const and nsTSubstring<T>::ToInteger(nsresult*, uint32_t) const?

njn, can we change that ToDouble sets NS_OK in the case where we converted the string to some extent instead of setting NS_ERROR_ILLEGAL_VALUE? It seems ToInteger returns NS_OK in such cases.

I don't know. It seems reasonable to make ToDouble's behaviour match ToInteger's behaviour, but there's some risk that existing code depends on the current behaviour. I suggest doing a try push and see what happens. If it doesn't cause problems on try and you decide to go forward with it, please update the header file comments for ToInteger, ToInteger64, ToDouble, and ToFloat to describe this behaviour.

You could also look through all the call sites, there aren't that many. That might give you an additional sense of whether it's safe.

If changing the behaviour does cause trouble, you could create a new function with a name like ToDoubleAllowTrailingChars().

Thanks for the suggestion. Actually one of the call sites in nsAttrValue::ParseDoubleValue which is used for parsing value and max attributes of HTML progress element can't be changed. I will add the ToDoubleAllowTrailingChars. Thanks!

Comment 6

[Nicholas Nethercote [inactive]]

Interesting. You could consider renaming the existing ToInteger{,64} functions as ToIntegerAllowTrailingChars{,64} so that the names match the semantics.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Nicholas Nethercote [:njn] from comment #6)

Interesting. You could consider renaming the existing ToInteger{,64} functions as ToIntegerAllowTrailingChars{,64} so that the names match the semantics.

Filed bug 1574353.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1573648 - Introduce ToDoubleAllowTrailingChars and ToFloatAllowTrailingChars. r?njn

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1573648 - Use ToFloatAllowTrailingChars for parsing scale values in meta viewport tag. r?botond

From the spec https://drafts.csswg.org/css-device-adapt/#parsing-algorithm

If a prefix of property-value can be converted to a number using strtod,
the value will be that number. The remainder of the string is ignored.

Note that nsTSubStrubg::ToInteger() allows trailing characters as it is so that
we don't need to modify the width/height parser for meta viewport.

Depends on D42239

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38f4e91d3a35
Introduce ToDoubleAllowTrailingChars and ToFloatAllowTrailingChars. r=njn
https://hg.mozilla.org/integration/autoland/rev/92a7cfbc46ba
Use ToFloatAllowTrailingChars for parsing scale values in meta viewport tag. r=botond

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/38f4e91d3a35
https://hg.mozilla.org/mozilla-central/rev/92a7cfbc46ba

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR68 approval when you get a chance.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9085929 [details]
Bug 1573648 - Use ToFloatAllowTrailingChars for parsing scale values in meta viewport tag. r?botond

Beta/Release Uplift Approval Request

• User impact if declined: Sites affected by this bug are rendered as if it's on desktop
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change does just handle scale values having trailing characters that are unrelated numbers as valid scales in meta viewport (it's what the spec defines!), I believe it doesn't have risky parts.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is one of mobile web compat issues we've been tackling and worth fixing in Fennec.
• User impact if declined: Sites affected by this bug are rendered as if it's on desktop
• Fix Landed on Version: 70 as of now
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9085928 [details]
Bug 1573648 - Introduce ToDoubleAllowTrailingChars and ToFloatAllowTrailingChars. r?njn

Fixes a pretty visible mobile webcompat issue. Approved for 69.0b16 and Fennec 68.1b8.

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/40120557f1e2
https://hg.mozilla.org/releases/mozilla-beta/rev/750a7788ab03

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/b2e95f71010d
https://hg.mozilla.org/releases/mozilla-esr68/rev/24da9c922e80

Comment 17

[Laurentiu Apahidean]

I have tested the issue on the latest Firefox builds: 69.0b16 and 70.0a1 (2019-08-21) using a Google Pixel 3a XL (Android 9) and the issue no longer occurs. I will mark this issue accordingly. I will leave the qe-verify+ flag up until the fix is uplifted to the esr build.

Comment 18

[Andrei Jolțan [:Andi]]

Hi, verified as fixed on Firefox 68.1b8 using the following devices:
• Samsung Galaxy Note 9 (Android 9)
• Samsung Galaxy S9 (Android 8)
• Samsung Galaxy S7 (Android 7)
• Samsung Galaxy S6 (Android 6.0.1)