Bug 79040

Summary: (function () { while(true) { } })() runs forever with DFG JIT
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Critical CC: barraclough, fpizlo, gaborb, gergely, ggaren, hausmann, laszlo.gombos, loki, mark.lam, ossy, pvarga, tonikitoo, yong.li.webkit, zherczeg
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 96916    
Bug Blocks: 79668    
Attachments:
Description Flags
Patch
none
Proposed patch none

Description Csaba Osztrogonác 2012-02-20 11:30:56 PST
tst_QWebPage Qt API tests fails after enabling DFG JIT:

QFATAL : tst_QWebPage::infiniteLoopJS() Received signal 15 
FAIL!  : tst_QWebPage::infiniteLoopJS() Received a fatal error.

But unfortunately we didn't notice it before, because zillion API tests fail
and nobody cares with them. And unfortunately timeouts were hidden before
http://trac.webkit.org/changeset/108246

It might be a JSC bug, but we should check it on a small QWebPage independent example.
Comment 1 Csaba Osztrogonác 2012-02-20 22:51:47 PST
Created attachment 127922 [details]
Patch
Comment 2 Csaba Osztrogonác 2012-02-20 22:53:18 PST
(In reply to comment #1)
> Created an attachment (id=127922) [details]
> Patch

It is a workaround to make our bots happier. Without fixing this bug or without this workaround our bots can't be green anymore. (because of stricter rule in master.cfg - http://trac.webkit.org/changeset/108246)
Comment 3 Simon Hausmann 2012-02-21 03:27:38 PST
Filip, it seems that LLint doesn't do the timeout checks for loops, like it's done in op_loop in Interpreter.cpp. Is that a missing feature in llint?
Comment 4 Simon Hausmann 2012-02-21 03:29:02 PST
Comment on attachment 127922 [details]
Patch

rs=me. Let's get this workaround in until we figure out why the timeout don't seem to be getting called with llint.
Comment 5 Csaba Osztrogonác 2012-02-21 04:24:58 PST
I made a little digging, and checked if TimeoutChecker::didTimeOut(ExecState* exec) is called or not.

It is called from tst_QWebPage::infiniteLoopJS()if DFG JIT is disabled.
But it isn't called if DFG JIT is enabled. And it isn't called with
same infinite loop from DRT and QtTestBrowser irrespectively of DFG JIT is enabled or not.
Comment 6 Csaba Osztrogonác 2012-02-21 04:34:02 PST
Comment on attachment 127922 [details]
Patch

Clearing flags on attachment: 127922

Committed r108341: <http://trac.webkit.org/changeset/108341>
Comment 7 Zoltan Herczeg 2012-08-01 03:14:11 PDT
Hey Filip, I think I tracked down this issue. It seems the DFG JIT does not call TimeoutChecker at all, so nothing checks the timeout in case of a simple

(function () { while(true) { } })()

Which essentially stops the browser forever. Any idea how to fix this?
Comment 8 Gavin Barraclough 2012-09-21 01:12:10 PDT
We don't currently support a mechanism to asynchronously interrupt JavaScript execution.  To stop a runaway script in WebKit2, you can kill the web process - this ensures all resources are released.
Comment 9 Zoltan Herczeg 2012-09-21 01:34:32 PDT
We discussed this issue with Philip before, and we decided that we refactor the code. This work is currently in progress.
Comment 10 Gabor Ballabas 2012-09-27 09:36:51 PDT
Created attachment 166022 [details]
Proposed patch

Proposed patch without changelog. It depends on https://bugs.webkit.org/show_bug.cgi?id=96916
Comment 11 Geoffrey Garen 2012-10-30 16:51:19 PDT
Mark Lam is working on a more efficient fix for this.
Comment 12 Zoltan Herczeg 2012-10-30 20:49:10 PDT
Thanks for letting us know!
Comment 13 Csaba Osztrogonác 2012-10-30 22:57:01 PDT
(In reply to comment #11)
> Mark Lam is working on a more efficient fix for this.

:(( 

I don't think if ignoring review at all for a month is the best /
most friendlier way to let us know that you don't want Gábor's fix
and you prefer fixing by yourself.
Comment 14 Geoffrey Garen 2012-10-31 08:52:34 PDT
> I don't think if ignoring review at all for a month is the best /
> most friendlier way to let us know that you don't want Gábor's fix
> and you prefer fixing by yourself.

To clarify, Mark wasn't working on this a month ago.

If you want this patch reviewed, the best thing to do is mark the review flag r? and post performance numbers for a reviewer to evaluate.
Comment 15 Gabor Ballabas 2012-11-13 07:20:38 PST
Sunspider benchmark results on x86_64 Linux:

sunspider:


TEST                   COMPARISON           r134387          Proposed patch       DETAILS

=============================================================================

** TOTAL **:           *1.014x as slow*  208.5ms +/- 0.1%   211.4ms +/- 0.2%     significant

=============================================================================

  3d:                  *1.019x as slow*   32.6ms +/- 0.4%    33.2ms +/- 0.4%     significant
    cube:              *1.050x as slow*   11.5ms +/- 1.1%    12.1ms +/- 0.5%     significant
    morph:             ??                  9.1ms +/- 0.9%     9.3ms +/- 1.1%     not conclusive: might be *1.012x as slow*
    raytrace:          -                  11.9ms +/- 0.5%    11.9ms +/- 0.5% 

  access:              ??                 18.0ms +/- 0.0%    18.0ms +/- 0.2%     not conclusive: might be *1.001x as slow*
    binary-trees:      -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 
    fannkuch:          ??                  8.0ms +/- 0.0%     8.0ms +/- 0.3%     not conclusive: might be *1.002x as slow*
    nbody:             -                   4.0ms +/- 0.0%     4.0ms +/- 0.0% 
    nsieve:            -                   4.0ms +/- 0.0%     4.0ms +/- 0.0% 

  bitops:              1.025x as fast     12.5ms +/- 0.8%    12.2ms +/- 0.7%     significant
    3bit-bits-in-byte: -                   1.0ms +/- 0.0%     1.0ms +/- 0.0% 
    bits-in-byte:      1.057x as fast      5.5ms +/- 1.8%     5.2ms +/- 1.6%     significant
    bitwise-and:       -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 
    nsieve-bits:       -                   4.0ms +/- 0.0%     4.0ms +/- 0.0% 

  controlflow:         -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 
    recursive:         -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 

  crypto:              -                  17.0ms +/- 0.0%    17.0ms +/- 0.1% 
    aes:               -                  10.0ms +/- 0.0%    10.0ms +/- 0.0% 
    md5:               ??                  4.0ms +/- 0.0%     4.0ms +/- 0.5%     not conclusive: might be *1.002x as slow*
    sha1:              -                   3.0ms +/- 0.0%     3.0ms +/- 0.0% 

  date:                ??                 31.0ms +/- 0.1%    31.1ms +/- 0.5%     not conclusive: might be *1.002x as slow*
    format-tofte:      ??                 16.0ms +/- 0.2%    16.1ms +/- 0.8%     not conclusive: might be *1.006x as slow*
    format-xparb:      -                  15.0ms +/- 0.2%    15.0ms +/- 0.3% 

  math:                *1.073x as slow*   18.6ms +/- 0.5%    20.0ms +/- 0.0%     significant
    cordic:            *1.33x as slow*     3.0ms +/- 0.0%     4.0ms +/- 0.0%     significant
    partial-sums:      -                  13.0ms +/- 0.2%    13.0ms +/- 0.0% 
    spectral-norm:     *1.141x as slow*    2.6ms +/- 3.7%     3.0ms +/- 0.0%     significant

  regexp:              -                  11.0ms +/- 0.2%    11.0ms +/- 0.0% 
    dna:               -                  11.0ms +/- 0.2%    11.0ms +/- 0.0% 

  string:              *1.017x as slow*   65.8ms +/- 0.2%    66.9ms +/- 0.4%     significant
    base64:            ??                  5.0ms +/- 0.0%     5.0ms +/- 0.4%     not conclusive: might be *1.002x as slow*
    fasta:             -                  10.0ms +/- 0.0%    10.0ms +/- 0.0% 
    tagcloud:          *1.008x as slow*   16.2ms +/- 0.5%    16.4ms +/- 0.6%     significant
    unpack-code:       *1.025x as slow*   26.5ms +/- 0.4%    27.1ms +/- 0.6%     significant
    validate-input:    *1.042x as slow*    8.1ms +/- 0.6%     8.4ms +/- 1.5%     significant


V8:


TEST              COMPARISON            r134387           Proposed patch     DETAILS

=============================================================================

** TOTAL **:      *1.016x as slow*  748.9ms +/- 0.0%   760.7ms +/- 0.1%     significant

=============================================================================

  v8:             *1.016x as slow*  748.9ms +/- 0.0%   760.7ms +/- 0.1%     significant
    crypto:       *1.107x as slow*   86.8ms +/- 0.1%    96.1ms +/- 0.1%     significant
    deltablue:    *1.009x as slow*  135.4ms +/- 0.1%   136.7ms +/- 0.1%     significant
    earley-boyer: *1.006x as slow*   97.8ms +/- 0.1%    98.4ms +/- 0.2%     significant
    raytrace:     *1.002x as slow*   70.8ms +/- 0.1%    71.0ms +/- 0.1%     significant
    regexp:       1.010x as fast    107.2ms +/- 0.1%   106.1ms +/- 0.1%     significant
    richards:     *1.014x as slow*  113.5ms +/- 0.2%   115.0ms +/- 0.2%     significant
    splay:        -                 137.3ms +/- 0.2%   137.4ms +/- 0.3%
Comment 16 Csaba Osztrogonác 2012-11-21 06:30:00 PST
As far as I know, Gábor doesn't work on it anymore. Could you confirm it?
Comment 17 Gabor Ballabas 2012-11-21 06:33:36 PST
(In reply to comment #16)
> As far as I know, Gábor doesn't work on it anymore. Could you confirm it?

Yes, that's true. As far as I know Mark Lam is working on it now.