Bug 166658 – Reduce the use of EncodedJSValue, and use JSValue instead.

• Status: ASSIGNED
• Component: JavaScriptCore

[Mark Lam]

Existing code uses EncodedJSValue for passing JSValues in and out of host functions.  However, it does not seem that this is necessary in most cases.  Specifically, NativeFunction, GetValueFunc, and PutValueFunc can all use JSValue with no performance implications.  Using JSValue directly (instead of EncodedJSValue) also improves type safety, and reduces the need to marshal values back and forth using JSValue::encode() and JSValue::decode().  This makes the code more easy to read and write, with no performance loss.

The 2 primary reason for code to still use EncodedJSValue are:
1. functions that require C linkage (e.g. JIT_OPERATION functions).
2. as an element storage type where using EncodedJSValue allows us to use the trivial constructor for int64_t.

Note: this patch is not exhaustive in removing all unnecessary uses of EncodedJSValue.  It merely tries to reduce it as much as possible with minimal effort.

[Mark Lam]

Created attachment 297946 [details]
x86_64 JSC benchmark results.

[Mark Lam]

Created attachment 297947 [details]
proposed patch.

[Mark Lam]

Created attachment 297951 [details]
proposed patch: rebased.

[WebKit Commit Bot]

Attachment 297951 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:129:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 192 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Created attachment 297966 [details]
proposed patch: with gtk/efl build fixes.

[Csaba Osztrogonác]

(In reply to comment #5)
> Created attachment 297966 [details]
> proposed patch: with gtk/efl build fixes.

I checked this patch on ARM Linux. (JSCOnly port) It works fine on AArch64,
but almost all tests crash on ARMv7 with ARM and Thumb2 backend too. I tried
to get a crash backtrace, but got only a useless log (with and without JIT):
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0xfffffffc in ?? ()

This memory address is completely invalid, no idea what happens before the crash.

I don't and won't have more time to investigate this issue, maybe 
GTK guys have, AFAIK they are interested in a working ARMv7 backend.

[Mark Lam]

Comment on attachment 297966 [details]
proposed patch: with gtk/efl build fixes.

Taking out of review until after I do some due diligence on whether JSValues are still passed in registers on all ports with this change.

[Carlos Alberto Lopez Perez]

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 297966 [details]
> > proposed patch: with gtk/efl build fixes.
> 
> I checked this patch on ARM Linux. (JSCOnly port) It works fine on AArch64,
> but almost all tests crash on ARMv7 with ARM and Thumb2 backend too. I tried
> to get a crash backtrace, but got only a useless log (with and without JIT):
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  0xfffffffc in ?? ()
> 
> This memory address is completely invalid, no idea what happens before the
> crash.
> 
> I don't and won't have more time to investigate this issue, maybe 
> GTK guys have, AFAIK they are interested in a working ARMv7 backend.

I did some tests on an x86 chroot (32 bits) and I confirm this patch breaks JSC on WebKitGTK+:

Before the patch:

./WebKitBuild/Release/bin/jsc
>>> a
Exception: ReferenceError: Can't find variable: a


After the patch:

./WebKitBuild/Release/bin/jsc
>>> a
Segmentation fault (core dumped)


I'm attaching the backtrace for the coredump generated

[Carlos Alberto Lopez Perez]

Created attachment 298213 [details]
Backtrace from the crash on JSC on a x86 chroot with a WebKitGTK+ build

[Carlos Alberto Lopez Perez]

(In reply to comment #8)
> I did some tests on an x86 chroot (32 bits) and I confirm this patch breaks
> JSC on WebKitGTK+:
> 

The issue is not reproducible with WebKitGTK+ on x86_64... so it seems something that only happens on 32-bit platforms.

[Mark Lam]

(In reply to comment #10)
> The issue is not reproducible with WebKitGTK+ on x86_64... so it seems
> something that only happens on 32-bit platforms.

Carlos, for my reference, what version of GCC were you using to do your 32-bit testing?

[Carlos Alberto Lopez Perez]

(In reply to comment #11)
> (In reply to comment #10)
> > The issue is not reproducible with WebKitGTK+ on x86_64... so it seems
> > something that only happens on 32-bit platforms.
> 
> Carlos, for my reference, what version of GCC were you using to do your
> 32-bit testing?

gcc version 4.9.2 
Its the default on Debian 8