|Summary:||get_by_pname can become confused when iterating over objects with static properties|
|Product:||WebKit||Reporter:||Mark Hahnenberg <mhahnenberg>|
|Severity:||Normal||CC:||benjamin, ddkilzer, ggaren, kadam, scerveau, zarvai|
|Version:||528+ (Nightly build)|
|Bug Depends on:||114157|
Description Mark Hahnenberg 2013-04-02 15:37:06 PDT
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.
Comment 3 Simon Fraser (smfr) 2013-04-02 15:44:48 PDT
Comment on attachment 196243 [details] Patch No testcase. You should also mention the actual site that this fixes.
Comment 4 Oliver Hunt 2013-04-02 15:45:42 PDT
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
Comment 5 Geoffrey Garen 2013-04-02 15:52:34 PDT
It should be possible to include a test case here. I believe the bug report includes one.
Comment 6 Geoffrey Garen 2013-04-02 15:53:41 PDT
> 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.
Comment 7 Mark Hahnenberg 2013-04-02 19:20:42 PDT
Dromaeo results: Without patch: http://dromaeo.com/?id=192872 With patch: http://dromaeo.com/?id=192876
Comment 9 Geoffrey Garen 2013-04-03 10:42:40 PDT
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]').
Comment 10 Mark Hahnenberg 2013-04-03 10:45:51 PDT
> > 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
Comment 11 Mark Hahnenberg 2013-04-03 11:23:07 PDT
> 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.
Comment 12 Mark Hahnenberg 2013-04-03 11:36:56 PDT
Committed r147570: <http://trac.webkit.org/changeset/147570>
Comment 13 Mark Hahnenberg 2013-04-03 12:25:29 PDT
Reopening to make the test better.
Comment 15 Benjamin Poulain 2013-04-03 14:06:25 PDT
The test dom-static-property-for-in-iteration.html started failing after r147570 landed.
Comment 16 Mark Hahnenberg 2013-04-03 14:09:00 PDT
(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.
Comment 18 Mark Hahnenberg 2013-04-03 14:33:38 PDT
Committed r147585: <http://trac.webkit.org/changeset/147585>
Comment 19 Zoltan Arvai 2013-04-04 04:03:55 PDT
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
Comment 20 Zoltan Arvai 2013-04-04 04:07:47 PDT
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