Bug 29005 – The values of RuntimeArray are not enumerable

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Benjamin Poulain]

RuntimeArray does not behave like a regular JSArray regarding the listing of its properties. The indices of the array are not enumerated.

[Benjamin Poulain]

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.

[Benjamin Poulain]

Created attachment 39144 [details]
patch suggestion

Change the for() loop to follow the coding convention.

[Geoffrey Garen]

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.

[Benjamin Poulain]

Created attachment 39641 [details]
patch suggestion

The same patch with a test for Objective-C.

[Eric Seidel (no email)]

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?

[Kenneth Rohde Christiansen]

+    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?

[Benjamin Poulain]

Created attachment 39685 [details]
proposed patch

Same patch, same test.
Fixed the code to follow the coding convention (thanks Eric and Kenneth).

[Benjamin Poulain]

(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.

[Eric Seidel (no email)]

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?

[Simon Hausmann]

Comment on attachment 39685 [details]
proposed patch

Schedule for cq, as correctly suggested by Eric :)

[WebKit Commit Bot]

Comment on attachment 39685 [details]
proposed patch

Clearing flags on attachment: 39685

Committed r48712: <http://trac.webkit.org/changeset/48712>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

Comment on attachment 39685 [details]
proposed patch

It is incorrect to override getPropertyNames in almost any case.  You should be overriding getOwnPropertyNames.

[Oliver Hunt]

I rolled this patch out in r48712 due to it being incorrect

[Eric Seidel (no email)]

Thank you oliver.

[Benjamin Poulain]

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?

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44394 [details] without any errors.

[Darin Adler]

(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.

[Darin Adler]

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

[Eric Seidel (no email)]

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.

[Benjamin Poulain]

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.

[WebKit Review Bot]

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

[Benjamin Poulain]

Thanks Darin for the explanation. I had totally misunderstood the purpose of getPropertyNames().

I will open a bug report for the problem of QtInstance.

[Benjamin Poulain]

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...

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44667 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 44667 [details]
Patch

Clearing flags on attachment: 44667

Committed r51985: <http://trac.webkit.org/changeset/51985>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.