Bug 19346 - REGRESSION: Mootools 1.2 Class inheritance broken in post-SquirrelFish merge
Summary: REGRESSION: Mootools 1.2 Class inheritance broken in post-SquirrelFish merge
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Normal
Assignee: Nobody
URL: http://prototype.tangerinesmash.com/D...
Keywords: HasReduction, Regression
Depends on:
Blocks:
 
Reported: 2008-05-31 16:26 PDT by Andrew Gleave
Modified: 2008-06-08 18:00 PDT (History)
2 users (show)

See Also:


Attachments
Reduced test case (159 bytes, text/html)
2008-05-31 18:04 PDT, Mark Rowe (bdash)
no flags Details
this test case shows that caller skips native function callers (257 bytes, text/html)
2008-05-31 18:48 PDT, Maciej Stachowiak
no flags Details
Patch in progress (2.12 KB, patch)
2008-06-08 02:32 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Reduction (3.08 KB, text/html)
2008-06-08 14:23 PDT, Cameron Zwarich (cpst)
no flags Details
Further reduction (215 bytes, text/html)
2008-06-08 15:57 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (4.69 KB, patch)
2008-06-08 17:19 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch with test (6.80 KB, patch)
2008-06-08 17:45 PDT, Cameron Zwarich (cpst)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Gleave 2008-05-31 16:26:51 PDT
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
Comment 1 Mark Rowe (bdash) 2008-05-31 17:08:30 PDT
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.
Comment 2 Mark Rowe (bdash) 2008-05-31 18:04:18 PDT
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.
Comment 3 Maciej Stachowiak 2008-05-31 18:48:32 PDT
Created attachment 21454 [details]
this test case shows that caller skips native function callers
Comment 4 Cameron Zwarich (cpst) 2008-06-07 23:15:49 PDT
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.
Comment 5 Cameron Zwarich (cpst) 2008-06-08 02:32:31 PDT
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.
Comment 6 Cameron Zwarich (cpst) 2008-06-08 03:39:26 PDT
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.
Comment 7 Cameron Zwarich (cpst) 2008-06-08 14:23:08 PDT
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.
Comment 8 Cameron Zwarich (cpst) 2008-06-08 15:11:36 PDT
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.
Comment 9 Cameron Zwarich (cpst) 2008-06-08 15:57:40 PDT
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.
Comment 10 Cameron Zwarich (cpst) 2008-06-08 16:58:30 PDT
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.
Comment 11 Cameron Zwarich (cpst) 2008-06-08 17:19:24 PDT
Created attachment 21584 [details]
Proposed patch

Here's a fix. I won't put it up for review until I write some layout tests.
Comment 12 Cameron Zwarich (cpst) 2008-06-08 17:45:05 PDT
Created attachment 21585 [details]
Proposed patch with test
Comment 13 Darin Adler 2008-06-08 17:48:26 PDT
Comment on attachment 21585 [details]
Proposed patch with test

r=me
Comment 14 Cameron Zwarich (cpst) 2008-06-08 18:00:05 PDT
Landed in r34457. I will make a new bug for the issue with a direct native caller.