Bug 75602 – [JSC] Short circuit for a 16 bit pattern character and an 8 bit string failed

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Hojong Han]

In case of ARM_TRADITIONAL, crash occurs like below when loading http://v.youku.com/v_playlist/f16850225o1p0.html

#0  0xfffffffe in ?? ()
No symbol table info available.
#1  0x419d160c in execute (jitObject=<value optimized out>, input=<value optimized out>, start=<value optimized out>, 
    length=<value optimized out>, output=0x0)
    at /home/SOURCE/webkit-efl/Source/JavaScriptCore/yarr/YarrJIT.h:74
No locals.
#2  JSC::Yarr::execute (jitObject=<value optimized out>, input=<value optimized out>, start=<value optimized out>, 
    length=<value optimized out>, output=0x0)
    at /home/SOURCEwebkit-efl/Source/JavaScriptCore/yarr/YarrJIT.cpp:2505

The problem is caused by the instruction which loads the invalid branch target(0xffffffff).
That instruction is append to the assembler buffer while generating greedy pattern character about a 16 bit pattern character and an 8 bit string.
Updating the branch target for that instruction is necessary but it's not in any where.

[Hojong Han]

Created attachment 121366 [details]
Patch

[Hojong Han]

Created attachment 121368 [details]
Patch

[Gavin Barraclough]

Comment on attachment 121368 [details]
Patch

Looks right to me.  Good catch!

[WebKit Review Bot]

Comment on attachment 121368 [details]
Patch

Clearing flags on attachment: 121368

Committed r108456: <http://trac.webkit.org/changeset/108456>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(In reply to comment #5)
> All reviewed patches have been landed.  Closing bug.

Looks like this broke fast/regex/pcre-test-4.html :

--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/regex/pcre-test-4-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/regex/pcre-test-4-actual.txt 
@@ -70,11 +70,11 @@
 PASS regex34.exec(input0); is results
 PASS regex35.exec(input0); is results
 PASS regex36.exec(input0); is results
-PASS regex37.exec(input0); is results
+FAIL regex37.exec(input0); should be . Was null.
 PASS regex37.exec(input1); is results
 PASS regex38.exec(input0); is results
 PASS regex38.exec(input1); is results
-PASS regex39.exec(input0); is results
+FAIL regex39.exec(input0); should be . Was null.
 PASS regex39.exec(input1); is results
 PASS regex40.exec(input0); is results
 PASS regex41.exec(input0); is results
@@ -215,7 +215,7 @@
 PASS regex104.exec(input0); is results
 PASS regex105.exec(input0); is results
 PASS regex105.exec(input1); is results
-PASS regex106.exec(input0); is results
+FAIL regex106.exec(input0); should be a. Was null.
 PASS regex107.exec(input0); is results
 PASS input0.match(regexGlobal0); is results
 PASS input1.match(regexGlobal0); is results

[Csaba Osztrogonác]

Reopen, because it fails on SL and Qt too.

[Hojong Han]

(In reply to comment #7)
> Reopen, because it fails on SL and Qt too.

That test didn't even run in my environment, arm & linux, before the patch was applied.
Now I'm finding out the reason why the returned value is null, not string "".

[Csaba Osztrogonác]

Rollout landed in http://trac.webkit.org/changeset/108484

[Hojong Han]

Created attachment 128382 [details]
Patch

[Hojong Han]

(In reply to comment #9)
> Rollout landed in http://trac.webkit.org/changeset/108484
I'm really sorry for rollout despite your grateful review and comments.

In the previous patch, I had focused on the short circuit necessary to be linked.
But there's one thing missed is that short circuit is not necessary for the pattern character greedy although it's helpful for non-greedy.

I ran regular expression tests. Broken results are fine with new patch.

[Hojong Han]

Created attachment 128449 [details]
Patch

[Michael Saboff]

Comment on attachment 128449 [details]
Patch

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

r+

> Source/JavaScriptCore/yarr/YarrJIT.cpp:819
> +        if (!((ch > 0xff) && (m_charSize == Char8))) {

Please add a comment similar to what was removed.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:879
> +            JumpList nonGreedyFailures;

Add a comment here as well.

[Hojong Han]

Created attachment 128617 [details]
Patch

[Hojong Han]

(In reply to comment #13)
> (From update of attachment 128449 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128449&action=review
> 
> r+
> 
> > Source/JavaScriptCore/yarr/YarrJIT.cpp:819
> > +        if (!((ch > 0xff) && (m_charSize == Char8))) {
> 
> Please add a comment similar to what was removed.
> 
> > Source/JavaScriptCore/yarr/YarrJIT.cpp:879
> > +            JumpList nonGreedyFailures;
> 
> Add a comment here as well.

Thank you for your review.
I added comments, and uploaded new patch that has more comments on what you reviewed.
Didn't I need to upload the patch again?

[WebKit Review Bot]

Comment on attachment 128617 [details]
Patch

Clearing flags on attachment: 128617

Committed r108753: <http://trac.webkit.org/changeset/108753>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.