Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8590022:
Work around wrong UA sniffing by Yahoo! JAPAN (Closed)
|
Created:
9 years, 1 month ago by Yuzo Modified:
9 years ago CC:
chromium-reviews, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWork around wrong UA sniffing by Yahoo! JAPAN
Some Yahoo! JAPAN pages sniff UA wrongly and reject Chrome, misjudging that
Silverlight is not supported. Work around this issue by spoofing the UA as
Safari on Mac and Firefox on Win for the pages. See the bug below for the
justification. This workaround should be removed once the pages are fixed.
BUG=104426
TEST=Open the URLs listed in the bug
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111877
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed review comments #Patch Set 3 : Addressed review comments #
Total comments: 2
Patch Set 4 : Addressed the 2nd set of comments. #
Total comments: 4
Patch Set 5 : Addressed comments on Nov 21 #Patch Set 6 : Fixed a comment #
Total comments: 2
Patch Set 7 : Fixed nits #Messages
Total messages: 19 (0 generated)
Tony, can you take a look?
Drive-by http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:354: #if defined(OS_MACOSX) || defined(OS_WIN) Don't needlessly if-def things that are meaningful-but-unused on other platforms. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:392: return Breaking the statement into a bunch of ifdef'd pieces makes this harder to read; just do a bunch of #ifdef ... if (...) return true; #elif if (...) return true; #endif return false; http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:413: StartsWithASCII(url.path(), "/videonews/", true)); So... on Windows you spoof this page as Windows Firefox, on Mac you spoof the same page as Mac Safari, and on Linux you don't spoof? That's really confusing. Why such a complex system? Needs a comment. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:426: if (ShouldSpoofUAHack(url)) { I really don't like adding code that appears to be a generic abstraction; it's not. If we decide to spoof another site tomorrow for Windows, how do you know Windows Firefox will be the right UA string? We shouldn't pretend we can make a generic solution for this problem, since it will likely lead to bad spoofing. Every spoofing case is different, and needs to be evaluated differently, and use the right UA for that case.
http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:354: #if defined(OS_MACOSX) || defined(OS_WIN) On 2011/11/17 09:59:17, stuartmorgan wrote: > Don't needlessly if-def things that are meaningful-but-unused on other > platforms. I have no strong opinions about this. If Tony also thinks that way, I can remove the IFDEFs. Just out of curiosity, in what sense is it meaningful on Linux? http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:392: return On 2011/11/17 09:59:17, stuartmorgan wrote: > Breaking the statement into a bunch of ifdef'd pieces makes this harder to read; > just do a bunch of > > #ifdef ... > if (...) > return true; > #elif > if (...) > return true; > #endif > return false; I have no strong opinion on this either. Let's wait for Tony's words. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:413: StartsWithASCII(url.path(), "/videonews/", true)); On 2011/11/17 09:59:17, stuartmorgan wrote: > So... on Windows you spoof this page as Windows Firefox, on Mac you spoof the > same page as Mac Safari, and on Linux you don't spoof? That's really confusing. > Why such a complex system? Needs a comment. Silverlight is not supported on Linux. I don't know how widely Moonshine is used, but at least the Yahoo! JAPAN pages don't support them. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:426: if (ShouldSpoofUAHack(url)) { On 2011/11/17 09:59:17, stuartmorgan wrote: > I really don't like adding code that appears to be a generic abstraction; it's > not. If we decide to spoof another site tomorrow for Windows, how do you know > Windows Firefox will be the right UA string? > > We shouldn't pretend we can make a generic solution for this problem, since it > will likely lead to bad spoofing. Every spoofing case is different, and needs to > be evaluated differently, and use the right UA for that case. I'm not pretending anything -- I've just extracted out a function for refactoring. If you have better naming ideas, please comment. I think spoofing should be rare -- there is only one spoofing case before this change. I'd propose not worrying about other spoofing cases until they actually appear.
http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:354: #if defined(OS_MACOSX) || defined(OS_WIN) On 2011/11/17 10:32:02, Yuzo wrote: > Just out of curiosity, in what sense is it meaningful on Linux? The concept of a UA hack is meaningful on Linux (as evidenced by the fact that we used to have one), it's just not currently used. This code would compile just fine without the ifdefs, just as the old code did. Compare with, say, gfx::CGImageToSkBitmap elsewhere in this file, which is meaningless except on Mac because CGImage doesn't exist anywhere else. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:413: StartsWithASCII(url.path(), "/videonews/", true)); On 2011/11/17 10:32:02, Yuzo wrote: > On 2011/11/17 09:59:17, stuartmorgan wrote: > > So... on Windows you spoof this page as Windows Firefox, on Mac you spoof the > > same page as Mac Safari, and on Linux you don't spoof? That's really > confusing. > > Why such a complex system? Needs a comment. > > Silverlight is not supported on Linux. > I don't know how widely Moonshine is used, but at least the Yahoo! JAPAN pages > don't support them. Please add a comment explaining why you are using the UAs you are using, since it's non-obvious why having different browser's UAs on the different platforms is desirable. Also please add a comment explaining why headlines.yahoo.co.jp is being treated differently than the other yahoo pages in terms of platforms. For something as hacky as UA spoofing, there should be a very clear explanation in the code of why specific choices were made. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:426: if (ShouldSpoofUAHack(url)) { On 2011/11/17 10:32:02, Yuzo wrote: > I'm not pretending anything -- I've just extracted out a function for > refactoring. If you have better naming ideas, please comment. My point is that the refactoring suggests that there are two completely orthogonal concepts--"should we spoof" and "what UA should I use given that"--when those are *not* orthogonal; on the contrary there is a *very* tight bond between the URL being spoofed, and the UA that should be used. That's not a naming problem, it's a fundamental issue with what you have factored apart. If anyone needed to add more spoofing, they would almost certainly have to undo this refactoring to do it correctly. We shouldn't do refactoring that is misleading about the way spoofing should be done, and that makes doing it right in the future harder.
Have we reached out to Yahoo Japan about these issues? When we did the Yahoo Mail spoof for Linux, it was after their dev team told us that they have no plans of supporting Chromium Linux. We should always try to reach out to dev teams first. gwilson handles evangelism issues like this normally, but I'm not sure if there's someone more appropriate to contact Yahoo Japan. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:354: #if defined(OS_MACOSX) || defined(OS_WIN) I agree with Stuart. Fewer #ifs are better. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:392: return On 2011/11/17 10:32:02, Yuzo wrote: > I have no strong opinion on this either. Let's wait for Tony's words. I agree with Stuart here too. The current form is very hard to read. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:433: "Firefox/8.0"; Does Safari Win not work?
(I believe Karen is doing the evangelism these days...) On Thu, Nov 17, 2011 at 10:13 AM, <tony@chromium.org> wrote: > Have we reached out to Yahoo Japan about these issues? > > When we did the Yahoo Mail spoof for Linux, it was after their dev team > told us > that they have no plans of supporting Chromium Linux. We should always > try to > reach out to dev teams first. gwilson handles evangelism issues like this > normally, but I'm not sure if there's someone more appropriate to contact > Yahoo > Japan. > > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > glue/webkit_glue.cc<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc> > File webkit/glue/webkit_glue.cc (right): > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > glue/webkit_glue.cc#newcode354<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newcode354> > webkit/glue/webkit_glue.cc:**354: #if defined(OS_MACOSX) || > defined(OS_WIN) > I agree with Stuart. Fewer #ifs are better. > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > glue/webkit_glue.cc#newcode392<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newcode392> > webkit/glue/webkit_glue.cc:**392: return > On 2011/11/17 10:32:02, Yuzo wrote: > >> I have no strong opinion on this either. Let's wait for Tony's words. >> > > I agree with Stuart here too. The current form is very hard to read. > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > glue/webkit_glue.cc#newcode433<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newcode433> > webkit/glue/webkit_glue.cc:**433: "Firefox/8.0"; > Does Safari Win not work? > > http://codereview.chromium.**org/8590022/<http://codereview.chromium.org/8590... >
Yes, I reached out to Yahoo! JAPAN. They told us that they recognized this issue but cannot commit when to fix. It means no ETA at this time. They indicated it will take longer. I already told that we will probably hack Chrome to workaround this issue. On 2011/11/17 22:12:07, Glenn Wilson wrote: > (I believe Karen is doing the evangelism these days...) > > On Thu, Nov 17, 2011 at 10:13 AM, <mailto:tony@chromium.org> wrote: > > > Have we reached out to Yahoo Japan about these issues? > > > > When we did the Yahoo Mail spoof for Linux, it was after their dev team > > told us > > that they have no plans of supporting Chromium Linux. We should always > > try to > > reach out to dev teams first. gwilson handles evangelism issues like this > > normally, but I'm not sure if there's someone more appropriate to contact > > Yahoo > > Japan. > > > > > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > > > glue/webkit_glue.cc<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc> > > File webkit/glue/webkit_glue.cc (right): > > > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > > > glue/webkit_glue.cc#newcode354<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newcode354> > > webkit/glue/webkit_glue.cc:**354: #if defined(OS_MACOSX) || > > defined(OS_WIN) > > I agree with Stuart. Fewer #ifs are better. > > > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > > > glue/webkit_glue.cc#newcode392<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newcode392> > > webkit/glue/webkit_glue.cc:**392: return > > On 2011/11/17 10:32:02, Yuzo wrote: > > > >> I have no strong opinion on this either. Let's wait for Tony's words. > >> > > > > I agree with Stuart here too. The current form is very hard to read. > > > > http://codereview.chromium.**org/8590022/diff/1/webkit/** > > > glue/webkit_glue.cc#newcode433<http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newcode433> > > webkit/glue/webkit_glue.cc:**433: "Firefox/8.0"; > > Does Safari Win not work? > > > > > http://codereview.chromium.**org/8590022/%3Chttp://codereview.chromium.org/85...> > >
Hi, Stuart, Tony, I've addressed the comments. Can you take another look? http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:354: #if defined(OS_MACOSX) || defined(OS_WIN) On 2011/11/17 18:13:06, tony wrote: > I agree with Stuart. Fewer #ifs are better. Done. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:392: return On 2011/11/17 18:13:06, tony wrote: > On 2011/11/17 10:32:02, Yuzo wrote: > > I have no strong opinion on this either. Let's wait for Tony's words. > > I agree with Stuart here too. The current form is very hard to read. Done. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:413: StartsWithASCII(url.path(), "/videonews/", true)); On 2011/11/17 10:50:38, stuartmorgan wrote: > On 2011/11/17 10:32:02, Yuzo wrote: > > On 2011/11/17 09:59:17, stuartmorgan wrote: > > > So... on Windows you spoof this page as Windows Firefox, on Mac you spoof > the > > > same page as Mac Safari, and on Linux you don't spoof? That's really > > confusing. > > > Why such a complex system? Needs a comment. > > > > Silverlight is not supported on Linux. > > I don't know how widely Moonshine is used, but at least the Yahoo! JAPAN pages > > don't support them. > > Please add a comment explaining why you are using the UAs you are using, since > it's non-obvious why having different browser's UAs on the different platforms > is desirable. > > Also please add a comment explaining why headlines.yahoo.co.jp is being treated > differently than the other yahoo pages in terms of platforms. > > For something as hacky as UA spoofing, there should be a very clear explanation > in the code of why specific choices were made. Done. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:426: if (ShouldSpoofUAHack(url)) { On 2011/11/17 10:50:38, stuartmorgan wrote: > On 2011/11/17 10:32:02, Yuzo wrote: > > I'm not pretending anything -- I've just extracted out a function for > > refactoring. If you have better naming ideas, please comment. > > My point is that the refactoring suggests that there are two completely > orthogonal concepts--"should we spoof" and "what UA should I use given > that"--when those are *not* orthogonal; on the contrary there is a *very* tight > bond between the URL being spoofed, and the UA that should be used. That's not a > naming problem, it's a fundamental issue with what you have factored apart. > > If anyone needed to add more spoofing, they would almost certainly have to undo > this refactoring to do it correctly. We shouldn't do refactoring that is > misleading about the way spoofing should be done, and that makes doing it right > in the future harder. Done. http://codereview.chromium.org/8590022/diff/1/webkit/glue/webkit_glue.cc#newc... webkit/glue/webkit_glue.cc:433: "Firefox/8.0"; On 2011/11/17 18:13:06, tony wrote: > Does Safari Win not work? No, it doesn't. Added a comment.
http://codereview.chromium.org/8590022/diff/4003/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/4003/webkit/glue/webkit_glue.cc#n... webkit/glue/webkit_glue.cc:403: url.host() == "gyao.yahoo.co.jp") { Please make a helper function for the yahoo cases. Something like, bool IsYahooSiteThatNeedsSpoofing(const GURL& url) { if (url.host() == "headlines.yahoo.co.jp" && StartsWithASCII(url.path(), "/videonews/", true)) { return true; } #if defined(OS_MACOSX) if (url.host() == "downloads.yahoo.co.jp" && StartsWithASCII(url.path(), "/docs/silverlight/", true)) || url.host() == "gyao.yahoo.co.jp") { return true; #elif defined(OS_WIN) if (url.host() == "weather.yahoo.co.jp" && StartsWithASCII(url.path(), "/weather/zoomradar/", true)) { return true; } #endif return false; }
On 2011/11/18 01:50:30, takuya wrote: > Yes, I reached out to Yahoo! JAPAN. They told us that they recognized this > issue but cannot commit when to fix. It means no ETA at this time. They > indicated it will take longer. I already told that we will probably hack Chrome > to workaround this issue. A user agent spoof is OK for now. Please make sure to check again in a few months (maybe make a calendar reminder or something). We don't want to forget about this hack.
On 2011/11/18 18:36:51, tony wrote: > On 2011/11/18 01:50:30, takuya wrote: > > Yes, I reached out to Yahoo! JAPAN. They told us that they recognized this > > issue but cannot commit when to fix. It means no ETA at this time. They > > indicated it will take longer. I already told that we will probably hack > Chrome > > to workaround this issue. > > A user agent spoof is OK for now. Please make sure to check again in a few > months (maybe make a calendar reminder or something). We don't want to forget > about this hack. We'll keep the bug opened until the issue is fixed on Yahoo! JAPAN's sites.
Can you take yet another look? http://codereview.chromium.org/8590022/diff/4003/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/4003/webkit/glue/webkit_glue.cc#n... webkit/glue/webkit_glue.cc:403: url.host() == "gyao.yahoo.co.jp") { On 2011/11/18 18:35:10, tony wrote: > Please make a helper function for the yahoo cases. Something like, > > bool IsYahooSiteThatNeedsSpoofing(const GURL& url) { > if (url.host() == "headlines.yahoo.co.jp" && > StartsWithASCII(url.path(), "/videonews/", true)) { > return true; > } > #if defined(OS_MACOSX) > if (url.host() == "downloads.yahoo.co.jp" && > StartsWithASCII(url.path(), "/docs/silverlight/", true)) || > url.host() == "gyao.yahoo.co.jp") { > return true; > #elif defined(OS_WIN) > if (url.host() == "weather.yahoo.co.jp" && > StartsWithASCII(url.path(), "/weather/zoomradar/", true)) { > return true; > } > #endif > return false; > } Done.
I still don't like that the two different cases are mingled together here, since it will make removing one of them without accidentally regressing the other one much harder (someone would have to go back and look at all the details of both bugs to figure out which platforms are part of which spoofing). I would rather see the one line of setting the Mac UA duplicated so that the two unrelated spoofing blocks could be completely separate. http://codereview.chromium.org/8590022/diff/11001/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/11001/webkit/glue/webkit_glue.cc#... webkit/glue/webkit_glue.cc:440: // Pretend to be Firefox. Mac Safari doesn't support Silverlight. This is not a true statement; not only does Mac Safari support Silverlight, Microsoft considers it a supported platform. I assume what you really mean is that the old script they are using doesn't consider it supported, so you should say that.
http://codereview.chromium.org/8590022/diff/11001/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/11001/webkit/glue/webkit_glue.cc#... webkit/glue/webkit_glue.cc:442: "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 " Can we use webkit_glue::BuildOSCpuInfo() here? I could imagine Windows version and 64 bitness may matter on these Yahoo sites.
Can you take yet another look? http://codereview.chromium.org/8590022/diff/11001/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/11001/webkit/glue/webkit_glue.cc#... webkit/glue/webkit_glue.cc:440: // Pretend to be Firefox. Mac Safari doesn't support Silverlight. On 2011/11/21 08:17:16, stuartmorgan wrote: > This is not a true statement; not only does Mac Safari support Silverlight, > Microsoft considers it a supported platform. I assume what you really mean is > that the old script they are using doesn't consider it supported, so you should > say that. I meant Win Safari, not Mac Safari. Corrected. http://codereview.chromium.org/8590022/diff/11001/webkit/glue/webkit_glue.cc#... webkit/glue/webkit_glue.cc:442: "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 " On 2011/11/21 21:25:16, tony wrote: > Can we use webkit_glue::BuildOSCpuInfo() here? I could imagine Windows version > and 64 bitness may matter on these Yahoo sites. Good point. Thanks!
LGTM; I guess since they are both Silverlight-related sharing the UA code is okay. I'd strongly prefer the helper functions end with "NeedsSilverlightSpoofing" or "NeedsSpoofingForSilverlight" though, to make it very clear that these are two related cases. That way it's less likely someone trying to add some new, completely different spoofing later won't think the right thing to do is to just add another site check to the existing if block. http://codereview.chromium.org/8590022/diff/18002/webkit/glue/webkit_glue.cc File webkit/glue/webkit_glue.cc (right): http://codereview.chromium.org/8590022/diff/18002/webkit/glue/webkit_glue.cc#... webkit/glue/webkit_glue.cc:411: StartsWithASCII(url.path(), "/docs/silverlight/", true)) || Indentation is off here and in the next if http://codereview.chromium.org/8590022/diff/18002/webkit/glue/webkit_glue.cc#... webkit/glue/webkit_glue.cc:438: BuildUserAgentFromProduct("Version/5.0.4 Safari/533.20.27"); As long as you are in this code, it would be a good time to bump this to the current version ("Version/5.1.1 Safari/534.51.22")
LGTM2 with Stuart's nits fixed.
On 2011/11/28 17:34:46, tony wrote: > LGTM2 with Stuart's nits fixed. Tony, Stuart, thank you for the review. I'll land this after fixing the nits.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yuzo@chromium.org/8590022/22001 |
