[webkit-reviews] review denied: [Bug 125377] REGRESSION: 2x regression on Dromaeo DOM query tests : [Attachment 219286] Fixes the bug

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 125377: REGRESSION: 2x regression on Dromaeo DOM query tests
https://bugs.webkit.org/show_bug.cgi?id=125377

Attachment 219286: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=219286&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219286&action=review


R=me on the code changes but you should make the tests do the right thing for
concurrent JIT.  Otherwise the DFG tests will turn into flaky tests, instead of
failing reliably, if someone breaks this again in the future.  We don't want
flaky tests.

>
LayoutTests/js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slo
t-traps-2.html:25
> +description("Tests what happens when you make prototype chain accesses with
impure GetOwnPropertySlot traps in the way.");
> +
> +var obj = {};
> +obj.__proto__ = document;
> +
> +function f() {
> +    return obj.getElementsByTagName;
> +}
> +
> +var expected = "\"function\"";
> +for (var i = 0; i < 400; ++i) {
> +    if (i == 350) {
> +	   var img = new Image();
> +	   img.name = "getElementsByTagName";
> +	   document.body.appendChild(img);
> +	   expected = "\"object\"";
> +    }
> +    shouldBe("typeof f()", expected);
> +}

Can you rewrite these tests to use dfgIncrement?  That will make them relevant
even in the case of concurrent JIT.

Or you can use dfgShouldBe().

>
LayoutTests/js/dom/dfg-prototype-chain-caching-with-impure-get-own-property-slo
t-traps-4.html:26
> +var expected = "\"function\"";
> +for (var i = 0; i < 400; ++i) {
> +    if (i == 350) {
> +	   var img = new Image();
> +	   img.name = "foo";
> +	   document.body.appendChild(img);
> +	   expected = "\"object\"";
> +    }
> +    shouldBe("typeof f()", expected);
> +}

Ditto.

>
LayoutTests/js/dom/prototype-chain-caching-with-impure-get-own-property-slot-tr
aps-2.html:25
> +var expected = "\"function\"";
> +for (var i = 0; i < 40; ++i) {
> +    if (i == 35) {
> +	   var img = new Image();
> +	   img.name = "getElementsByTagName";
> +	   document.body.appendChild(img);
> +	   expected = "\"object\"";
> +    }
> +    shouldBe("typeof f()", expected);
> +}

Ditto.