Bug 150663 - Crash making a tail call from a getter to a host function
Summary: Crash making a tail call from a getter to a host function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-10-28 22:08 PDT by Michael Saboff
Modified: 2015-10-30 15:15 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch for all platforms except Windows X86 (5.80 KB, patch)
2015-10-28 22:15 PDT, Michael Saboff
ggaren: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (672.91 KB, application/zip)
2015-10-28 22:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (745.38 KB, application/zip)
2015-10-28 23:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (769.96 KB, application/zip)
2015-10-28 23:04 PDT, Build Bot
no flags Details
Patch with updated expectations for new test. (5.80 KB, patch)
2015-10-29 07:08 PDT, Michael Saboff
ggaren: review-
Details | Formatted Diff | Diff
Patch (6.16 KB, patch)
2015-10-29 15:25 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2015-10-28 22:08:54 PDT
This test crashes with a debug build:
——
"use strict";

class Test {
    constructor(a, b) {
        this.a = a;
        this.b = b;
    }

    get sum() {
        return Number(this.a + this.b);
    }
}

var testObj = new Test(40, 2);

var result = testObj.sum;

print(result);
——

Crash trace

ASSERTION FAILED: cell->isObject()
/Volumes/Data/src/webkit/Source/JavaScriptCore/runtime/JSObject.h(1064) : JSC::JSObject *JSC::asObject(JSC::JSCell *)
1   0x108f84880 WTFCrash
2   0x1083db051 JSC::asObject(JSC::JSCell*)
3   0x1083dafd0 JSC::asObject(JSC::JSValue)
4   0x1083dafa2 JSC::Register::object() const
5   0x1083daf0c JSC::ExecState::callee() const
6   0x1083d65f9 JSC::ExecState::vm() const
7   0x108b1d73e getHostCallReturnValueWithExecState
8   0x108ccf26e vmEntryToJavaScript
9   0x108b08275 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
10  0x108ada4f2 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
11  0x10847f07e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
12  0x108a0360d JSC::callGetter(JSC::ExecState*, JSC::JSValue, JSC::JSValue)
13  0x108e55d3d JSC::PropertySlot::functionGetter(JSC::ExecState*) const
14  0x1083e9aad JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const
15  0x1083e98eb JSC::JSValue::get(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const
16  0x108cc59c1 llint_slow_path_get_by_id
17  0x108cd2330 llint_entry
18  0x108ccf26e vmEntryToJavaScript
19  0x108b08275 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
20  0x108ad9e92 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
21  0x10851e011 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
22  0x108340f8f runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow, 16ul> const&, bool, bool)
23  0x10833ff53 jscmain(int, char**)
24  0x10833fa3b main
25  0x7fff940545ad start
Segmentation fault: 11


The problem is that the getHostCallReturnValue() function passes its frame pointer as an argument to getHostCallReturnValueWithExecState(), which wants to use the frame to get the current VM.  The passed frame is the caller of a host function, which is a C++ function in the test above.  A fix is to change getHostCallReturnValue() to pass what will be the callee's frame address to getHostCallReturnValueWithExecState().

<rdar://problem/23287664>
Comment 1 Michael Saboff 2015-10-28 22:15:13 PDT
Created attachment 264300 [details]
Proposed patch for all platforms except Windows X86

Need to work hands on to get Windows X86 working.
Comment 2 Build Bot 2015-10-28 22:58:46 PDT
Comment on attachment 264300 [details]
Proposed patch for all platforms except Windows X86

Attachment 264300 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/351761

New failing tests:
js/regress-150663.html
Comment 3 Build Bot 2015-10-28 22:58:49 PDT
Created attachment 264301 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 4 Build Bot 2015-10-28 23:02:40 PDT
Comment on attachment 264300 [details]
Proposed patch for all platforms except Windows X86

Attachment 264300 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/351770

New failing tests:
js/regress-150663.html
Comment 5 Build Bot 2015-10-28 23:02:43 PDT
Created attachment 264302 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 6 Build Bot 2015-10-28 23:04:49 PDT
Comment on attachment 264300 [details]
Proposed patch for all platforms except Windows X86

Attachment 264300 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/351760

New failing tests:
js/regress-150663.html
Comment 7 Build Bot 2015-10-28 23:04:51 PDT
Created attachment 264303 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Geoffrey Garen 2015-10-28 23:36:45 PDT
Comment on attachment 264300 [details]
Proposed patch for all platforms except Windows X86

EWS is angry.
Comment 9 Michael Saboff 2015-10-29 07:08:43 PDT
Created attachment 264314 [details]
Patch with updated expectations for new test.

Forgot to update expected results after changing description() in prior patch.

Still need to implement Windows X86.
Comment 10 Geoffrey Garen 2015-10-29 11:24:30 PDT
> Still need to implement Windows X86.

Will this patch start crashing the windows testers if it lands in its current state?
Comment 11 Michael Saboff 2015-10-29 11:30:34 PDT
(In reply to comment #10)
> > Still need to implement Windows X86.
> 
> Will this patch start crashing the windows testers if it lands in its
> current state?

The new test should crash on Windows in the current state.

I'm testing the Windows-86 specific part of the change now.  I hope to post a new patch soon.
Comment 12 Geoffrey Garen 2015-10-29 11:32:56 PDT
Comment on attachment 264314 [details]
Patch with updated expectations for new test.

OK, let's not land this yet, since it would turn the windows testers red.
Comment 13 Michael Saboff 2015-10-29 15:25:21 PDT
Created attachment 264355 [details]
Patch

Tested on Mac 32 & 64, iOS 32 & 64 and Win 32.
Comment 14 Geoffrey Garen 2015-10-29 16:04:05 PDT
Comment on attachment 264355 [details]
Patch

r=me
Comment 15 Michael Saboff 2015-10-29 17:04:08 PDT
Committed r191765: <http://trac.webkit.org/changeset/191765>
Comment 16 Michael Saboff 2015-10-30 15:15:34 PDT
The Windows X86-64 change was inadvertently missed when fixing this bug.  That change is tracked in https://bugs.webkit.org/show_bug.cgi?id=150737