Closed Bug 479430 Opened 14 years ago Closed 14 years ago

Missing operation callback checks

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: testcase, verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
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().
Attachment #363305 - Flags: review?(gal)
Attachment #363305 - Attachment is obsolete: true
Attachment #363305 - Flags: review?(gal)
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);
Attached patch v2Splinter Review
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.
Assignee: gal → igor
Attachment #363313 - Flags: review?(gal)
Attachment #363305 - Flags: review+
Attachment #363305 - Flags: review+
Aha, this is what was causing the hang in bug 416912!  Do carry on here... :-)
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.
Attachment #363313 - Flags: review?(gal) → review+
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)".
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.
Flags: blocking1.9.1?
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/83b0d493732e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: blocking1.9.1? → blocking1.9.1+
Flags: in-testsuite?
Keywords: testcase
Duplicate of this bug: 459398
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
Flags: in-testsuite? → in-testsuite+
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.