Created attachment 511228 [details] [diff] [review] 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.
Created attachment 511542 [details] [diff] [review] 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?
Attachment #511228 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.