Bug 33371 – Infinite recursion in RuntimeObjectImp::getOwnPropertyNames()

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kent Hansen]

This causes a crash if Object.keys is called on a plugin object, for example.

[Kent Hansen]

Created attachment 46126 [details]
Proposed patch

Patch needs to be adapted once the patch for https://bugs.webkit.org/show_bug.cgi?id=32242 is landed.

[WebKit Review Bot]

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

[Geoffrey Garen]

Comment on attachment 46126 [details]
Proposed patch

r=me, but this can't land until the other patch does.

[Kent Hansen]

Created attachment 46439 [details]
Revised patch

Yay, that other monster patch finally landed. :)

[Kent Hansen]

(In reply to comment #4)
> Created an attachment (id=46439) [details]
> Revised patch

I didn't remove the unused mode parameter name. Style bot didn't complain (and check-webkit-style doesn't if I remove the name, either). Is it an unwritten style rule that the name should/should not be present if the argument is unused, or is it a dontcare?

[Kent Hansen]

Created attachment 46445 [details]
Revised patch (remove unused parameter name)

Ouch, with the unused parameter name, Safari didn't build for me (warnings treated as errors).
Whereas on the Qt port, unused parameter warnings are just disabled. That's a bit odd.

[Darin Adler]

Comment on attachment 46445 [details]
Revised patch (remove unused parameter name)

Since this patch includes a new regression test, it also needs to include the expected results for that test. The patch otherwise looks great!

[Kent Hansen]

(In reply to comment #7)
> (From update of attachment 46445 [details])
> Since this patch includes a new regression test, it also needs to include the
> expected results for that test. The patch otherwise looks great!

The test will continue to say SUCCESS if all of it succeeds (including the new part), like before. But it now has a new potential FAILURE output.
So the expected results don't need to be changed, unless you want me to change the test to have separate PASS output for each "part" of the test.

[Kent Hansen]

Comment on attachment 46445 [details]
Revised patch (remove unused parameter name)

Confused about Darin's comment (see my response in comment #8), setting review? again in hope of being advised before I do any more changes to the test.

[Darin Adler]

(In reply to comment #8)
> The test will continue to say SUCCESS if all of it succeeds (including the new
> part), like before. But it now has a new potential FAILURE output.
> So the expected results don't need to be changed, unless you want me to change
> the test to have separate PASS output for each "part" of the test.

OK. My mistake.

[WebKit Commit Bot]

Comment on attachment 46445 [details]
Revised patch (remove unused parameter name)

Clearing flags on attachment: 46445

Committed r53285: <http://trac.webkit.org/changeset/53285>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.