Bug 119343 - Some cleanup in JSValue::get
Summary: Some cleanup in JSValue::get
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-31 10:44 PDT by Gavin Barraclough
Modified: 2013-08-01 14:05 PDT (History)
5 users (show)

See Also:


Attachments
fix (93.13 KB, patch)
2013-07-31 10:48 PDT, Gavin Barraclough
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2013-07-31 10:44:20 PDT
JSValue::get is implemented to:
    1) Check if the value is a cell – if not, synthesize a prototype to search,
    2) call getOwnPropertySlot on the cell,
    3) if this returns false, cast to JSObject to get the prototype, and walk the prototype chain.
By all rights this should crash when passed a string and accessing a property that does not exist, because the string is a cell, getOwnPropertySlot should return false, and the cast to JSObject should be unsafe.  To work around this, JSString::getOwnPropertySlot actually implements 'get' functionality - searching the prototype chain, and faking out a return value of undefined if no property is found.

This is a huge hazard, since fixing JSString::getOwnPropertySlot or calling getOwnPropertySlot on cells from elsewhere would introduce bugs.  Fortunately it is only ever called in this one place.

The fix here is to move getOwnPropertySlot onto JSObjecte and end this madness - cells don't have property slots anyway.
Comment 1 Gavin Barraclough 2013-07-31 10:48:44 PDT
Created attachment 207862 [details]
fix
Comment 2 Geoffrey Garen 2013-07-31 10:53:23 PDT
Comment on attachment 207862 [details]
fix

View in context: https://bugs.webkit.org/attachment.cgi?id=207862&action=review

r=me

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:675
> -    
> +

Please revert.
Comment 3 Build Bot 2013-07-31 11:27:49 PDT
Comment on attachment 207862 [details]
fix

Attachment 207862 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1297657
Comment 4 Gavin Barraclough 2013-07-31 13:43:43 PDT
Fixed in r153532, r153537.
Comment 5 Simon Fraser (smfr) 2013-07-31 18:44:39 PDT
This broke bindings generation tests; please fix:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29?numbuilds=200
Comment 6 Mark Lam 2013-08-01 14:05:56 PDT
(In reply to comment #5)
> This broke bindings generation tests; please fix:
> http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29?numbuilds=200

The fix for the bindings test is at https://bugs.webkit.org/show_bug.cgi?id=119410.