Bug 113831 – get_by_pname can become confused when iterating over objects with static properties

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

It doesn't take these into account when using a JSPropertyNameIterator to directly access the backing store. One way to fix this is to not cache any properties when iterating over objects with static properties.

[Mark Hahnenberg]

<rdar://problem/12035186>

[Mark Hahnenberg]

Created attachment 196243 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 196243 [details]
Patch

No testcase. You should also mention the actual site that this fixes.

[Oliver Hunt]

Comment on attachment 196243 [details]
Patch

Have you done perf tests on this?  Huge numbers of dom objects have static properties - it might be good to run dromaeo

[Geoffrey Garen]

It should be possible to include a test case here. I believe the bug report includes one.

[Geoffrey Garen]

> Have you done perf tests on this?  Huge numbers of dom objects have static properties - it might be good to run dromaeo

Since DOM objects only ever got the get_by_pname optimization for their custom properties, which are rare, we expect this change to have no performance effect. I agree that we should confirm using Dromaeo.

[Mark Hahnenberg]

Dromaeo results:

Without patch: http://dromaeo.com/?id=192872
With patch: http://dromaeo.com/?id=192876

[Mark Hahnenberg]

Created attachment 196366 [details]
Patch

[Geoffrey Garen]

Comment on attachment 196366 [details]
Patch

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

r=me

> LayoutTests/fast/js/dom-static-property-for-in-iteration-expected.txt:163
> +PASS true is true

This is a bit opaque.

> LayoutTests/fast/js/dom-static-property-for-in-iteration.html:18
> +                var actual = a[i];
> +                var expected = a["" + i];
> +                shouldBeTrue(String(actual === expected));

You'll get better test output if you do something like this: shouldBe('a[i]', 'a["" + i]').

[Mark Hahnenberg]

> > LayoutTests/fast/js/dom-static-property-for-in-iteration-expected.txt:163
> > +PASS true is true
> 
> This is a bit opaque.
> 
> > LayoutTests/fast/js/dom-static-property-for-in-iteration.html:18
> > +                var actual = a[i];
> > +                var expected = a["" + i];
> > +                shouldBeTrue(String(actual === expected));
> 
> You'll get better test output if you do something like this: shouldBe('a[i]', 'a["" + i]').

I originally did that, but I get the following error message in the test output:

CONSOLE MESSAGE: line 1: ReferenceError: Can't find variable: a

[Mark Hahnenberg]

> I originally did that, but I get the following error message in the test output:
> 
> CONSOLE MESSAGE: line 1: ReferenceError: Can't find variable: a

Nevermind, there were some nested double quotes inside the property value that were throwing things off. I got it working with shouldBe() now.

[Mark Hahnenberg]

Committed r147570: <http://trac.webkit.org/changeset/147570>

[Mark Hahnenberg]

Reopening to make the test better.

[Mark Hahnenberg]

Created attachment 196389 [details]
Patch

[Benjamin Poulain]

The test dom-static-property-for-in-iteration.html started failing after r147570 landed.

[Mark Hahnenberg]

(In reply to comment #15)
> The test dom-static-property-for-in-iteration.html started failing after r147570 landed.

Ahh, there was a system-specific path in there. I should change the test to avoid those properties.

[Mark Hahnenberg]

Created attachment 196410 [details]
Patch

[Mark Hahnenberg]

Committed r147585: <http://trac.webkit.org/changeset/147585>

[Zoltan Arvai]

On Qt port there is some difference between actual and expected results. Is it acceptable?

http://build.webkit.org/results/Qt%20Linux%20Release/r147616%20%2859023%29/fast/js/dom-static-property-for-in-iteration-pretty-diff.html

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/js/dom-static-property-for-in-iteration-expected.txt
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/js/dom-static-property-for-in-iteration-actual.txt
@@ -45,7 +45,7 @@
 PASS a["scrollWidth"] is 0
 PASS a["attributes"] is [object NamedNodeMap]
 PASS a["webkitRegionOverset"] is undefined
-PASS a["offsetWidth"] is 39
+PASS a["offsetWidth"] is 37
 PASS a["classList"] is 
 PASS a["offsetLeft"] is 8
 PASS a["className"] is 
@@ -59,9 +59,9 @@
 PASS a["scrollLeft"] is 0
 PASS a["firstElementChild"] is null
 PASS a["clientLeft"] is 0
-PASS a["offsetHeight"] is 18
+PASS a["offsetHeight"] is 19
 PASS a["clientHeight"] is 0
-PASS a["offsetTop"] is 954
+PASS a["offsetTop"] is 956
 PASS a["scrollTop"] is 0
 PASS a["scrollHeight"] is 0
 PASS a["previousSibling"] is [object Text]
@@ -85,7 +85,6 @@
 PASS a["hick"] is 4
 PASS a["hock"] is 5
 PASS a["snood"] is 6
-PASS a["ALLOW_KEYBOARD_INPUT"] is 1
 PASS a["NOTATION_NODE"] is 12
 PASS a["CDATA_SECTION_NODE"] is 4
 PASS a["ELEMENT_NODE"] is 1

[Zoltan Arvai]

Some addition information on Qt. On Qt WK2 only offset is different. Qt WK1 misses "PASS a["ALLOW_KEYBOARD_INPUT"] is 1".

http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r147616%20%2834444%29/fast/js/dom-static-property-for-in-iteration-pretty-diff.html

[Stephane Cerveau]

*** Bug 114036 has been marked as a duplicate of this bug. ***