Bug 113831 - get_by_pname can become confused when iterating over objects with static properties
Summary: get_by_pname can become confused when iterating over objects with static prop...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
: 114036 (view as bug list)
Depends on: 114157
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-02 15:37 PDT by Mark Hahnenberg
Modified: 2013-06-19 01:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.52 KB, patch)
2013-04-02 15:42 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (10.20 KB, patch)
2013-04-03 09:59 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (9.13 KB, patch)
2013-04-03 12:28 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (8.80 KB, patch)
2013-04-03 14:30 PDT, Mark Hahnenberg
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 1 Mark Hahnenberg 2013-04-02 15:39:31 PDT
<rdar://problem/12035186>
Comment 2 Mark Hahnenberg 2013-04-02 15:42:19 PDT
Created attachment 196243 [details]
Patch
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 8 Mark Hahnenberg 2013-04-03 09:59:06 PDT
Created attachment 196366 [details]
Patch
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 14 Mark Hahnenberg 2013-04-03 12:28:07 PDT
Created attachment 196389 [details]
Patch
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 17 Mark Hahnenberg 2013-04-03 14:30:17 PDT
Created attachment 196410 [details]
Patch
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
Comment 21 Stephane Cerveau 2013-06-19 01:38:23 PDT
*** Bug 114036 has been marked as a duplicate of this bug. ***