Bug 132695 – Base case for get-by-id inline cache doesn't check for HasImpureGetOwnPropertySlot

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 231100 [details]
Patch

[Filip Pizlo]

Comment on attachment 231100 [details]
Patch

Can has test?

[Mark Hahnenberg]

Created attachment 231112 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #3)
> Created an attachment (id=231112) [details]
> Patch

Re-uploaded because test.

[WebKit Commit Bot]

Comment on attachment 231112 [details]
Patch

Clearing flags on attachment: 231112

Committed r168510: <http://trac.webkit.org/changeset/168510>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tobias Netzel]

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.