Closed Bug 1165834 Opened 5 years ago Closed 5 years ago

add domain that serves styles for 'm.taobao.com' to the -webkit prefix emulation "fixlist"/"whitelist"

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: angelc04, Assigned: dholbert)

Details

Attachments

(3 files)

Attached image m.taobao.com screenshot
After test more, we found that m.taobao.com still have css layout problems. So we would like to add it to the whitelist.

Please see the screenshot for details.
The left picture is after css un-prefixing fix and the right one is before css un-prefixing fix.
So, there's actually a lot going on here:

(1) Back when I fixed bug 1132743, m.taobao.com redirected to http://h5.m.taobao.com/index2.html , which (if I load it directly) works great in Firefox Nightly for Android.  The circular menu shown in the screenshot here renders correctly.

(2) BUT, with the "HTTPS Everywhere" addon installed, my requests to TbCDN.cn (the site we have whitelisted) get upgraded to be HTTPS, and that apparently changes the page's server-side behavior such that it sends me some styles from alicdn.com instead. (which is *not* whitelisted)

So, loading the full "h5.m" URL still works correctly *if* I load it without HTTPS Everywhere activated. (because of the fact that styles are served from a different domain, if my requests for CSS are made using HTTPS)

Moreover:
(3) Now when I load http://m.taobao.com/ or http://www.taobao.com/ (in Firefox or Chrome, with a mobile user agent string), I'm now redirected to http://m.intl.taobao.com/ . Note the "intl" subdomain there. This is new & different behavior, per (1) above. (They must have changed their redirect behavior.)  And it looks like Peipei gets to stay at www.taobao.com -- she does not get redirected away. So I can't actually directly test the page she's seeing.

(4) The "intl" mobile site that I'm redirected to looks different from the one that Peipei sees. It's got some minor layout problems from -webkit-box usage (different from the layout problem in Peipei's screenshot).

(5) The "intl" mobile site gets its resources from an alicdn.com URL.

SO, bottom-line points:
  * I can't actually load the site that Peipei is reporting issues, so I can't be 100% sure what domain needs adding to the whitelist. (It's probably not m.taobao.com; it's some other server where they host their styles. Probably alicdn.com, but read on...)

  * For the versions of this site that *I* can actually load (h5.m.taobao.com and m.intl.taobao.com), the layout issues seem to require that we whitelist alicdn.com subdomains. This is unfortunate, since I suspect a lot of sites might use alicdn.com (if it's Alibaba's equivalent of Akamai). If a lot of other sites do use that CDN, then they'd all get the unprefixing service applied to their CDN-hosted styles, without needing it, and some could conceivably be broken (and slowed down a bit during style parsing) as a result.
Attached patch fix v1Splinter Review
Given that taobao was the #1 site requested in bug 1107378 comment 4, I guess it makes sense to unprefix alicdn.com subdomains on its behalf.  (Though I don't feel great about it, and we should watch for fallout.)

Here's a patch to do that.
(In reply to Daniel Holbert [:dholbert] from comment #1)
>   * I can't actually load the site that Peipei is reporting issues, so I
> can't be 100% sure what domain needs adding to the whitelist. (It's probably
> not m.taobao.com; it's some other server where they host their styles.
> Probably alicdn.com, but read on...)

Peipei, if you're able to connect Fennec to the DevTools (see [1]), it would be very helpful if you could take a screenshot of the CSS files from the Network panel: see https://cloudup.com/c22NtDlS2nJ. There's a CSS filter you can select to only show the CSS. It would be even more awesome if you could right-click on each CSS file name, select "Copy as URL" and paste the results here. Then we could verify where the problematic CSS is coming from.

[1] https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_with_WebIDE
(In reply to Daniel Holbert [:dholbert] from comment #1)
> (1) Back when I fixed bug 1132743, m.taobao.com redirected to
> http://h5.m.taobao.com/index2.html , which (if I load it directly) works
> great in Firefox Nightly for Android.
[...]
> (2) BUT, with the "HTTPS Everywhere" addon installed, my requests to
> TbCDN.cn (the site we have whitelisted) get upgraded to be HTTPS, and that
> apparently changes the page's server-side behavior such that it sends me
> some styles from alicdn.com instead. (which is *not* whitelisted)

FWIW, I can also reproduce this problem by simply loading https://h5.m.taobao.com/ and accepting the invalid security cert (which is a *.alicdn.com cert).

I just tested "fix v1" and verified that it fixes this ^ problem and also fixes the issues I see at http://m.intl.taobao.com/ , so I think it'll likely fix the issue Peipei reported as well.

(In reply to Mike Taylor [:miketaylr] from comment #3)
> Peipei, if you're able to connect Fennec to the DevTools

Given that I'm pretty sure alicdn.com is what we need here, it may be easier to just have Peipei test a patched build. I've got a Try run going here:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef4fd4be7d3a
SGTM.
Attached image taobao_css_domain.png
Dear Daniel & Mike, 
Thank you very much again for your prompt response!

I think taobao displays different content for different country. I used proxy to confirm that changing ip to Japan, it will redirect to a global site.

Here is the steps I followed:
1. input 'taobao.com' in Nightly
   it will redirect to url: http://m.taobao.com/?sprefer=sypc00
2. tap on "触屏版" (the red link near the bottom of this page)
   it goes to http://m.taobao.com/?spm=0.0.0.0.XQnHjT&sid=null&v=0
   And the circular menu is broken. 

Attached is the css network traffic I get for this page. It is true that those css comes from g.alicdn.com so I think your fix could solve my problem~ many thanks~
Flags: needinfo?(pcheng)
Comment on attachment 8607109 [details] [diff] [review]
fix v1

(In reply to Peipei Cheng (needinfo if you need my action) from comment #6
> It is true that
> those css comes from g.alicdn.com

Thanks.

I believe I saw some "i.alicdn.com" requests when I was testing this earlier, and particular CDN subdomain usage seem likely to change over time, so I think we probably should just whitelist *.alicdn.com (as the attached patch does).  

(The shorter "sBaseDomainsOnWhitelist" array modified in this patch is for *.foo.com matching.)

Mike, mind reviewing?
Attachment #8607109 - Flags: review?(miket)
Comment on attachment 8607109 [details] [diff] [review]
fix v1

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

lgtm.
https://hg.mozilla.org/mozilla-central/rev/5562df0e7460
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Is it possible to pick up this fix in v39.0 since the css un-prefixing service is already in v39.0? I think there should be no risk.
(In reply to Peipei Cheng (needinfo if you need my action) from comment #11)
> Is it possible to pick up this fix in v39.0 since the css un-prefixing
> service is already in v39.0? I think there should be no risk.

I wouldn't say "no risk"; as noted at the end of comment 1 (after "This is unfortunate"), the fix here (whitelisting *.alicdn.com for the CSS Unprefixing Service) seems likely to impact other sites that use this CDN.

In theory, assuming well-designed sites, unforseen unprefixing should *hopefully* have little impact (and what impact it has will probably be positive, layout-wise).  But e.g. if someone has a site that hosts its stylesheets on alicdn.com, and uses "display:-webkit-box" with reasonable fallback styles, and our unprefixing service now starts treating "-webkit-box" as "flex" in a way that doesn't quite translate, then their site might end up breaking on Firefox Mobile.

So there is some risk here. But I do think it's reasonable to backport as far as Aurora -- particularly because our nightly coverage for mobile is pretty low (I think) -- and then we should be on the watch for unlikely-but-possible fallout.
Comment on attachment 8607109 [details] [diff] [review]
fix v1

Approval Request Comment
[Feature/regressing bug #]: Not a regression; taobao seems to have moved some of its styles to a CDN, which requiring that we whitelist the CDN for the unprefixing service.

[User impact if declined]: Broken UI on m.taobao.com (due to their dependence on -webkit-prefixed styles). This is the #1 site on the list of top Chinese sites that motivated our unprefixing effort, as noted in comment 2.

[Describe test coverage new/current, TreeHerder]: N/A; this is juts adding a new domain to a whitelist to get opted in to a feature. (That feature, the CSS unprefixing whitelist, is reasonably well tested with mochitests.)

[Risks and why]: It's conceivable that we might break the layout on (or slightly slow down) other sites that share this CDN -- alicdn.com. We can only find out about this by getting broad testing, so from that perspective, getting this on Aurora for broader testing sooner is probably a good thing.  And if this does break something, we can easily back it out.

[String/UUID change made/needed]: None
Attachment #8607109 - Flags: approval-mozilla-aurora?
Comment on attachment 8607109 [details] [diff] [review]
fix v1

OK, let's do it then...
Attachment #8607109 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → dholbert
Thanks!
Summary: add 'm.taobao.com' to the -webkit prefix emulation "fixlist"/"whitelist" → add domain that serves styles for 'm.taobao.com' to the -webkit prefix emulation "fixlist"/"whitelist"
You need to log in before you can comment on or make changes to this bug.