Bug 151839 - [ES6] Add support for Symbol.hasInstance
Summary: [ES6] Add support for Symbol.hasInstance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
: 151120 (view as bug list)
Depends on: 152214 152245 152256 152370
Blocks: 152005
  Show dependency treegraph
 
Reported: 2015-12-03 17:36 PST by Keith Miller
Modified: 2016-06-06 20:53 PDT (History)
8 users (show)

See Also:


Attachments
Patch (88.89 KB, patch)
2015-12-03 18:57 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Benchmark results (63.03 KB, text/plain)
2015-12-03 19:12 PST, Keith Miller
no flags Details
Patch (116.42 KB, patch)
2015-12-07 15:55 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (111.83 KB, patch)
2015-12-07 16:26 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (115.31 KB, patch)
2015-12-07 16:48 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (109.99 KB, patch)
2015-12-08 12:32 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (110.04 KB, patch)
2015-12-09 17:33 PST, Keith Miller
saam: review+
Details | Formatted Diff | Diff
Benchmark results (62.75 KB, text/plain)
2015-12-09 19:30 PST, Keith Miller
no flags Details
Patch (115.75 KB, patch)
2015-12-09 20:21 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (115.25 KB, patch)
2015-12-10 17:20 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (120.33 KB, patch)
2015-12-17 13:26 PST, Keith Miller
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2015-12-03 17:36:26 PST
[ES6] Add support for Symbol.hasInstance
Comment 1 Keith Miller 2015-12-03 18:57:51 PST
Created attachment 266587 [details]
Patch
Comment 2 Keith Miller 2015-12-03 19:12:21 PST
Created attachment 266588 [details]
Benchmark results

64-bit benchmark results.
Comment 3 Keith Miller 2015-12-07 15:55:15 PST
Created attachment 266823 [details]
Patch
Comment 4 Keith Miller 2015-12-07 16:26:27 PST
Created attachment 266830 [details]
Patch
Comment 5 WebKit Commit Bot 2015-12-07 16:27:40 PST
Attachment 266830 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Keith Miller 2015-12-07 16:48:28 PST
Created attachment 266833 [details]
Patch
Comment 7 WebKit Commit Bot 2015-12-07 16:49:39 PST
Attachment 266833 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Keith Miller 2015-12-08 10:53:37 PST
Comment on attachment 266833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266833&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        [WIP] Still need to do 32-bit, add tests, and cleanup but should be roughly done.

Need to delete this.
Comment 9 Saam Barati 2015-12-08 11:01:23 PST
Comment on attachment 266833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266833&action=review

> Source/JavaScriptCore/ChangeLog:17
> +        needs non-default behavior for resolving the expression. i.e. The constructor has a Symbol.hasInstance that differs from the one on

From reading this sentence I would think that jsTrue=>non-default behavior and that jsFalse=>default behavior but it's the opposite.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2191
> +    instructions().append(hasInstanceSymbol->index());

Nit: I think the name hasInstanceSymbol is a bit misleading here (and in other places) because it's the result of doing a GetById with the hasInstanceSymbol and not the symbol itself.
Maybe something like "symbolHasInstanceResult" or "hasInstanceSymbolResult" or just "hasInstanceFunction"?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:603
> +        RegisterID* emitIsFunction(RegisterID* dst, RegisterID* src);

This isn't called

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4222
> +        if (!hasInstanceSymbolNode->isCellConstant()) {
> +
> +            notDefault = m_jit.branchPtr(MacroAssembler::NotEqual, hasInstanceSymbol.gpr(), TrustedImmPtr(defaultHasInstanceFunction));
> +        }

Style: This can be an "if" with no curly braces.

> Source/JavaScriptCore/jit/JSInterfaceJIT.h:165
> +    ALWAYS_INLINE JSInterfaceJIT::JumpList JSInterfaceJIT::emitJumpIfNull(JSValueRegs reg, GPRReg scratch1, GPRReg scratch2) {

This really jumps if it's undefined or null, right? If so, we should name it emitJumpIfNullOrUndefined

> Source/JavaScriptCore/llint/LLIntData.cpp:158
> +    ASSERT(!(reinterpret_cast<ptrdiff_t>((reinterpret_cast<WriteBarrier<JSCell>*>(0x4000)->slot())) - 0x4000));

Why is this here?
Comment 10 Keith Miller 2015-12-08 11:16:45 PST
Comment on attachment 266833 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266833&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2191
>> +    instructions().append(hasInstanceSymbol->index());
> 
> Nit: I think the name hasInstanceSymbol is a bit misleading here (and in other places) because it's the result of doing a GetById with the hasInstanceSymbol and not the symbol itself.
> Maybe something like "symbolHasInstanceResult" or "hasInstanceSymbolResult" or just "hasInstanceFunction"?

How do you feel about hasInstanceValue (since it may or may not be a function)?

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:603
>> +        RegisterID* emitIsFunction(RegisterID* dst, RegisterID* src);
> 
> This isn't called

Deleted.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4222
>> +        }
> 
> Style: This can be an "if" with no curly braces.

Fixed.

>> Source/JavaScriptCore/llint/LLIntData.cpp:158
>> +    ASSERT(!(reinterpret_cast<ptrdiff_t>((reinterpret_cast<WriteBarrier<JSCell>*>(0x4000)->slot())) - 0x4000));
> 
> Why is this here?

This verifies that the offset of the jsvalue in a WriteBarrier is at the zero offset.
Comment 11 Keith Miller 2015-12-08 11:23:46 PST
> >> +    ASSERT(!(reinterpret_cast<ptrdiff_t>((reinterpret_cast<WriteBarrier<JSCell>*>(0x4000)->slot())) - 0x4000));
> > 
> > Why is this here?
> 
> This verifies that the offset of the jsvalue in a WriteBarrier is at the
> zero offset.

I should mention that we use this fact in the LLInt as part of the ptr test on Function.prototype[Symbol.hasInstance].
Comment 12 Geoffrey Garen 2015-12-08 11:26:05 PST
Need updated es6.yaml results.

Please rename "check has instance" to "overrides has instance".
Comment 13 Keith Miller 2015-12-08 12:32:25 PST
Created attachment 266927 [details]
Patch
Comment 14 Keith Miller 2015-12-09 17:33:14 PST
Created attachment 267058 [details]
Patch
Comment 15 WebKit Commit Bot 2015-12-09 17:34:57 PST
Attachment 267058 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Saam Barati 2015-12-09 18:56:35 PST
Comment on attachment 267058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267058&action=review

r=me with comments

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2980
> +    // Note that we intentionally clobber the hasInstanceValue register here on x86 because we
> +    // are short on registers. This is valid because we know InstanceOfCustom is the last use
> +    // of the GetById which produces it.

This seems a bit sketchy to me and also probably unnecessary.
We probably don't care much about X86.
Also, if you're worried about the extra register here you could call an operation that returns a value that fits in a single register on 32-bit platforms.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2997
> +    MacroAssembler::Jump slowCase = m_jit.jump();

I don't think you need a variable for this (or above).

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5780
> +        // Unlike, the DFG we don't worry about cleaning this code up for the case where we have proven the hasInstanceValue is a constant as LLVM should fix it for us.

Style: "Unlike, the DFG" => "Unlike in the DFG,"

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5785
> +        m_out.branch(m_out.notEqual(hasInstance, m_out.constIntPtr(defaultHasInstanceFunction)), unsure(continuation), unsure(defaultHasInstance));

I think you should have unlikely(continuation) and likely(defaultHasInstance)

> Source/JavaScriptCore/jit/JITOpcodes.cpp:854
> +    int proto = currentInstruction[3].u.operand;

style: You call this "proto" here and "constructor" below. I say just go w/ constructor.

> Source/JavaScriptCore/runtime/JSTypeInfo.h:41
> +static const unsigned OverridesHasInstanceFlag = 1 << 2; // FixMe: This is only trivially used by the runtime and should be removed: https://bugs.webkit.org/show_bug.cgi?id=152005

style: FixMe => FIXME

> LayoutTests/js/Object-getOwnPropertyNames-expected.txt:61
>  PASS getSortedOwnPropertyNames(Error.prototype) is ['constructor', 'message', 'name', 'toString']

What happened to all the other tests?
Comment 17 Saam Barati 2015-12-09 18:57:19 PST
Also, have you run perf tests?
Are they neutral? Can you post them?
Comment 18 Keith Miller 2015-12-09 19:28:06 PST
(In reply to comment #17)
> Also, have you run perf tests?
> Are they neutral? Can you post them?

They are attached above and are neutral (The !s are on tests that don't use instanceof).
Comment 19 Keith Miller 2015-12-09 19:30:55 PST
Created attachment 267064 [details]
Benchmark results

More recent benchmark results.
Comment 20 Keith Miller 2015-12-09 19:55:38 PST
Comment on attachment 267058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267058&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2980
>> +    // of the GetById which produces it.
> 
> This seems a bit sketchy to me and also probably unnecessary.
> We probably don't care much about X86.
> Also, if you're worried about the extra register here you could call an operation that returns a value that fits in a single register on 32-bit platforms.

I suppose I could add a function that returns an int32_t or something that contains the boolean result of the slow path but I'm not sure that feels any less sketchy.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2997
>> +    MacroAssembler::Jump slowCase = m_jit.jump();
> 
> I don't think you need a variable for this (or above).

While you are correct in this case, I dealt with bugs related assigning a gpr after a jump due to calls occurring in the wrong order. I'd feel a bit safer when the order is extra explicit in this context.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5780
>> +        // Unlike, the DFG we don't worry about cleaning this code up for the case where we have proven the hasInstanceValue is a constant as LLVM should fix it for us.
> 
> Style: "Unlike, the DFG" => "Unlike in the DFG,"

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5785
>> +        m_out.branch(m_out.notEqual(hasInstance, m_out.constIntPtr(defaultHasInstanceFunction)), unsure(continuation), unsure(defaultHasInstance));
> 
> I think you should have unlikely(continuation) and likely(defaultHasInstance)

I had that at one point but this node is almost never generated unless JS code uses a custom hasInstance in some form. So it makes sense to not punish users when they have a custom hasInstance.

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:854
>> +    int proto = currentInstruction[3].u.operand;
> 
> style: You call this "proto" here and "constructor" below. I say just go w/ constructor.

This is just the diff being laid out poorly... All of this code is just deleted emitSlow_op_check_has_instance being mixed with existing emitSlow_op_instanceof that I did not change... "proto" is actually the prototype of the constructor not the constructor itself.

>> Source/JavaScriptCore/runtime/JSTypeInfo.h:41
>> +static const unsigned OverridesHasInstanceFlag = 1 << 2; // FixMe: This is only trivially used by the runtime and should be removed: https://bugs.webkit.org/show_bug.cgi?id=152005
> 
> style: FixMe => FIXME

Fixed.

>> LayoutTests/js/Object-getOwnPropertyNames-expected.txt:61
>>  PASS getSortedOwnPropertyNames(Error.prototype) is ['constructor', 'message', 'name', 'toString']
> 
> What happened to all the other tests?

Ughh... I forgot to add them... I'll re-upload another version with them again.
Comment 21 Keith Miller 2015-12-09 20:21:35 PST
Created attachment 267069 [details]
Patch
Comment 22 Saam Barati 2015-12-09 21:15:57 PST
Comment on attachment 267058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267058&action=review

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2980
>>> +    // of the GetById which produces it.
>> 
>> This seems a bit sketchy to me and also probably unnecessary.
>> We probably don't care much about X86.
>> Also, if you're worried about the extra register here you could call an operation that returns a value that fits in a single register on 32-bit platforms.
> 
> I suppose I could add a function that returns an int32_t or something that contains the boolean result of the slow path but I'm not sure that feels any less sketchy.

Maybe just have one path for everything then?
Up to you.

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5785
>>> +        m_out.branch(m_out.notEqual(hasInstance, m_out.constIntPtr(defaultHasInstanceFunction)), unsure(continuation), unsure(defaultHasInstance));
>> 
>> I think you should have unlikely(continuation) and likely(defaultHasInstance)
> 
> I had that at one point but this node is almost never generated unless JS code uses a custom hasInstance in some form. So it makes sense to not punish users when they have a custom hasInstance.

makes sense

>>> Source/JavaScriptCore/jit/JITOpcodes.cpp:854
>>> +    int proto = currentInstruction[3].u.operand;
>> 
>> style: You call this "proto" here and "constructor" below. I say just go w/ constructor.
> 
> This is just the diff being laid out poorly... All of this code is just deleted emitSlow_op_check_has_instance being mixed with existing emitSlow_op_instanceof that I did not change... "proto" is actually the prototype of the constructor not the constructor itself.

oops I missed that. ok
Comment 23 WebKit Commit Bot 2015-12-10 00:32:13 PST
Attachment 267069 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Keith Miller 2015-12-10 17:20:19 PST
Created attachment 267142 [details]
Patch
Comment 25 WebKit Commit Bot 2015-12-10 17:21:45 PST
Attachment 267142 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:488:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Keith Miller 2015-12-10 17:24:26 PST
After some investigation, I think you are right that we should use a non-encodedjsvalue to hold the result of the custom slow path. It looks like we use int32_t for that so I changed JIT_OPERATION to return an int32_t. If you want to give it another quick look over, I would appreciate it.
Comment 27 Saam Barati 2015-12-11 12:50:05 PST
Comment on attachment 267142 [details]
Patch

LGTM
Comment 28 WebKit Commit Bot 2015-12-11 13:44:08 PST
Comment on attachment 267142 [details]
Patch

Clearing flags on attachment: 267142

Committed r193974: <http://trac.webkit.org/changeset/193974>
Comment 29 WebKit Commit Bot 2015-12-11 13:44:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Csaba Osztrogonác 2015-12-12 02:37:50 PST
(In reply to comment #28)
> Comment on attachment 267142 [details]
> Patch
> 
> Clearing flags on attachment: 267142
> 
> Committed r193974: <http://trac.webkit.org/changeset/193974>

It made many JSC tests crash on Linux bots:
- GTK X86_64 - https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/12637
- GTK X86 - https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/57694
- EFL X86_64 - https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/25878
Comment 31 Csaba Osztrogonác 2015-12-14 02:25:30 PST
(In reply to comment #30)
> (In reply to comment #28)
> > Comment on attachment 267142 [details]
> > Patch
> > 
> > Clearing flags on attachment: 267142
> > 
> > Committed r193974: <http://trac.webkit.org/changeset/193974>
> 
> It made many JSC tests crash on Linux bots:
> - GTK X86_64 -
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/12637
> - GTK X86 -
> https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/57694
> - EFL X86_64 -
> https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/
> builds/25878

- EFL Linux AArch64: https://build.webkit.org/builders/EFL%20Linux%20AArch64%20Release/builds/4700
- GTK ARMv7(?): https://build.webkit.org/builders/GTK%20Linux%20ARM%20Release/builds/9652

(EFL ARMv7 bots work fine.)
Comment 32 Csaba Osztrogonác 2015-12-14 02:29:42 PST
I filed a new bug report to fix this regression: bug152245
Comment 33 Carlos Alberto Lopez Perez 2015-12-14 03:05:15 PST
The debug bots are giving this assertion since r193974:


ASSERTION FAILED: i < m_numBits
../../Source/WTF/wtf/FastBitVector.h(151) : void WTF::FastBitVector::set(size_t)
WTFCrashWithSecurityImplication(...)


Example:

[...]
stress/instanceof-not-cell.js.no-llint: ASSERTION FAILED: i < m_numBits
stress/instanceof-not-cell.js.no-llint: ../../Source/WTF/wtf/FastBitVector.h(151) : void WTF::FastBitVector::set(size_t)
stress/instanceof-not-cell.js.no-llint: 1   0x7f58e30974ae /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrashWithSecurityImplication+0x1e) [0x7f58e30974ae]
stress/instanceof-not-cell.js.no-llint: 2   0x7f58e2719b66 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF13FastBitVector3setEm+0x42) [0x7f58e2719b66]
stress/instanceof-not-cell.js.no-llint: 3   0x7f58e2716964 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e7964) [0x7f58e2716964]
stress/instanceof-not-cell.js.no-llint: 4   0x7f58e2717a96 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e8a96) [0x7f58e2717a96]
stress/instanceof-not-cell.js.no-llint: 5   0x7f58e2718543 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e9543) [0x7f58e2718543]
stress/instanceof-not-cell.js.no-llint: 6   0x7f58e2717af5 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e8af5) [0x7f58e2717af5]
stress/instanceof-not-cell.js.no-llint: 7   0x7f58e27169dc /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e79dc) [0x7f58e27169dc]
stress/instanceof-not-cell.js.no-llint: 8   0x7f58e2716ae6 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e7ae6) [0x7f58e2716ae6]
stress/instanceof-not-cell.js.no-llint: 9   0x7f58e2716bad /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x10e7bad) [0x7f58e2716bad]
stress/instanceof-not-cell.js.no-llint: 10  0x7f58e2716de5 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC24BytecodeLivenessAnalysis19runLivenessFixpointEv+0x231) [0x7f58e2716de5]
stress/instanceof-not-cell.js.no-llint: 11  0x7f58e27179b9 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC24BytecodeLivenessAnalysis7computeEv+0x6b) [0x7f58e27179b9]
stress/instanceof-not-cell.js.no-llint: 12  0x7f58e271665a /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC24BytecodeLivenessAnalysisC1EPNS_9CodeBlockE+0x68) [0x7f58e271665a]
stress/instanceof-not-cell.js.no-llint: 13  0x7f58e2951e6a /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZSt11make_uniqueIN3JSC24BytecodeLivenessAnalysisEIPNS0_9CodeBlockEEENSt10_Unique_ifIT_E14_Single_objectEDpOT0_+0x3a) [0x7f58e2951e6a]
stress/instanceof-not-cell.js.no-llint: 14  0x7f58e295024d /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC9CodeBlock16livenessAnalysisEv+0x91) [0x7f58e295024d]
stress/instanceof-not-cell.js.no-llint: 15  0x7f58e294d53d /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG5Graph11livenessForEPNS_9CodeBlockE+0xd9) [0x7f58e294d53d]
stress/instanceof-not-cell.js.no-llint: 16  0x7f58e294d643 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG5Graph11livenessForEPNS_15InlineCallFrameE+0x35) [0x7f58e294d643]
stress/instanceof-not-cell.js.no-llint: 17  0x7f58e294d9a1 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG5Graph16isLiveInBytecodeENS_15VirtualRegisterENS_10CodeOriginE+0x179) [0x7f58e294d9a1]
stress/instanceof-not-cell.js.no-llint: 18  0x7f58e2a2af02 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fbf02) [0x7f58e2a2af02]
stress/instanceof-not-cell.js.no-llint: 19  0x7f58e2a2acf6 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fbcf6) [0x7f58e2a2acf6]
stress/instanceof-not-cell.js.no-llint: 20  0x7f58e2a2a8be /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fb8be) [0x7f58e2a2a8be]
stress/instanceof-not-cell.js.no-llint: 21  0x7f58e2a2b753 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fc753) [0x7f58e2a2b753]
stress/instanceof-not-cell.js.no-llint: 22  0x7f58e2a2b173 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x13fc173) [0x7f58e2a2b173]
stress/instanceof-not-cell.js.no-llint: 23  0x7f58e2a2ad60 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG23performPhantomInsertionERNS0_5GraphE+0x2b) [0x7f58e2a2ad60]
stress/instanceof-not-cell.js.no-llint: 24  0x7f58e2a2f940 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG4Plan19compileInThreadImplERNS0_14LongLivedStateE+0x5c6) [0x7f58e2a2f940]
stress/instanceof-not-cell.js.no-llint: 25  0x7f58e2a2efdf /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG4Plan15compileInThreadERNS0_14LongLivedStateEPNS0_10ThreadDataE+0x165) [0x7f58e2a2efdf]
stress/instanceof-not-cell.js.no-llint: 26  0x7f58e2b1a9af /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG8Worklist9runThreadEPNS0_10ThreadDataE+0x2d7) [0x7f58e2b1a9af]
stress/instanceof-not-cell.js.no-llint: 27  0x7f58e2b1ad5c /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3JSC3DFG8Worklist14threadFunctionEPv+0x2a) [0x7f58e2b1ad5c]
stress/instanceof-not-cell.js.no-llint: 28  0x7f58e30b2598 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1a83598) [0x7f58e30b2598]
stress/instanceof-not-cell.js.no-llint: 29  0x7f58e30b2748 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1a83748) [0x7f58e30b2748]
stress/instanceof-not-cell.js.no-llint: 30  0x7f58e2b3d046 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZNKSt8functionIFvvEEclEv+0x32) [0x7f58e2b3d046]
stress/instanceof-not-cell.js.no-llint: 31  0x7f58e30b247a /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1a8347a) [0x7f58e30b247a]
stress/instanceof-not-cell.js.no-llint: Segmentation fault
stress/instanceof-not-cell.js.no-llint: ERROR: Unexpected exit code: 139
[...]

More at: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/6385/steps/jscore-test/logs/stdio
Comment 34 Chris Dumez 2015-12-14 09:40:52 PST
Reopening as it was rolled out in <http://trac.webkit.org/changeset/194036> for causing crashes.
Comment 35 Keith Miller 2015-12-17 13:26:01 PST
Created attachment 267579 [details]
Patch
Comment 36 WebKit Commit Bot 2015-12-17 13:27:38 PST
Attachment 267579 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:490:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Saam Barati 2015-12-17 13:47:24 PST
Comment on attachment 267579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267579&action=review

r=me again

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:76
> +        ASSERT(opcodeLengths[opcodeID] >= 1);

All these asserts should be > and not >=.
Comment 38 Keith Miller 2015-12-17 13:50:28 PST
Comment on attachment 267579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267579&action=review

>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:76
>> +        ASSERT(opcodeLengths[opcodeID] >= 1);
> 
> All these asserts should be > and not >=.

Fixed.
Comment 39 Keith Miller 2015-12-17 16:38:02 PST
Committed r194248: <http://trac.webkit.org/changeset/194248>
Comment 40 Joseph Pecoraro 2016-06-06 20:53:33 PDT
*** Bug 151120 has been marked as a duplicate of this bug. ***