Bug 9742 – REGRESSION: WebKit hangs when loading <http://www.vtbook.com>

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Troy Brandt]

This hang is 100% reproducible for me in WebKit 13302 and 15159, it does not appear to affect Safari on 10.4.7. Attaching a sample.

[Troy Brandt]

Created attachment 9209 [details]
Sample

[Geoffrey Garen]

Infinite for loop in JS. Not sure why.

[Alexey Proskuryakov]

Confirmed with TOT. Regression -> P1

[Joost de Valk (AlthA)]

My guess is the infinite loop occurs in: http://www.villagetronic.com/site/z-j-ntbcore.js perhaps someone could look at that?

[Mark Rowe (bdash)]

Created attachment 9427 [details]
Reduced test case

We seem to be erronously treating anArray[null] and anArray[0] as equivalent.  This regression occurred prior to r13093 (the oldest Universal nightly build).

[Mark Rowe (bdash)]

Created attachment 9428 [details]
Patch

This patch only allows immediate types tagged as numeric to be converted to UInt32.  This resolves the issue without affecting any JSCore or layout tests.

[Mark Rowe (bdash)]

Created attachment 9429 [details]
Updated patch including ChangeLog entry for the layout test

[Darin Adler]

Comment on attachment 9429 [details]
Updated patch including ChangeLog entry for the layout test

r=me

[Geoffrey Garen]

Comment on attachment 9429 [details]
Updated patch including ChangeLog entry for the layout test

Great catch!

Two objections:

1. JavaScript layout tests should be written as pure JavaScript, and the HTML to run them in DumpRenderTree generated by the /WebKitTools/Scripts/make-js-test-wrappers script. You can see example .js files in LayoutTests/fast/js/resources. The reason for this is that we want to be able to run these tests outside of the browser, too.

2. It looks like the other get functions, like getNumber, have the same issue. It would be best to change them too.

Note to commiter: We'll need to performance test the final patch before landing, since it may change the inlining behavior of some hot functions.

[Geoffrey Garen]

Created attachment 9433 [details]
patch for both cases, 1 less branch

OK, so I'm a little obsessive about value.h. Here's the patch I had in mind. Do you see how it eliminates a branch inside getUInt32? My testing says this patch is not a performance regression, but the alternative would be about a 1% regression.

I'm pretty swamped right now. Do you think you could package this up w/ChangeLog + js-style layout test?

[Darin Adler]

(In reply to comment #9)
> 2. It looks like the other get functions, like getNumber, have the same issue.
> It would be best to change them too.

Are you sure getNumber needs this change? getUInt32 is special, used for the "numeric property" optimization. That's why it doesn't want to convert non-numeric values. Is that right for getNumber? What test demonstrates that?

[Mark Rowe (bdash)]

As far as I can see the only use of getNumber(double& v) is in WebCore from JSHTMLOptionsCollection::setLength.  Making this change causes a regression in the behaviour of setLength -- when called with boolean true as its argument, it incorrectly truncates the length of the options collection to zero rather than one.   For this reason, I think the change to getNumber is not required.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> As far as I can see the only use of getNumber(double& v) is in WebCore from
> JSHTMLOptionsCollection::setLength.  Making this change causes a regression in
> the behaviour of setLength -- when called with boolean true as its argument, it
> incorrectly truncates the length of the options collection to zero rather than
> one.   For this reason, I think the change to getNumber is not required.

The JSHTMLOptionsCollection::setLength method originally called value->isNumber() before it called value->getNumber() (see Bug 9179 Comment #17).  Does it need to be changed back (if it's the only reason this change can't be made)?

[Mark Rowe (bdash)]

Created attachment 9437 [details]
Patch

Updated patch that uses Geoffrey's suggested change to getUInt32.  This omits the change to getNumber for the reasons stated in comment  #12.  I have also updated two existing tests related to JSHTMLOptionsCollection::setLength to ensure that regressions such as the one the getNumber change would have introduced are noticed in future.

[Geoffrey Garen]

> Are you sure getNumber needs this change? getUInt32 is special, used for the
> "numeric property" optimization. That's why it doesn't want to convert
> non-numeric values. Is that right for getNumber? What test demonstrates that?

I think it needs to change as a matter of API consistency, not a matter of behavior. Our "get" functions are dinstinguished from our "to" functions in that they are quicker/simpler, but they fail if you try to "get" a type from a value that is not of that type. In fact, getNumber is documented as " NaN if not a number."

So we should change the getNumber caller, too. I see now that's probably best as a separate patch, though.

[Geoffrey Garen]

Comment on attachment 9437 [details]
Patch

These tests fill me with warm fuzzies.

[David Kilzer (:ddkilzer)]

Committed revision 15417.

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> I think it needs to change as a matter of API consistency, not a matter of
> behavior. Our "get" functions are dinstinguished from our "to" functions in
> that they are quicker/simpler, but they fail if you try to "get" a type from a
> value that is not of that type. In fact, getNumber is documented as " NaN if
> not a number."
> 
> So we should change the getNumber caller, too. I see now that's probably best
> as a separate patch, though.

Filed Bug 9903 for this follow-up issue.