485704 - cnn.com - Local news broken in Fx3.5 (depends on cross-site XHR throwing)

Status: RESOLVED FIXED

(Tech Evangelism Graveyard :: English US, defect)

Description

[Craig]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3

Certain Buttons that are clickable in previous Firefox versions are not clickable in the Firefox 3.1.3 beta.

As an example, try entering a Local News Zip Code on the www.cnn.com Home Page and the attempt to click the "Go" Button located immediately to the right to display your local news.  The Go Button is appears unclickable and your local news will not be displayed using Firefox 3.1.3. In previous versions, the Go Button will behave as intended.

Reproducible: Always

Steps to Reproduce:
1. www.cnn.com
2. enter any valid zip code in the Local News section on the Home Page
3. Attempt to click the Go Button located immediately to the right of the Zip Code

Actual Results:  
The Go Button is unclickable in Firefox 3.1.3 and does not display any local news


Expected Results:  
Any previous Firefox version or other browser will result in display of local news.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Can you give a zip code?

Comment 2

[Craig]

enter any valid zipcode...  clicking the Go button thereafter using 3.1.3 will not result in display any local news, even tho previous FF versions or any IE version will.  3.1.3 does not seem to recognize the Go button as clickable

Comment 3

[Matthias Versen [:Matti]]

>>Can you give a zip code?
..
>enter any valid zipcode... 

Ria wants a valid zip code, entering "any valid zip code" doesn't work, he wants a number. Why do you think that the whole world knows US zip codes ?
Do you know valid german or dutch zip codes as counterexample ?

Comment 4

[Craig]

Please forgive my ignorance about the global nature of this debugging effort.  I sincerely apologize.  Try entering 48075 and then press Go.  The Go Button appears to be "unclickable" in 3.1.3 and nothing happens.   In all other previous FF versions you will obtain a local news display

Comment 5

[Matthias Versen [:Matti]]

Thanks for the valid Zip Code !

I can confirm this issue with Seamonkey 1.9.1 branch
This isn't jit related, I don't know where this should go, someone must analyze this issue.

requesting blocking 3.5+ because http://www.cnn.com is a top500 site and such an issue directly on the homepage is bad.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: screenshot in 3.1 and 3.0.8 showing regression

It's more than just the zip code element; I'll wager that this is to do with recent changes to XHR and cross site scripting policies.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for top500 regression.

--> Core::DOM for now, though that might not be the right place. Boris, Jonas, Ben: take a peek at this cnn.com regression?

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, quick notes:

No zip code input needed to see the problem.  By default, the "Local" section shows a weather forecast and news for some randomly selected zip code.  Entering a zip code just switches to displaying weather+news for that zip code, but even the default display is not working in current builds.

The regression range (on Mac m-c) is 2008-09-30-02 (rev 86b982e8b73e) to 2008-10-01-02 (rev 63b0791327ab); I'm a little surprised no one noticed this before.... ;)  Pushlog link: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=86b982e8b73e&tochange=63b0791327ab

Lots of checkins; I'll look through it, but others probably should too.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, a bit more data:  This is NOT a regression from bug 450973 (checked via local backout).

The regression range does include Access-Control landing, though, and in a debug build clicking that OK button gives me:

  WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file
  content/base/src/nsCrossSiteListenerProxy.cpp, line 237

That's NS_ERROR_NOT_AVAILABLE.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Making nsCrossSiteListenerProxy::CheckRequestApproved always return NS_OK makes things work.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The access control landing did introduce one change in behavior. We no longer throw when xhr.open is called with a non-same-origin address. Instead we fail the load later.

Or the problem could be that there's a bug in the code that somehow makes a same-site request fail.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Wonderful.  I tried just returning error from nsCrossSiteListenerProxy::UpdateChannel in the branch when it would set mHasBeenCrossSite to true, and that makes the site work.

It looks like the site makes an XHR to 

  http://weather.cnn.com/weather/citySearch?time=22&search_term=ENTER%20A%20U.S.%2FINTL%20CITY%20OR%20ZIP%20CODE&mode=json_html&filter=true&csiID=csi5

from http://www.cnn.com/.  This send() call used to throw, and now doesn't.  Apparently the site was depending on it throwing...

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, so clicking that button calls the checkInput() function in <http://i.cdn.turner.com/cnn/.element/js/2.0/local.js>.  This calls CSIManager.prototype.call as defined in <http://i.cdn.turner.com/cnn/.element/js/2.0/csiManager.js> and passes it updateValidationData as funcObj.

This calls into prototype.js like so:

  new Ajax.Request( ...., onException, onFailure, ...)

where onException and onFailure are locally defined functions that just throw.

Theyre using prototype 1.5.0.  The JS stack inside prototype code:

1 anonymous(url = "http://weather.cnn.com/weather/citySearch") ["http://i.cdn.turner.com/cnn/.element/js/2.0/scripts/prototype.js":866]
    body = null
    params = "time=22&search_term=ENTER%20A%20U.S.%2FINTL%20CITY%20OR%20ZIP%20CODE&mode=json_html&filter=true&csiID=csi5"
    this = [object Object]
2 anonymous(options = [object Object], url = "http://weather.cnn.com/weather/citySearch") ["http://i.cdn.turner.com/cnn/.element/js/2.0/scripts/prototype.js":831]
    this = [object Object]
3 anonymous("http://weather.cnn.com/weather/citySearch", [object Object]) ["http://i.cdn.turner.com/cnn/.element/js/2.0/scripts/prototype.js":23]
    this = [object Object]

Frame 0 there makes the send() call, then catches the resulting exception (in old builds) and calls dispatchException() on it.  This calls the onException handler the caller defined, which in this case throws.

For extra fun, there's this wonderful line of code in the csiManager.js code right before they try to create the XHR and dispatch it:

  if (navigator.userAgent.toLowerCase().indexOf("safari")!=-1)
    throw new Error("SafariCrossDomainFailure");

So in Safari this code always throws.  Oh, and they have a separate codepath in csiManager for IE7 (uses dojo instead of prototype).  That mostly throws too.

In any case, this code used to throw, and all this XHR gunk is wrapped in a try/catch where the catch clause falls back on using an iframe instead of XHR.  See line 301 in csiManager.js linked above.

So in other words, it sounds like they use this csiManager thing on both weather.cnn.com and elsewhere.  When used on the former, they do the XHR stuff.  When used elsewhere they fall back on the iframe.  Unfortunately, their fallback relies on send() throwing (plus the hack for safari).

If their onFailure callback (which gets called async in this case) just did their fallback, things would work, I suspect.  But instead, it just throws....

I don't think we can do anything to fix this on our end short of backing out cross-site XHR altogether.  They need to either adjust their Safari hack to check for all browsers supporting cross-site XHR (which is all of them, soon) or fix their onFailure callback as described above.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Not moving to evangelism for now, because I'm not sure whether we can block 1.9.1 on evang bugs....  But that's where this bug belongs.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Chris, we don't seem to have a way to block on this, but this is probably reasonably high-priority as evang bugs go.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Do we have contacts at CNN, or should we just go through webmaster@?

Comment 17

[Samuel Sidler (old account; do not CC)]

I have a contact, which I'll email.

Comment 18

[Chris Lawson (gone)]

I assume this isn't Windows- or Intel-specific...

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Indeed

Comment 20

[Samuel Sidler (old account; do not CC)]

Assigning to myself for now...

Comment 21

[Christopher Blizzard (:blizzard)]

Sam, any updates?

Comment 22

[Samuel Sidler (old account; do not CC)]

For this bug, yes. For you, no. :(

Since I last updated this bug, the problem has now been entered in their issue tracking system and developers will be working on it. However, I don't have an ETA for deployment.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Sam: if you can poke your contact there and let them know that the browser that's run by 20% of the world will be releasing soon with this feature on their site broken, that'd be awesome.

Comment 24

[Samuel Sidler (old account; do not CC)]

Poked (on Monday, actually).

Comment 25

[Jesse Ruderman]

> In any case, this code used to throw, and all this XHR gunk is wrapped in a
> try/catch where the catch clause falls back on using an iframe instead of XHR. 

The bad news is that this code pattern is understandable and may be used on other sites.  The good news is that sites broken in this manner can just start sending Access-control headers, and suddenly they'll not only work again, but they'll be faster than they used to be (since XHR is saner than frames).

Comment 26

[Christopher Blizzard (:blizzard)]

Sam checked in with the cnn folks.  Sounds like they are moving on this but it's still not fixed in production.

Comment 27

[Christopher Blizzard (:blizzard)]

Looks like the local section is working on shiretoko nightlies now - marking RESOLVED.