REGRESSION (r174226): Header on huffingtonpost.com is too large

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

* STEPS TO REPRODUCE
1. Load http://www.huffingtonpost.com

* RESULTS
The header is supposed to stay within the white background but it overflows into the grey background now.

It looks like we get an extra "LifeStyle" section in the header compared to before r174226.

rdar://problem/19243435

[Michael Saboff]

Created attachment 244369 [details]
Reduced Test Case

[Michael Saboff]

Created attachment 244447 [details]
Patch

[Mark Lam]

Comment on attachment 244447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244447&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        Changed BytecodeGenerator::willResolveToArguments() to check to see if the arguments
> +        register is captured.  If so, we can't use the arguments register, we have to resolve
> +        arguments.  Renamed the function to willResolveToArgumentsRegister() to better indicate
> +        what we are checking.

This explains what was done, but doesn’t say why.  Maybe it should be obvious, but I’m missing the detail of why we can’t use the value in the argument register.  By “resolve arguments”, I presume you mean resolving arguments from the captured lexical environment.  If so, why do we need to do that?  Why is the captured arguments object pointer different than that in the arguments register?  Thanks.

[Mark Lam]

Comment on attachment 244447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244447&action=review

> LayoutTests/js/script-tests/arguments-iterator.js:-95
> -function testOverwrittenArguments() {
> -    var i = 0;
> -    arguments = "foobar";
> -    for (arg of arguments) {
> -        realArg = arguments[i++];
> -        shouldBeTrue("arg === realArg");
> -    }
> -    iteratedArgumentsLength = i;
> -    actualArgumentsLength = arguments.length;
> -    shouldBe("actualArgumentsLength", "iteratedArgumentsLength");
> -}
> -
> -
> -testOverwrittenArguments();
> -testOverwrittenArguments("a");
> -testOverwrittenArguments("a", "b");
> -testOverwrittenArguments({})

Why remove these tests?  Can you say something in the ChangeLog comment that explains why they are not needed or inappropriate?

> LayoutTests/js/script-tests/arguments-iterator.js:-117
> -function testNonArrayLikeArguments() {
> -    var i = 0;
> -    arguments = {};
> -    for (arg of arguments) {
> -        fail("nothing to iterate");
> -        return false;
> -    }
> -    return true;
> -}
> -shouldBeTrue("testNonArrayLikeArguments()");

Ditto.  An explanation of we can/should remove this test would be helpful.

[Michael Saboff]

Created attachment 244496 [details]
Improved the ChangeLog comments as requested.

[Mark Lam]

Comment on attachment 244496 [details]
Improved the ChangeLog comments as requested.

View in context: https://bugs.webkit.org/attachment.cgi?id=244496&action=review

I’m uncomfortable with the removal of the tests, but other than that, LGTM with the comment clarification.

> Source/JavaScriptCore/ChangeLog:11
> +        BytecodeGenerator::willResolveToArguments() is used to check to see if we can use the
> +        arguments register or whether we need to resolve "arguments".  If the arguments have
> +        been captured, then they are stored in the lexical environment and the arguments
> +        register is not used.

I think this comment is unclear in that the issue is concerned about “arguments” being overloaded as a var, but the comment can be easily read as if it’s talking about whether the incoming parameters (e.g. a, b, c in function foo(a, b, c)) are captured.  How about rephrasing it like this:

We need to check if "arguments” has been overloaded in a function to define a local variable that is captured.  If it is overloaded and is captured, then its value should be retrieved from the lexicalEnvironment rather than the arguments register.  The arguments register still points to the Arguments object (if present) and not the variable it is overloaded with.

> LayoutTests/js/script-tests/arguments-iterator.js:-95
> -function testOverwrittenArguments() {
> -    var i = 0;
> -    arguments = "foobar";
> -    for (arg of arguments) {
> -        realArg = arguments[i++];
> -        shouldBeTrue("arg === realArg");
> -    }
> -    iteratedArgumentsLength = i;
> -    actualArgumentsLength = arguments.length;
> -    shouldBe("actualArgumentsLength", "iteratedArgumentsLength");
> -}
> -
> -
> -testOverwrittenArguments();
> -testOverwrittenArguments("a");
> -testOverwrittenArguments("a", "b");
> -testOverwrittenArguments({})

Thanks for the clarification on why you removed these.  However, I’m not sold on the given reason.  From what I can see, it does make sense that if you overload “arguments” with a local pointing to a string, you should be able to iterate it.  Instead, I think the real issue here is that the test output could be expressed better, but is a bit misleading right now.  Regardless, I think there is utility in a test confirming that “arguments” iteration in the function should work as tested if “arguments” is overloaded to point to a string as in this test.  As a result, I’m not comfortable removing this test.  Can you seek a second opinion on this please?

> LayoutTests/js/script-tests/arguments-iterator.js:-117
> -function testNonArrayLikeArguments() {
> -    var i = 0;
> -    arguments = {};
> -    for (arg of arguments) {
> -        fail("nothing to iterate");
> -        return false;
> -    }
> -    return true;
> -}
> -shouldBeTrue("testNonArrayLikeArguments()");

Ditto.  I think there is utility in testing what happens if “arguments” is overloaded to point to an empty object.  Please seek a second opinion on this removal.

[Michael Saboff]

(In reply to comment #6)
> Comment on attachment 244496 [details]
> Improved the ChangeLog comments as requested.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244496&action=review
> 
> I’m uncomfortable with the removal of the tests, but other than that, LGTM
> with the comment clarification.
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        BytecodeGenerator::willResolveToArguments() is used to check to see if we can use the
> > +        arguments register or whether we need to resolve "arguments".  If the arguments have
> > +        been captured, then they are stored in the lexical environment and the arguments
> > +        register is not used.
> 
> I think this comment is unclear in that the issue is concerned about
> “arguments” being overloaded as a var, but the comment can be easily read as
> if it’s talking about whether the incoming parameters (e.g. a, b, c in
> function foo(a, b, c)) are captured.  How about rephrasing it like this:
> 
> We need to check if "arguments” has been overloaded in a function to define
> a local variable that is captured.  If it is overloaded and is captured,
> then its value should be retrieved from the lexicalEnvironment rather than
> the arguments register.  The arguments register still points to the
> Arguments object (if present) and not the variable it is overloaded with.

The only change to willResolveToArguments() in this patch is to check if "arguments" was captured.  There already is a check for "overloading" of arguments by checking m_localArgumentsRegister which is set to null if any individual argument or "arguments" is overloaded.  If it is captured, the lexical environment is the where we get or set the value of arguments.  That is what is meant by resolve.  If any individual argument or "arguments" is overloaded, the argument register is set to null by the byte code generator and cannot be used.

> > LayoutTests/js/script-tests/arguments-iterator.js:-95
> > -function testOverwrittenArguments() {
> > -    var i = 0;
> > -    arguments = "foobar";
> > -    for (arg of arguments) {
> > -        realArg = arguments[i++];
> > -        shouldBeTrue("arg === realArg");
> > -    }
> > -    iteratedArgumentsLength = i;
> > -    actualArgumentsLength = arguments.length;
> > -    shouldBe("actualArgumentsLength", "iteratedArgumentsLength");
> > -}
> > -
> > -
> > -testOverwrittenArguments();
> > -testOverwrittenArguments("a");
> > -testOverwrittenArguments("a", "b");
> > -testOverwrittenArguments({})
> 
> Thanks for the clarification on why you removed these.  However, I’m not
> sold on the given reason.  From what I can see, it does make sense that if
> you overload “arguments” with a local pointing to a string, you should be
> able to iterate it.  Instead, I think the real issue here is that the test
> output could be expressed better, but is a bit misleading right now. 
> Regardless, I think there is utility in a test confirming that “arguments”
> iteration in the function should work as tested if “arguments” is overloaded
> to point to a string as in this test.  As a result, I’m not comfortable
> removing this test.  Can you seek a second opinion on this please?

The issue is that you can't iterate over a string with for .. of.  How about I put the "change arguments to a string" test back in and change the test to check for a string, but not iterate it?   

> > LayoutTests/js/script-tests/arguments-iterator.js:-117
> > -function testNonArrayLikeArguments() {
> > -    var i = 0;
> > -    arguments = {};
> > -    for (arg of arguments) {
> > -        fail("nothing to iterate");
> > -        return false;
> > -    }
> > -    return true;
> > -}
> > -shouldBeTrue("testNonArrayLikeArguments()");
> 
> Ditto.  I think there is utility in testing what happens if “arguments” is
> overloaded to point to an empty object.  Please seek a second opinion on
> this removal.

I can also put the test back in and check that arguments is an empty object, but not iterate with for .. of?

[Geoffrey Garen]

> The issue is that you can't iterate over a string with for .. of.  How about
> I put the "change arguments to a string" test back in and change the test to
> check for a string, but not iterate it?   

In that case, the best change is to change the item being iterated not to be a string.

We want to continue to use for-of because that's the only way to test the codegen code path that covers variable resolution in for-of ().

> I can also put the test back in and check that arguments is an empty object,
> but not iterate with for .. of?

Why can't we iterate with for-of in this case?

[Michael Saboff]

(In reply to comment #8)
> > The issue is that you can't iterate over a string with for .. of.  How about
> > I put the "change arguments to a string" test back in and change the test to
> > check for a string, but not iterate it?   
> 
> In that case, the best change is to change the item being iterated not to be
> a string.
> 
> We want to continue to use for-of because that's the only way to test the
> codegen code path that covers variable resolution in for-of ().

I'll change to an array.
 
> > I can also put the test back in and check that arguments is an empty object,
> > but not iterate with for .. of?
> 
> Why can't we iterate with for-of in this case?

For this test code:

function testEmptyObjectIteration() {
    var myObj = {};

    for (v of myObj) {
        fail("nothing to iterate");
        return false;
    }

    return true;
}

WebKit returns: FAIL testEmptyObjectIteration() should be true. Threw exception TypeError: undefined is not a function (evaluating 'v of myObj')

Chrome returns: FAIL testEmptyObjectIteration() should be true. Threw exception TypeError: undefined is not a function

Firefox returns: FAIL testEmptyObjectIteration() should be true. Threw exception TypeError: myObj['@@iterator'] is not a function

Doesn't look like the object returns an iterator.

[Geoffrey Garen]

> > > I can also put the test back in and check that arguments is an empty object,
> > > but not iterate with for .. of?

Seems like it would be more in the spirit of this test to iterate something iterable and empty with for-of -- for example, [].

[Michael Saboff]

Created attachment 244580 [details]
Updated arguments-iterator test to overwrite arguments with arrays

[Geoffrey Garen]

Comment on attachment 244580 [details]
Updated arguments-iterator test to overwrite arguments with arrays

r=me

[Michael Saboff]

Committed r178432: <http://trac.webkit.org/changeset/178432>

[Joseph Pecoraro]

Looks like this caused a lot of failures on the bots:
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/6701

> ** The following JSC stress test failures have been introduced:
> 	stress/inline-call-varargs-and-call.js.always-trigger-copy-phase
> 	stress/inline-call-varargs-and-call.js.default
> 	stress/inline-call-varargs-and-call.js.default-ftl
> 	stress/inline-call-varargs-and-call.js.dfg-eager
> 	stress/inline-call-varargs-and-call.js.dfg-eager-no-cjit-validate
> 	stress/inline-call-varargs-and-call.js.ftl-eager
> 	stress/inline-call-varargs-and-call.js.ftl-eager-no-cjit
> 	stress/inline-call-varargs-and-call.js.ftl-no-cjit-validate
> 	stress/inline-call-varargs-and-call.js.no-cjit-validate-phases
> 	stress/inline-call-varargs-and-call.js.no-llint
> 	stress/inline-call-varargs.js.always-trigger-copy-phase
> 	stress/inline-call-varargs.js.default
> 	stress/inline-call-varargs.js.default-ftl
> 	stress/inline-call-varargs.js.ftl-no-cjit-validate
> 	stress/inline-call-varargs.js.no-cjit-validate-phases
> 	stress/inline-call-varargs.js.no-llint
> 	v8-v6/v8-raytrace.js.dfg-eager
> 	v8-v6/v8-raytrace.js.dfg-eager-no-cjit-validate
> 	v8-v6/v8-raytrace.js.ftl-eager
> 	v8-v6/v8-raytrace.js.ftl-eager-no-cjit
> 
> Results for JSC stress tests:
>     20 failures found.

Should I roll this out?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 140460

[Joseph Pecoraro]

Rolled this out in r178435 (Michael Saboff is on vacation today).

[Michael Saboff]

Created attachment 244734 [details]
Updated patch with 32 bit build fixes

[Michael Saboff]

Committed r178591: <http://trac.webkit.org/changeset/178591>

[Alexey Proskuryakov]

This made two tests time out on Mac Debug:
js/slow-stress/call-spread.html
js/slow-stress/new-spread.html

And also it looks like Windows Release crashes on call-spread.html flakily after this patch (as it did when the previous version was landed).

[Andreas Kling]

Looks like this may have caused a ~3% regression on the Speedometer benchmark. The slowdown was caught by all the bots on perf.webkit.org.

This is the revision range:
<http://trac.webkit.org/log/?rev=178605&stop_rev=178589&verbose=on>

[Ryosuke Niwa]

(In reply to comment #20)
> Looks like this may have caused a ~3% regression on the Speedometer
> benchmark. The slowdown was caught by all the bots on perf.webkit.org.
> 
> This is the revision range:
> <http://trac.webkit.org/log/?rev=178605&stop_rev=178589&verbose=on>

Combining two graphs:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%2C%5B%22mac-yosemite%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D&days=29&zoom=%5B1421202788589.2793%2C1421592110327.5376%5D

yields even a smaller blame list:
http://trac.webkit.org/log/?rev=178592&stop_rev=178589&verbose=on

[Chris Dumez]

(In reply to comment #21)
> (In reply to comment #20)
> > Looks like this may have caused a ~3% regression on the Speedometer
> > benchmark. The slowdown was caught by all the bots on perf.webkit.org.
> > 
> > This is the revision range:
> > <http://trac.webkit.org/log/?rev=178605&stop_rev=178589&verbose=on>
> 
> Combining two graphs:
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-
> mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%2C%5B%22mac-
> yosemite%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D&days=29&zoom=%
> 5B1421202788589.2793%2C1421592110327.5376%5D
> 
> yields even a smaller blame list:
> http://trac.webkit.org/log/?rev=178592&stop_rev=178589&verbose=on

Perf regression was fixed in <http://trac.webkit.org/changeset/179202>.