479430 - Missing operation callback checks

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Attachment: v1

Currently JSOP_STOP, JSOP_RETRVAL and JSOP_THROW implementation in js_Interpret() in contrast to JSOP_RETURN does not check if its time to call the operation callback. This allows to delay the callback invocation for an arbitrary period of time using recursive functions. 

For example, in the following example the timeout function does not terminate the script in js shell in about 10ms with exit code 6 because the function f ends in JSOP_STOP. As such the example will not stop until it calls the function 2**101 - 1 times.

timeout(0.01);

function f(n)
{
    if (n != 0) {
        f(n - 1);
        f(n - 1);
    }
}

f(100);

Here is the example dealing with JSOP_THROW:

timeout(0.01);

function f(n)
{
    if (n != 0) {
        try { f(n - 1); } catch (e) {}
        try { f(n - 1); } catch (e) {}
    }
    throw 0;
}

f(100);

And with JSOP_RETRVAL:

timeout(0.01);

function f(n)
{
    if (n != 0) {
        f(n - 1);
        f(n - 1);
    }
    try {
        return 0;
    } finally {
        n++;
    }
}

f(100);

The attached patch fixes this adding the missing CHECK_BRANCH to the opcode implementations in js_Interpret().

Comment 1

[Igor Bukanov]

The previous patch is not enough. The code should check for the operation callback in catch and finally handlers as the exception can be thrown at an arbitrary place. Here is 2 more examples that shows the problem even with the above patch:

catch must check:

timeout(0.01);

function f(n)
{
    if (n != 0) {
        try { f(n - 1); } catch (e) {}
        try { f(n - 1); } catch (e) {}
    }
    name_that_does_not_exists;
}

f(100);

finally must check:

timeout(0.01);

function f(n)
{
    if (n != 0) {
        try { f(n - 1); } finally { f(n - 1); }
    }
    name_that_does_not_exists;
}

f(100);

Comment 2

[Igor Bukanov]

Attachment: v2

The new patch adds the operation callback checks to the exception handlers.

It also adds missing JS_ClearPendingExceptions to the operation callback calls to make sure that a script can not catch the cancellation if the callback was called with an exception pending.

Comment 3

[Jeff Walden [:Waldo]]

Aha, this is what was causing the hang in bug 416912!  Do carry on here... :-)

Comment 4

[Andreas Gal :gal]

Comment on attachment 363313 [details] [diff] [review]
v2

> 
>     nsAutoMonitor mon(pool->Monitor());
>     mon.Wait();
>-  } while (1);
>-
>-  NS_NOTREACHED("Shouldn't get here!");
>-  return JS_FALSE;
>+  }
> }

This is needed for gcc. gcc can't tell the return path is never hit and complains.

Comment 5

[Igor Bukanov]

Andreas Gal wrote in comment #4:

> >-  } while (1);
> >-
> >-  NS_NOTREACHED("Shouldn't get here!");
> >-  return JS_FALSE;
> >+  }
> > }
> 
> This is needed for gcc. gcc can't tell the return path is never hit and
> complains.

Hm, what version of GCC generates the warnings? I do not see them with 4.2 or 4.3. Also note that SM code in several places also uses loops that exit only via return statements without an extra return after the loop. I have not heard about the warnings for such code.

In any case, to be sure the patch changes the "do { ... } while (1)" loop into "for (;;) { ... }" to simplify the flow analysis for a compiler and silence potential complains from VC2007 as at a sufficiently high level of warnings that compiler starts to yell that the condition is always true in "while (1)".

Comment 6

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/83b0d493732e

I nominate the bug for 1.9.1 as the bug 416912 shows that these missing callback calls can leads to unresponsive browser with real-life scripts.

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/83b0d493732e

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0727e2d18b07

Comment 10

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/d3a88b2d7fc0

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-479430-01.js,v  <--  regress-479430-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-479430-02.js,v  <--  regress-479430-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-479430-03.js,v  <--  regress-479430-03.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-479430-04.js,v  <--  regress-479430-04.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_8_1/regress/regress-479430-05.js,v  <--  regress-479430-05.js
initial revision: 1.1

Comment 11

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2