633033 - Add assertions against missing error reports

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

shell/js.cpp doesn't actually check the return value of JS_ExecuteScript and such. It shouldn't have to, because those APIs, when used as the shell uses them, should always either succeed or call the error reporter and fail.

However, sometimes there's a bug like bug 633020 where, say, a getter or setter returns false without reporting an error. To the script, that behaves like an uncatchable error. To the shell, it's success. That is really bad if you're trying to write a test. Because the js executable returns exit code 0 either way, the test framework doesn't tell the difference.

We should be asserting throughout the engine that we don't return false without an exception pending or an error reported.

Attached is a sketch of this idea -- but some js/src/tests trip the assertions and I don't have time today to look at them. Maybe my assertions are too strict, or maybe some code is bending the rules for no good reason and can be straightened out.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: WIP 2 - passes tests

The tests that were failing were:

  - Ones that use timeout() or quit(), shell functions that return false
    without reporting an error.

    I added a JS_SilentFailure API for these. The new rule is, unreported errors
    are allowed, but only if you explicitly tell the engine you're doing it.

  - A test that tickles an error path in js::DefineConstructorAndPrototype
    (formerly js_InitClass) where it can call into application callbacks
    with an exception already pending. The function defines a property early
    on which it must delete if an error occurs later. It really just shouldn't
    be written that way.

    For this patch, I fixed it by making that code set the pending exception
    aside before deleting the property, ignore any errors that happen
    while deleting the property, and then restore the original pending
    exception. However for review I will rework the function a little bit.

Anyway. This could land, and I'm interested in landing something like it post-ff4. But the argument could be made that this is a bit too B&D. Feedback?

Comment 2

[Steven DeTar [:sdetar]]

This seems to be mostly fixed already.