Bug 19346 – REGRESSION: Mootools 1.2 Class inheritance broken in post-SquirrelFish merge

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andrew Gleave]

There seems to be a regression relating to the way class inheritance is implemented in the Mootools JS framework which has been broken recent WebKit nightlies.  This has been reported to Mootools in case they're relying on a JavaScriptCore quirk but they don't seem interested, so I'm reporting here as well since mrowe* indicated that regressions should be filed here as well.

I'm not sure quite what the problem is, although, it seems related to calling a base class method from a subclass.  This has been tested with r34254.

See URL for test case.

* See http://64.233.183.104/search?q=cache:u4k4sf4FNswJ:dev.mootools.net/ticket/711+Webkit+mootools+class&hl=en&client=safari&strip=1

[Mark Rowe (bdash)]

If the problem is caused by a change in behaviour of WebKit, then it's definitely preferable to file a bug report in WebKit's bug tracker as it's often an unintentional change.

I think we need a reduced test case to make headway on this issue.

[Mark Rowe (bdash)]

Created attachment 21453 [details]
Reduced test case

The problem is that arguments.callee.caller is returning null when the function is called via Function.apply.

[Maciej Stachowiak]

Created attachment 21454 [details]
this test case shows that caller skips native function callers

[Cameron Zwarich (cpst)]

The problem is that Machine::retrieveCaller() calls getCallerFunctionOffset(), which stops when it sees a null codeBlock. In that case, we want to check the prior call frame instead.

[Cameron Zwarich (cpst)]

Created attachment 21571 [details]
Patch in progress

Here's a patch in progress. It fixes the bug, but the code is somewhat ugly. I made it by inlining the calls to getCallFrame() and getCallerFunctionOffset() and modifying the logic to handle this special case. I am not quite sure of the necessity of the second if (!exec) at this hour.

When the caller functionality was added to SquirrelFish, a test case was added to fast/js/function-dot-arguments-and-caller.html that codifies the behaviour in the bug. This is the only JavaScriptCore or layout test that this patch fails, and JavaScriptCore also fails this test prior to SquirrelFish.

[Cameron Zwarich (cpst)]

Interestingly enough, the patch I posted works on the two test cases but not the URL linked in the bug. I'll try to figure out the difference.

[Cameron Zwarich (cpst)]

Created attachment 21581 [details]
Reduction

So, it turns out that the earlier reductions for a different bug. Here is a new reduction of the bug, working from the MooTools source. It can probably be reduced further, but I wanted to dump it on Bugzilla in case I have to do something else later.

There is no immediate native caller that is the problem, and all of the callers of the functions leading up to the one that is the problem are the same in Safari 3.1 and ToT.

[Cameron Zwarich (cpst)]

Sorry, when I said that ToT is the same as Safari 3.1 on all of those cases, I meant ToT with my patches. The call chain is as follows:

initialize() in Class:

Safari 3.1 and ToT agree that the caller is null.

initialize() in ExampleClass.Inherited:

Safari 3.1 says that the caller is initialize() in class, whereas ToT says that the caller is null. My patch fixes this, because this function is called by using apply in the body of initialize() in Class.

self.parent() in Extends:

Safari 3.1 says that the caller is initialize() in ExampleClass.Inherited, whereas both ToT and ToT with my patch say that the caller is null.

Thus, the problem is not directly due to a native call. I'll try to reduce it further and figure out what's going on.

[Cameron Zwarich (cpst)]

Created attachment 21582 [details]
Further reduction

Here is a further reduction of the bug. It has two cases to demonstrate that the native call messes up the later attempt to get a JS caller.

[Cameron Zwarich (cpst)]

The problem is this code in 

    Register* callerCallFrame = (*registerBase) + callerOffset;
    if (!callerCallFrame[Machine::CallerCodeBlock].u.codeBlock) // test for eval frame
        return false;

It is testing whether the caller is an eval frame by testing whether the caller's caller has a codeBlock, but this also happens when the caller's caller is native code. Removing the code fixes the bug, but causes an assertion failure when the caller is eval code. The easiest fix is probably to add a CodeType field to CodeBlock.

[Cameron Zwarich (cpst)]

Created attachment 21584 [details]
Proposed patch

Here's a fix. I won't put it up for review until I write some layout tests.

[Cameron Zwarich (cpst)]

Created attachment 21585 [details]
Proposed patch with test

[Darin Adler]

Comment on attachment 21585 [details]
Proposed patch with test

r=me

[Cameron Zwarich (cpst)]

Landed in r34457. I will make a new bug for the issue with a direct native caller.