Bug 140306 - REGRESSION (r174226): Header on huffingtonpost.com is too large
Summary: REGRESSION (r174226): Header on huffingtonpost.com is too large
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on: 140460
Blocks: 136869
  Show dependency treegraph
 
Reported: 2015-01-09 13:43 PST by Michael Saboff
Modified: 2015-02-26 17:19 PST (History)
9 users (show)

See Also:


Attachments
Reduced Test Case (2.42 KB, patch)
2015-01-09 13:48 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch (15.50 KB, patch)
2015-01-12 08:43 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Improved the ChangeLog comments as requested. (15.93 KB, patch)
2015-01-12 22:48 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Updated arguments-iterator test to overwrite arguments with arrays (16.51 KB, patch)
2015-01-13 21:34 PST, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff
Updated patch with 32 bit build fixes (21.14 KB, patch)
2015-01-15 17:11 PST, Michael Saboff
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2015-01-09 13:43:33 PST
* 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
Comment 1 Michael Saboff 2015-01-09 13:48:59 PST
Created attachment 244369 [details]
Reduced Test Case
Comment 2 Michael Saboff 2015-01-12 08:43:52 PST
Created attachment 244447 [details]
Patch
Comment 3 Mark Lam 2015-01-12 09:40:13 PST
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.
Comment 4 Mark Lam 2015-01-12 09:50:31 PST
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.
Comment 5 Michael Saboff 2015-01-12 22:48:25 PST
Created attachment 244496 [details]
Improved the ChangeLog comments as requested.
Comment 6 Mark Lam 2015-01-13 10:26:41 PST
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.
Comment 7 Michael Saboff 2015-01-13 11:06:56 PST
(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?
Comment 8 Geoffrey Garen 2015-01-13 11:31:48 PST
> 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?
Comment 9 Michael Saboff 2015-01-13 12:03:39 PST
(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.
Comment 10 Geoffrey Garen 2015-01-13 12:40:24 PST
> > > 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, [].
Comment 11 Michael Saboff 2015-01-13 21:34:49 PST
Created attachment 244580 [details]
Updated arguments-iterator test to overwrite arguments with arrays
Comment 12 Geoffrey Garen 2015-01-14 11:03:55 PST
Comment on attachment 244580 [details]
Updated arguments-iterator test to overwrite arguments with arrays

r=me
Comment 13 Michael Saboff 2015-01-14 11:38:17 PST
Committed r178432: <http://trac.webkit.org/changeset/178432>
Comment 14 Joseph Pecoraro 2015-01-14 12:00:18 PST
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?
Comment 15 WebKit Commit Bot 2015-01-14 12:09:24 PST
Re-opened since this is blocked by bug 140460
Comment 16 Joseph Pecoraro 2015-01-14 12:17:24 PST
Rolled this out in r178435 (Michael Saboff is on vacation today).
Comment 17 Michael Saboff 2015-01-15 17:11:44 PST
Created attachment 244734 [details]
Updated patch with 32 bit build fixes
Comment 18 Michael Saboff 2015-01-16 12:40:18 PST
Committed r178591: <http://trac.webkit.org/changeset/178591>
Comment 19 Alexey Proskuryakov 2015-01-16 15:57:11 PST
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).
Comment 20 Andreas Kling 2015-01-21 17:32:23 PST
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>
Comment 21 Ryosuke Niwa 2015-01-31 21:51:36 PST
(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
Comment 22 Chris Dumez 2015-02-26 17:19:23 PST
(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>.