824301 - cnn.com - Unresponsive (after changeset ea373e534245) due to Optimizely not restoring setters

Status: RESOLVED FIXED

(Tech Evangelism Graveyard :: English US, defect)

Description

[Gary [:streetwolf52]]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20121222 Firefox/20.0
Build ID: 20121222031804

Steps to reproduce:

Installed http://hg.mozilla.org/mozilla-central/rev/ea373e534245.

Went to cnn.com.


Actual results:

Page either doesn't load, get's an unresponsive message, or a script error.


Expected results:

Page should load normally.

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Setting to New - confirming...

OK in cset: https://hg.mozilla.org/mozilla-central/rev/04abdd43606b prior to m-i -> m-c 
merge noted in commnet #0

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

this in error console2:
Error: TypeError: k[0] is undefined
Source file: http://z.cdn.turner.com/cnn/.e/js/libs/jquery-1.7.2.min.js
Line: 2

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

Problem seems to be isolated to the Main www.cnn.com page.  Other pages on site work OK.

Page will start to load then dump to a blank white page with the status-bar-overlay at the bottom showing waiting for ads.cnn.com...  

Site issue or bad ad link ?

Comment 4

[Gary [:streetwolf52]]

I also had the problem on money.cnn

Comment 5

[Gary [:streetwolf52]]

cnn.com works fine under IE10

Comment 6

[Gary [:streetwolf52]]

http://abc.go.com/ also has the same problem.  It does appear to have something to do with ads.  I get a script error message.  The script is one that Admuncher uses.  It isn't an Admuncher problem as I killed the task and still had the problem. IE10 also uses Admuncher and that works fine.

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

http://abc.go.com  WFM - I don't use AdMuncher

CNN also does not work in a clean profile.  It's some 'js' in an Ad I think....or some change in JS in the affected cset.

Comment 8

[Gary [:streetwolf52]]

If I disable Admuncher abc.go.com works fine.  However, cnn.com produces the same results Jim gets.

Comment 9

[Gary [:streetwolf52]]

It appears that the jscript that Admuncher adds to both these sites is affected by the affected cset.  cnn.com because it fails for Jim and for me, even if AM is disabled, must mean cnn is using a jscript that is affected by the cset also.

Comment 10

[Alice0775 White]

Error: TypeError: k[0] is undefined
Source file: http://z.cdn.turner.com/cnn/tmpl_asset/static/intl_homepage/969/js/intlhplib-min.js
Line: 4
Error: TypeError: c is null
Source file: http://platform.twitter.com/widgets.js
Line: 1




Regression window:
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/98dddd0da122
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121221 Firefox/20.0 ID:20121221100222
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/06661265d9e9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121221 Firefox/20.0 ID:20121221102122
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=98dddd0da122&tochange=06661265d9e9

Suspected : a6d996266cfe	Peter Van der Beken — Fix for bug 821606 (Turn on WebIDL bindings for Element and HTMLElement). r=bz.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Interesting.  We were already using the WebIDL methods, so what exactly broke here?

The code preceding the use of k[0] is:

  k=p.getElementsByTagName("td")

which should generally speaking work....  

Just to check, how is Admuncher involved?  The bug is reproducible in a plain nightly with no extensions, right?  Certainly comment 10 seems to be...

Comment 12

[Boris Zbarsky [:bzbarsky]]

A testcase that can be run locally, and in particular with a non-minified jquery would be amazingly useful.... ;)

Comment 13

[Alice0775 White]

This bug happens intermittently(timing?/ advertisement?) with clean profile,

Comment 14

[Alice0775 White]

And now, I cannot reproduce the problem anymore...

Comment 15

[Alice0775 White]

(In reply to Alice0775 White from comment #14)
> And now, I cannot reproduce the problem anymore...
Sorr, pls ignore this comment

Comment 16

[Alice0775 White]

(In reply to Alice0775 White from comment #13)
> This bug happens intermittently(timing?/ advertisement?) with clean profile,
Sorry, pls also ignore this comment

Comment 17

[Alice0775 White]

Attachment: reduced html

I am not sure this is root cause or not.
But, this reduced html causes the following error on the bad build(cset a6d996266cfe).

Error: TypeError: tds[0] is undefined
Source File: http://127.0.0.1/20121223193608/intlhplib-min.js
Line: 1575

However, no such error appears on the good build.

Comment 18

[Peter Van der Beken [:peterv]]

Hmm, the innerHTML set at line 1572 doesn't call the innerHTML setter.

Comment 19

[Gary [:streetwolf52]]

Admuncher runs as a Windows program not as an addon to firefox therefore it can be used for other browsers and some other ad producing programs.

It adds CSS codes and scripts to the page before Fx executes it.  This is how it blocks ads.  I believe the script AM injects is failing because of the regression. 

When AM is totally disabled abc works because AM is not injecting it's script.  However cnn must be using it's own script that is also affected by the regression.  

It is not admuncher causing the problem but the problem is affecting AM's scripts.(In reply to Boris Zbarsky (:bz) from comment #11)
> Interesting.  We were already using the WebIDL methods, so what exactly
> broke here?
> 
> The code preceding the use of k[0] is:
> 
>   k=p.getElementsByTagName("td")
> 
> which should generally speaking work....  
> 
> Just to check, how is Admuncher involved?  The bug is reproducible in a
> plain nightly with no extensions, right?  Certainly comment 10 seems to be...

Comment 20

[Boris Zbarsky [:bzbarsky]]

So I debugged the getElementsByTagName thing and it's just being called on an element which has no kids.  So having no [0] makes sense.

The bit from comment 18 might explain why the element had no kids!

Comment 21

[Boris Zbarsky [:bzbarsky]]

The property descriptor for innerHTML has no setter?

Comment 22

[Boris Zbarsky [:bzbarsky]]

In particular, I did this on the lovely testcase from comment 17 (for which, Alice, thank you!):

	    console.log(Object.getOwnPropertyDescriptor(div.__proto__.__proto__, "innerHTML"));

and it logged:

  [09:52:18.785] ({configurable:true, enumerable:true, get:function () {
      [native code]
  }, set:(void 0)})

Comment 23

[Boris Zbarsky [:bzbarsky]]

Uh.  So the 128727546.js in the attached testcase has this thing:

  optly.Cleanse.getElementsByClassName = null;
  optly.Cleanse.getters = {};
  optly.Cleanse.logs = [];
  optly.Cleanse.properties = {};
  optly.Cleanse.types = {
    HTMLElement_: window.HTMLElement,
    Object_: Object
  };

and then this optly.Cleanse thing goes and deletes everything off both of those prototypes, while saving the getters and "properties" (haven't looked into what that bit is) and then goes through and:

        Object.prototype.__defineGetter__ && optly.Cleanse.each(optly.Cleanse.getters[a], function (c, e) {
          b.prototype.__defineGetter__(c, e);
          optly.Cleanse.log("restored getter", a, c)
        });

So it's nuking the setters of everything on HTMLElement, which means it breaks once we follow WebIDL and move things up to the proto.  :(

Is Admuncher using this "optly.Cleanse" thing as well?

Comment 24

[Boris Zbarsky [:bzbarsky]]

And in particular, this optly.Cleanse setup seems to have absolutely no provisions for saving/restoring setters, only getters....

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ah.  So abc.go.com has this bit:

  <script type="text/javascript" src="http://cdn.optimizely.com/js/9823262.js"></script>

which does the optly.Cleanse dance on HTMLElement and Object.

I wonder why this works in IE...  I thought IE put things on the right protos too?

Comment 26

[Boris Zbarsky [:bzbarsky]]

CNN has:

  <script src="http://cdn.optimizely.com/js/131788053.js"></script>

I just sent this mail to [e-mail]:


------------------------------------------------------------------------
Subject: Optimizely scripts break web pages (including cnn.com) in upcoming Firefox versions by breaking setting of .innerHTML


I can't find a way to report a bug directly, so hoping this will work...

Firefox 20 implements the WebIDL specification for HTML elements.  This means that properties on HTML elements are implemented via getter/setter pairs (which was already the case) but also that the ones shared by all HTML elements are a getter/setter pair on HTMLElement.prototype (whereas before they were getter/setter pairs on the most-derived prototype).

Now cnn.com loads <script src="http://cdn.optimizely.com/js/131788053.js"></script> which goes through and removes all enumerable properties from HTMLElement.prototype, then restores their values and getters but not their setters.

Which means that once this script has run, setting .innerHTML on, say, an HTML <div> no longer works.  Neither does setting all sorts of other properties (e.g. no more setting .onclick and company).

Would you mind fixing your script to preserve setters as well, not just getters?

Thanks,
Boris
------------------------------------------------------------------------

Comment 27

[Boris Zbarsky [:bzbarsky]]

And I tried contacting them on Twitter (@Optimizely) too.

The good news is that this is all coming via their CDN, so if they fix it on their end sites will pick it up automagically.

Comment 28

[Masatoshi Kimura [:emk]]

Attachment: optly.Cleanse.logs on Nightly

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: optly.Cleanse.logs on IE10

(In reply to Boris Zbarsky (:bz) from comment #25)
> I wonder why this works in IE...  I thought IE put things on the right
> protos too?

Apparently IE doesn't enumerate innerHTML...

Comment 30

[Peter Van der Beken [:peterv]]

We could try moving innerHTML to Element.prototype (see http://domparsing.spec.whatwg.org/#extensions-to-the-element-interface).

Comment 31

[Gary [:streetwolf52]]

Don't know if this helps but when AdMuncher is enabled and I go to cnn.com I will eventually get a message that a script is not responding. The script on the message is located at http://interceptedby.admuncher.com/26445D0911FFB298/helper.js#0.70615.0:1

If I stop the script the page loads, albeit with some things missing on it.

abc.com with AM enabled has this for a script error 
http://interceptedby.admuncher.com/26445D0911FFB298/helper.js#0.70624.0:1

Keep in mind that abc.com works fine when I disable AM. So as I said it appears that cnn.com has it's own script that is affected without AM enabled but abc.com is only affected if AM is enabled which must be the AM script.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> Apparently IE doesn't enumerate innerHTML...

Masatoshi-san, thank you for checking what IE is doing.  Does it perhaps put innerHTML on Element.prototype, as Peter suggests doing?

peterv, I hadn't realized that's been pushed up to Element.  I think moving it there is pretty reasonable.

For what it's worth, the optimizely folks replied to my mail and are looking into this on their end too, which they'll want to do no matter what we do on our end.

Gary, abc.com loads a script that messes up the DOM prototype chain.  So if admuncher relies on the DOM to actually work correctly it could easily go haywire on abc.com.

Comment 33

[Gary [:streetwolf52]]

FWIW   http://www.usmagazine.com/celebrities/brandi-glanville gets the same error when AM is enabled.  

Do you still need sites that are affected to be reported here?

Comment 35

[Peter Van der Beken [:peterv]]

I've uploaded a patch to bug 811701. It seems to fix attachment 695295 [details]. Need to check the sites too.

Comment 36

[Peter Van der Beken [:peterv]]

Seems to fix at least cnn.com.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> http://www.usmagazine.com/celebrities/brandi-glanville

	<script src="//cdn.optimizely.com/js/54104786.js"></script>

Gary, I think it's not worth reporting more sites for the moment; we know what's going on here.

Comment 38

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky (:bz) from comment #32)
> Masatoshi-san, thank you for checking what IE is doing.  Does it perhaps put
> innerHTML on Element.prototype, as Peter suggests doing?

>> Element.prototype.hasOwnProperty('innerHTML') 
false 
>> HTMLElement.prototype.hasOwnProperty('innerHTML') 
true 

It's beyond me what IE is doing...

Comment 39

[Boris Zbarsky [:bzbarsky]]

> It's beyond me what IE is doing...

What happens if you look at 

  JSON.stringify(Object.getOwnPropertyDescriptor(HTMLElement.prototype, "innerHTML"))

in IE?  Or even better, if it supports toSource, at 

  Object.getOwnPropertyDescriptor(HTMLElement.prototype, "innerHTML").toSource()

?  Or at least at 

  Object.getOwnPropertyDescriptor(HTMLElement.prototype, "innerHTML").enumerable

?

Comment 40

[Masatoshi Kimura [:emk]]

>> JSON.stringify(Object.getOwnPropertyDescriptor(HTMLElement.prototype,
"innerHTML")) 
"{"enumerable":true,"configurable":true}"

>> Object.getOwnPropertyDescriptor(HTMLElement.prototype,
"innerHTML").toSource() 
"オブジェクトは 'toSource' プロパティまたはメソッドをサポートしていません。"
(Translation: "Object doesn't support property or method 'toSource'")

>> Object.getOwnPropertyDescriptor(HTMLElement.prototype,
"innerHTML").enumerable 
true

Comment 41

[Boris Zbarsky [:bzbarsky]]

Ok, what about:

  var enum = false;
  for (var name in HTMLElement.prototype) {
    if (name == "innerHTML") enum = true;
  }
  alert(enum);

in IE?  Sorry for all these questions; my IE install is 3 timezones away until next week.  :(

Comment 42

[Boris Zbarsky [:bzbarsky]]

In any case, the fix for bug 811701 should fix this...

Comment 43

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky (:bz) from comment #41)
> Ok, what about:
> 
>   var enum = false;
>   for (var name in HTMLElement.prototype) {
>     if (name == "innerHTML") enum = true;
>   }
>   alert(enum);
> 
> in IE?

It complained "識別子に予約語を使用することは無効です。"
(Translation: "The use of a reserved word for an identifier is invalid.")
I replaced "enum" with "enumerated" and tried, then "true" was popped.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Ok.  So IE enumerates innerHTML fine...  I have no idea why the script in question is not replacing it in IE; I'd have to dig through its details to figure it out.

Comment 45

[Masatoshi Kimura [:emk]]

Ah, I understood. optly.Cleanse.each didn't enumerate getters on IE because IE doesn't support __lookupGetter__.
  optly.Cleanse.each = function (a, b, c) {
    var f = !! Object.prototype.__lookupGetter__,
      d;
    for (d in a) if (a.hasOwnProperty(d)) {
      var g = f ? a.__lookupGetter__(d) : null;
      try {
        b.call(c, d, !g ? a[d] : null, g)
      } catch (e) {}
    }
  };
IE tries to touch getters on protoypes (just like properties) and throws TypeError, then optly.Cleanse.each swallows the error silently.

Comment 46

[Alice0775 White]

Attachment: another reduced html

Slide show is not displayed.

Regression range is same as Comment 10.

m-i tinderbox build cset http://hg.mozilla.org/integration/mozilla-inbound/rev/5d900d13b2cf [1] does not help.

Steps to reproduce:
1. Extract 20121224181014.zip
2. Open index.html with file: protocol

Actual results:
Spinning image appears. but slide show is not displayed

Expected results:
Slide show should be displayed


[1]
5d900d13b2cf	Peter Van der Beken — Fix for bug 811701 (Move innerHTML/outerHTML/insertAdjacentHTML from HTMLElement to Element). r=bzbarsky.

Comment 47

[Masatoshi Kimura [:emk]]

> Jim Jeffery not reading bug-mail 1/2/11 from bug 811701 comment #14
> Just tested an m-i build with this patch cset:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/5d900d13b2cf
> 
> CNN will now load, but not fully.  I have spinning markers on 
> Featured TV box on right side
> Markets box below that on right side
> and Weather information is missing totally.
> 
> Several js errors in the Error console:
> Error: TypeError: o.style is undefined
> Source file: http://cdn.optimizely.com/js/131788053.js
> Line: 48
> 
> 4 of these:
> Error: TypeError: document.getElementById(...).style is undefined
> Source file:
> http://z.cdn.turner.com/cnn/tmpl_asset/static/www_homepage/1899/js/hplib-min.
> js
> Line: 23
> 
> Error: TypeError: d.style is undefined
> Source file: http://z.cdn.turner.com/cnn/.e/js/libs/jquery-1.7.2.min.js
> Line: 2
> 
> Error: TypeError: $(...).select is not a function
> Source file:
> http://z.cdn.turner.com/cnn/tmpl_asset/static/www_homepage/1899/js/hplib-min.
> js
> Line: 26
> 
> Error: TypeError: $(...).update is not a function
> Source file:
> http://z.cdn.turner.com/cnn/tmpl_asset/static/www_global/796/js/globallib-
> min.js
> Line: 2
> 
> Sorry I know nothing about webpage layout, js , etc...

Comment 48

[Masatoshi Kimura [:emk]]

.style defined on HTMLElement even in the latest HTML/DOM spec.
> Optimizely / Info / Cleanse / cleansed getter: HTMLElement.style
Now this bug should belong to Tech-Evang.

Comment 49

[Gary [:streetwolf52]]

Same problem as Jim's in comment 47 after installing 811701.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> Optimizely / Info / Cleanse / cleansed getter: HTMLElement.style

<sigh>

Can we consider just blocking the optimizely scripts from loading until they fix them?  :(

Comment 51

[Boris Zbarsky [:bzbarsky]]

> Slide show is not displayed.

This could well be due to the .style bustage.

Comment 52

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky (:bz) from comment #50)
> Can we consider just blocking the optimizely scripts from loading until they
> fix them?  :(

I don't think it works because optimizely scripts also contain many site-specific stuff...

Comment 53

[alex_mayorga]

Ended up here from http://forums.mozillazine.org/viewtopic.php?p=12578849#p12578849 where Alice0775 suggest optimizely it's also used at http://www.mylifetime.com/

Should a new bug be filled or would this one deal with all sites besides CNN?

Comment 54

[Boris Zbarsky [:bzbarsky]]

One bug is probably OK, since these are all using the optimizely CDN, thankfully.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Note that I just mailed optimizely again asking them for a timeline.  If we don't hear back from them by Monday, I'll work on a patch to block loads from their CDN until they resolve this...

Comment 56

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky (:bz) from comment #55)
> Note that I just mailed optimizely again asking them for a timeline.  If we
> don't hear back from them by Monday, I'll work on a patch to block loads
> from their CDN until they resolve this...

See comment #52.

Comment 57

[Boris Zbarsky [:bzbarsky]]

I did see that, but I didn't follow it.  As far as I can tell, optimizely is some sort of a/b testing thing.  Does not loading their scripts at all (e.g. blocking them with noscript) actually break any functionality on web pages?  The couple I've tried seemed to be ok.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Current response from them:

  Thanks for following up. I’m trying to get a status update on the issue, and it is
  definitely on our radar. I will let you know when I hear more.

Note that I'm not talking to engineers directly, as far as I can tell, but to support folks...

Comment 59

[Jim Jeffery not reading bug-mail 1/2/11]

Not sure if CNN fixed their Optmizely scripts or what, but its working.  Other sites with the same Optimizely script issue are still broken, so I'm assuming Optimizely has not fixed their end yet, and CNN did something. 

I'm not good at webpage analysis to know what changed or happened.

Comment 60

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #59)
> Not sure if CNN fixed their Optmizely scripts or what, but its working. 
> Other sites with the same Optimizely script issue are still broken, so I'm
> assuming Optimizely has not fixed their end yet, and CNN did something. 
> 
> I'm not good at webpage analysis to know what changed or happened.

Actually, http://techcrunch.com/, from bug 827692 is working fine for me, right now, in Mac trunk.

Comment 61

[Boris Zbarsky [:bzbarsky]]

The script CNN is loading has certainly been changed on the CDN.  It now restoring setters, not just getters.  And furthermore, it's now checking for each getter/setter/method that isNativeFunction doesn't test true before nuking it.  And isNativeFunction is defined like so:

    optly.nativity.isNativeFunction = function (a) {
        return a && -1 !== String(a).indexOf("[native code]")
    };

which happens to be true for our functions, so the script is no longer messing with them.

So this might be fixed.  But it's still tracking+; I don't know why that flag was removed.

The site in bug 828767 is still getting the old script version, though.  Maybe it hasn't propagated to their whole CDN....

Comment 62

[Boris Zbarsky [:bzbarsky]]

OK, the optimizely folks say they've deployed a fix.  Apparently sites still need to do something to update to it on their end.  cnn has done this, but clymb has not.  Going to see whether they can somehow push this out to people...

Comment 63

[Manuela Muntean [Away]]

I can't reproduce this issue with the Nightly from 2012-12-21 on a Windows 7 64-bit machine.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121221 Firefox/20.0
Build ID: 20121221030826

The websites work fine, I don't get any errors in the Error Console.
The test case from comment 17 also doesn't show any errors in the Error Console.
With the test case from comment 46, I can see the slide show.

Any suggestions would be appreciated.

Comment 64

[Boris Zbarsky [:bzbarsky]]

This is an evangelism bug.  That means that there were no changes on our end to fix; the site needed fixing.

If the site works fine, then the bug is fixed.

Comment 65

[Manuela Muntean [Away]]

(In reply to Boris Zbarsky (:bz) from comment #64)
> This is an evangelism bug.  That means that there were no changes on our end
> to fix; the site needed fixing.
> 
> If the site works fine, then the bug is fixed.

Ok, thank you Boris! Taking this into consideration, I'm marking this issue as verified, because the site works properly with Firefox 20 beta 1, build ID: 20130220104816.