Bug 11151 – CNN site hangs when loading on mobile (low CPU) device

• Component: Layout and Rendering
• Status: RESOLVED CONFIGURATION CHANGED

[David Carson]

CNN checks for support of innerHTML by calling it on the body, resulting in the entire document being serialized. It does not use the resultant string! From main.js loaded by cnn site:
      if(document.body && document.body.innerHTML && cnnUseDelayedCSI)

[David Carson]

Created attachment 10899 [details]
patch

[David Carson]

Created attachment 10902 [details]
corrected ifdef

Corrected #ifdef MOBILE to #if MOBILE

[Timothy Hatcher]

Comment on attachment 10902 [details]
corrected ifdef

I think we can do this a better way. I recall we can make innerHTML just return true when it is used in a boolean compare like this. We did something similar for document.all to hide it.

[David Carson]

(In reply to comment #3)
> (From update of attachment 10902 [details] [edit])
> I think we can do this a better way. I recall we can make innerHTML just return
> true when it is used in a boolean compare like this. We did something similar
> for document.all to hide it.

I think that document.all is quite different to innerHTML as document.all returns a JS array object that represents all elements in the page. The array is not evaluated till someone tries to access it. It seems that all arrays do this. 
innerHTML provides a string (a variant with string value set) that is a serialized version of the contents of a DOM node. If innerHTML was to copy that behaviour all javascript string objects would need to be changed to not contain the value until it is accessed?

[David Carson]

IRC discussion:
dacarson: I am looking at http://bugs.webkit.org/show_bug.cgi?id=6268
[10:44am] andersca: dacarson: so I saw
[10:44am] dacarson: andersca: in relation to http://bugs.webkit.org/show_bug.cgi?id=11151
[10:44am] andersca: dacarson: so cnn.com does a check for innerHTML which serializes the document?
[10:44am] dacarson: yep,
[10:44am] dacarson: I copied the line into the bug report
[10:45am] pmax: that could probably be optimized away
[10:45am] dacarson: if(document.body && document.body.innerHTML && cnnUseDelayedCSI)
[10:45am] dacarson: pmax:  trying to 
[10:45am] andersca: I wonder if it is possible to make a proxy object which would be serialized when needed
[10:46am] dacarson: thats what I figure all the JS array objects are doing
[10:46am] pmax: exactly
[10:47am] andersca: ggaren added something similar to some svg filter code I think
[10:47am] andersca: ggaren?
[10:47am] ggaren: andersca, dacarson: are we trying to pretend that innerHTML doesn't exist?
[10:47am] dacarson: ggaren: not quite
[10:47am] ggaren: andersca, dacarson: or are we trying to make the check for its existence efficient, and not make it generate the value, just check for its existence?
[10:48am] andersca: the latter
[10:48am] • dacarson nods
[10:48am] ggaren: andersca, dacarson: in theory, WebCore should already do that
[10:48am] ggaren: andersca, dacarson: JavaScriptCore's getPropertySlot method enables you to test for existence without generating the underlying value
[10:49am] ggaren: andersca, dacarson: only when you call propertSlot.getValue() or whatever does the value actually get computed
[10:49am] dacarson: ggaren: so the line "if(document.body && document.body.innerHTML && cnnUseDelayedCSI)" will not serialize the document?
[10:49am] ggaren: dacarson: i'm not sure
[10:49am] andersca: Im sure it will
[10:50am] dacarson: I think it will call getPropertySlot to get the value to eval the if statement
[10:50am] ggaren: dacarson: let me look
[10:50am] ggaren: dacarson: what's the bug we're trying to fix?
[10:51am] dacarson: ggaren: 11151
[10:51am] dacarson: ggaren: http://bugs.webkit.org/show_bug.cgi?id=11151
[10:54am] ggaren: dacarson: the problem is KJS::BinaryLogicalNode::evaluate
[10:54am] ggaren: dacarson: (that's what a && b turns into in the JS parse tree)
[10:54am] dacarson: ggaren: yeah
[10:54am] ggaren: dacarson: you can see that it needs to evaluate its terms and then convert to boolean
[10:55am] ggaren: dacarson: imagine a statement like "if (a.doSomethingWithSideEffects() && b)"
[10:55am] ggaren: dacarson: JavaScriptCore has no way of knowing that it doesn't need to evaluate each term
[10:55am] ggaren: dacarson: the solution on the websites end would be to use the "in" operator
[10:56am] ggaren: dacarson: if ("innerHTML" in document.body)
[10:56am] ggaren: dacarson: that tells JS that it only needs to test for existence, not evaluate a value
[10:56am] ggaren: dacarson: a solution in WebCore would be a little tricky, but may be possible
[10:56am] dacarson: ggaren: could there also be a getter on the innerHTML
[10:57am] dacarson: ggaren: that would cause issue ?
[10:57am] ggaren: dacarson: I think WebCore would have to define a special subclass of KJS::StringImp
[10:57am] dacarson: ggaren: a proxy, as andersca suggested?
[10:58am] ggaren: dacarson: i think andersca was suggesting a proxy to fix the issue, not that a proxy was the cause
ggaren: dacarson: innerHTML cold generate a proxy object that only fetched the document's contents when converted to string or whatever
[10:58am] ggaren: dacarson: but simply return true when converted to boolean
[10:59am] dacarson: ggaren: yep - I think that is what andersca was getting at.

[Ahmad Saleem]

CNN Website has evolved and Safari / WebKit in general as well and have various performance improvements to tackle these.

Marking this as 'RESOLVED CONFIGURATION CHANGED'.