Bug 75602 - [JSC] Short circuit for a 16 bit pattern character and an 8 bit string failed
Summary: [JSC] Short circuit for a 16 bit pattern character and an 8 bit string failed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 79223
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-04 21:57 PST by Hojong Han
Modified: 2012-02-24 01:49 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.08 KB, patch)
2012-01-05 16:42 PST, Hojong Han
no flags Details | Formatted Diff | Diff
Patch (2.07 KB, patch)
2012-01-05 16:51 PST, Hojong Han
no flags Details | Formatted Diff | Diff
Patch (2.70 KB, patch)
2012-02-22 20:59 PST, Hojong Han
no flags Details | Formatted Diff | Diff
Patch (3.01 KB, patch)
2012-02-23 03:24 PST, Hojong Han
no flags Details | Formatted Diff | Diff
Patch (3.49 KB, patch)
2012-02-23 17:33 PST, Hojong Han
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hojong Han 2012-01-04 21:57:36 PST
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.
Comment 1 Hojong Han 2012-01-05 16:42:39 PST
Created attachment 121366 [details]
Patch
Comment 2 Hojong Han 2012-01-05 16:51:51 PST
Created attachment 121368 [details]
Patch
Comment 3 Gavin Barraclough 2012-02-21 23:24:35 PST
Comment on attachment 121368 [details]
Patch

Looks right to me.  Good catch!
Comment 4 WebKit Review Bot 2012-02-22 00:43:37 PST
Comment on attachment 121368 [details]
Patch

Clearing flags on attachment: 121368

Committed r108456: <http://trac.webkit.org/changeset/108456>
Comment 5 WebKit Review Bot 2012-02-22 00:43:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Philippe Normand 2012-02-22 00:58:16 PST
(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
Comment 7 Csaba Osztrogonác 2012-02-22 04:14:41 PST
Reopen, because it fails on SL and Qt too.
Comment 8 Hojong Han 2012-02-22 04:23:49 PST
(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 "".
Comment 9 Csaba Osztrogonác 2012-02-22 05:09:39 PST
Rollout landed in http://trac.webkit.org/changeset/108484
Comment 10 Hojong Han 2012-02-22 20:59:20 PST
Created attachment 128382 [details]
Patch
Comment 11 Hojong Han 2012-02-22 21:13:56 PST
(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.
Comment 12 Hojong Han 2012-02-23 03:24:24 PST
Created attachment 128449 [details]
Patch
Comment 13 Michael Saboff 2012-02-23 09:58:08 PST
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.
Comment 14 Hojong Han 2012-02-23 17:33:26 PST
Created attachment 128617 [details]
Patch
Comment 15 Hojong Han 2012-02-23 17:45:09 PST
(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?
Comment 16 WebKit Review Bot 2012-02-24 01:49:17 PST
Comment on attachment 128617 [details]
Patch

Clearing flags on attachment: 128617

Committed r108753: <http://trac.webkit.org/changeset/108753>
Comment 17 WebKit Review Bot 2012-02-24 01:49:23 PST
All reviewed patches have been landed.  Closing bug.