Bug 29005 - The values of RuntimeArray are not enumerable
Summary: The values of RuntimeArray are not enumerable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 29008
  Show dependency treegraph
 
Reported: 2009-09-07 03:36 PDT by Benjamin Poulain
Modified: 2009-12-11 07:22 PST (History)
8 users (show)

See Also:


Attachments
patch suggestion (1.89 KB, patch)
2009-09-07 03:48 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
patch suggestion (1.88 KB, patch)
2009-09-07 07:18 PDT, Benjamin Poulain
ggaren: review-
Details | Formatted Diff | Diff
patch suggestion (6.38 KB, patch)
2009-09-16 05:45 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
proposed patch (6.35 KB, patch)
2009-09-17 01:37 PDT, Benjamin Poulain
oliver: review-
Details | Formatted Diff | Diff
Patch (6.44 KB, patch)
2009-12-07 02:50 PST, Benjamin Poulain
darin: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2009-12-11 03:58 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2009-12-11 04:17 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2009-09-07 03:36:48 PDT
RuntimeArray does not behave like a regular JSArray regarding the listing of its properties. The indices of the array are not enumerated.
Comment 1 Benjamin Poulain 2009-09-07 03:48:35 PDT
Created attachment 39138 [details]
patch suggestion

I am not sure how I could do the test for this since I need a RuntimeArray in the runtime.
Comment 2 Benjamin Poulain 2009-09-07 07:18:51 PDT
Created attachment 39144 [details]
patch suggestion

Change the for() loop to follow the coding convention.
Comment 3 Geoffrey Garen 2009-09-08 14:06:51 PDT
Comment on attachment 39144 [details]
patch suggestion

Code looks good.

You can test RuntimeArray by passing an NSArray * from Objective-C to JavaScript. The best place to do that is probably inside DumpRenderTree's ObjcController.

r- because there's no regression test.
Comment 4 Benjamin Poulain 2009-09-16 05:45:39 PDT
Created attachment 39641 [details]
patch suggestion

The same patch with a test for Objective-C.
Comment 5 Eric Seidel (no email) 2009-09-16 17:21:24 PDT
Style is wrong here:
 46     virtual void getPropertyNames(ExecState* exec, PropertyNameArray &propertyNames);
(yes, it's clearly wrong in other parts of the file too.)  No need for either argument name (they don't add clarity) and the & goes next to the type.

Doesn't Obj-C transparently convert JS arrays to Obj-C arrays?  Would it make more sense to have a more generic testing method which converted whatever was passed in to Obj-C and then back to a string on output?
Comment 6 Kenneth Rohde Christiansen 2009-09-16 17:28:17 PDT
+    for (unsigned i = 0; i < length; ++i) {
+        propertyNames.add(Identifier::from(exec, i));
+    }

This part should not have braces.

 46     virtual void getPropertyNames(ExecState* exec, PropertyNameArray
&propertyNames);

This is similar to what is already used in that file, so maybe it should be changed in a follow-up, clean-up patch?
Comment 7 Benjamin Poulain 2009-09-17 01:37:23 PDT
Created attachment 39685 [details]
proposed patch

Same patch, same test.
Fixed the code to follow the coding convention (thanks Eric and Kenneth).
Comment 8 Benjamin Poulain 2009-09-17 01:42:55 PDT
(In reply to comment #5)
> Doesn't Obj-C transparently convert JS arrays to Obj-C arrays?  Would it make
> more sense to have a more generic testing method which converted whatever was
> passed in to Obj-C and then back to a string on output?

The NSArray is "converted" to RuntimeArray in convertObjcValueToValue(). The purpose of the test is to use RuntimeArray in a for() loop of Javascript.
Comment 9 Eric Seidel (no email) 2009-09-23 17:17:32 PDT
Comment on attachment 39685 [details]
proposed patch

I still think we could write a smarter test.  But the code change looks OK to me.  Did you mean to set commit-queue? or do you have other plans for how this should be committed?
Comment 10 Simon Hausmann 2009-09-24 02:46:20 PDT
Comment on attachment 39685 [details]
proposed patch

Schedule for cq, as correctly suggested by Eric :)
Comment 11 WebKit Commit Bot 2009-09-24 03:42:54 PDT
Comment on attachment 39685 [details]
proposed patch

Clearing flags on attachment: 39685

Committed r48712: <http://trac.webkit.org/changeset/48712>
Comment 12 WebKit Commit Bot 2009-09-24 03:42:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Oliver Hunt 2009-09-24 03:57:25 PDT
Comment on attachment 39685 [details]
proposed patch

It is incorrect to override getPropertyNames in almost any case.  You should be overriding getOwnPropertyNames.
Comment 14 Oliver Hunt 2009-09-24 03:59:57 PDT
I rolled this patch out in r48712 due to it being incorrect
Comment 15 Eric Seidel (no email) 2009-09-24 11:11:04 PDT
Thank you oliver.
Comment 16 Benjamin Poulain 2009-12-07 02:50:23 PST
Created attachment 44394 [details]
Patch

Exactly the same patch as before, but use getOwnPropertyNames() instead of qetPropertyNames().

Oliver, could you please describe in which cases qetPropertyNames() is appropriate?
Comment 17 WebKit Review Bot 2009-12-07 02:55:29 PST
style-queue ran check-webkit-style on attachment 44394 [details] without any errors.
Comment 18 Darin Adler 2009-12-07 09:40:36 PST
(In reply to comment #16)
> Oliver, could you please describe in which cases getPropertyNames() is
> appropriate?

I'm not sure this is an entirely pertinent question, but I'll try to answer.

The getPropertyNames function is a loop that calls getOwnPropertyNames on all the objects in the prototype chain.

To add properties, it would never be good to override getPropertyNames, because it would create properties that you can see when enumerating the entire chain, but not when looking at an individual object with the getOwnPropertyNames code path.

The overrides of getPropertyNames inside the bridge directory in classes like RuntimeObjectImp, CInstance, and QTInstance are almost certainly all indicative of bugs that should be fixed.

The code in RuntimeObjectImp::getPropertyNames should probably be moved to RuntimeObjectImp::getOwnPropertyNames, and in fact the code in the latter looks like it will just cause an infinite loop and stack exhaustion. The trick here is finding a way to test.

Similar with CInstance::getPropertyNames and QtInstance::getPropertyNames.

JSDOMWindow overrides getPropertyNames because it wants to control access to the properties of its prototypes. That’s probably OK. It's a workaround for the fact that the call to get the prototype inside the getPropertyNames function doesn't have a convenient hook to allow the object to control access to the prototype.

Long term we might decide that getPropertyNames shouldn't even be a virtual function.
Comment 19 Darin Adler 2009-12-07 09:42:33 PST
Comment on attachment 44394 [details]
Patch

> +    const unsigned int length = getLength();

On the next line of code you use "unsigned" but here you used "unsigned int".

Further, you mark this local variable "const" and we don't do that in WebKit. There are so many that could be marked const, and I don't think we want to start heading that direction now.

r=me despite this slight strangeness
Comment 20 Eric Seidel (no email) 2009-12-09 14:02:00 PST
Comment on attachment 44394 [details]
Patch

Darin has suggested revisions.  I think we should see an update patch if we're going to cq this.
Comment 21 Benjamin Poulain 2009-12-11 03:58:55 PST
Created attachment 44665 [details]
Patch

Sorry for the delay.
I have updated the patch to change the "unsigned int" to "unsigned" and to remove the "const" keyword.
I have also put getOwnPropertyNames() with the other getOwnPropertyXXX() in the header to group the related functions.
Comment 22 WebKit Review Bot 2009-12-11 04:03:30 PST
Attachment 44665 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bridge/runtime_array.cpp:68:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1
Comment 23 Benjamin Poulain 2009-12-11 04:05:32 PST
Thanks Darin for the explanation. I had totally misunderstood the purpose of getPropertyNames().

I will open a bug report for the problem of QtInstance.
Comment 24 Benjamin Poulain 2009-12-11 04:17:53 PST
Created attachment 44667 [details]
Patch

Sorry for the delay.
I have updated the patch to change the "unsigned int" to "unsigned" and to
remove the "const" keyword.
I have also put getOwnPropertyNames() with the other getOwnPropertyXXX() in the
header to group the related functions.

+ removed a \t for my misconfigured XCode...
Comment 25 WebKit Review Bot 2009-12-11 04:19:02 PST
style-queue ran check-webkit-style on attachment 44667 [details] without any errors.
Comment 26 WebKit Commit Bot 2009-12-11 07:22:10 PST
Comment on attachment 44667 [details]
Patch

Clearing flags on attachment: 44667

Committed r51985: <http://trac.webkit.org/changeset/51985>
Comment 27 WebKit Commit Bot 2009-12-11 07:22:20 PST
All reviewed patches have been landed.  Closing bug.