Bug 132695 - Base case for get-by-id inline cache doesn't check for HasImpureGetOwnPropertySlot
Summary: Base case for get-by-id inline cache doesn't check for HasImpureGetOwnPropert...
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:
Depends on:
Blocks:
 
Reported: 2014-05-08 11:49 PDT by Mark Hahnenberg
Modified: 2014-10-25 09:53 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.06 KB, patch)
2014-05-08 14:22 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (8.60 KB, patch)
2014-05-08 16:45 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2014-05-08 11:49:42 PDT
We check in the case where we're accessing something other than the base object, but we fail to do so for the base object.
Comment 1 Mark Hahnenberg 2014-05-08 14:22:04 PDT
Created attachment 231100 [details]
Patch
Comment 2 Filip Pizlo 2014-05-08 14:23:07 PDT
Comment on attachment 231100 [details]
Patch

Can has test?
Comment 3 Mark Hahnenberg 2014-05-08 16:45:28 PDT
Created attachment 231112 [details]
Patch
Comment 4 Mark Hahnenberg 2014-05-08 16:47:16 PDT
(In reply to comment #3)
> Created an attachment (id=231112) [details]
> Patch

Re-uploaded because test.
Comment 5 WebKit Commit Bot 2014-05-08 17:23:06 PDT
Comment on attachment 231112 [details]
Patch

Clearing flags on attachment: 231112

Committed r168510: <http://trac.webkit.org/changeset/168510>
Comment 6 WebKit Commit Bot 2014-05-08 17:23:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Tobias Netzel 2014-10-25 09:53:15 PDT
I'm seeing a fail in the test added here when running with CLoop interpreter only.
The check for HasImpureGetOwnPropertySlot that was added here seems to be missing in slow_path_get_by_id() in LLIntSlowPaths.cpp .
I experimentally added an equivalent check there and this test now passes.

In case a fix is needed please give feedback whether you'll fix it yourself or if I should open a new bug and post a patch. As I'm maintaining my own fork of WebKit that's always somewhat outdated, currently at 600.1.25, posting a patch against the trunk would be difficult for me - so I guess you'd want to fix it yourself.

In fact I the exisiting check for
  "!structure->typeInfo().newImpurePropertyFiresWatchpoints()"
seems to be wrong to me and I replaced it with
  "!(structure->typeInfo().hasImpureGetOwnPropertySlot()
     && !structure->typeInfo().newImpurePropertyFiresWatchpoints())",
as this is what is checked for in the JIT compiler. But I have no idea whether this is correct.