Closed Bug 248065 Opened 20 years ago Closed 20 years ago

Teardown on script error kills all script "threads"

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: simonl, Unassigned)

Details

(Keywords: regression)

Attachments

(4 files)

Reporter's testcase
394 bytes, text/html
Details
proposed js/src fix, for use by dom patch
2.55 KB, patch
shaver
: review+
jst
: superreview+
Details | Diff | Splinter Review
DOM patch to use the new JS_ReportPendingException().
44.52 KB, patch
bzbarsky
: review-
bzbarsky
: superreview-
Details | Diff | Splinter Review
Make sure nsContentUtils::Init() fails if the XPConnect service can't be loaded.
45.74 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040413 Debian/1.6-5 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040413 Debian/1.6-5 This bug exists in mozilla-1.6 and probably all later, at least 1.7. --- Scenario --- Before body.onload, a script can use document.writeln() to insert stuff. The interesting part is what happens if it inserts another script, fx: <!-- The "inserting" script is static --> <script> // document.writeln() generates the "inserted" script document.writeln("<script>alert('executing inserted script')</script>"); alert('done inserting'); </script> Eventually the user will see both dialogs. The interesting part is that it might happen in any order (this is not the bug): 1) Deferred execution: In some cases, the inserted script is deferred. The inserting script executes to completion, then the inserted script is executed. 2) Immediate execution: In other cases, the inserted script is executed *immediately*. The inserting script is really interrupted, and a new "thread" is started, executing the inserted script. In Mozilla's case it has the rule has traditionally been: - If the inserted script is inline (as above), it is executed immediately. - If the inserted script is external (<script src="..."></script>), it is executed deferred. I have tested the behaviour in all browsers since I rely on it, and they all behave differently. --- The bug --- The important property that I rely on is: - When inserted script is executed immediately - and it fails with a script error - the inserting script will continue from where it was interrupted, as if the inserted script had exited normally. It has always been that way, in all browsers. Now, except mozilla-1.6 and 1.7 (tested under Debian Linux and Windows XP SP1). In mozilla-1.6+, when an inserted script is executed immediately, and it fails, it also takes down the "main thread"; the inserting script. The inserting script is simply never re-entered. It seems that the error handler is to greedy when cleaning up. It takes down the stack of the inserted script, which it should, but apparently it also takes down the stack of the inserting script, which it should not. Reproducible: Always Steps to Reproduce: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> <html><head> <title>Teardown on script error is too greedy</title> <meta http-equiv="Content-Script-Type" content="text/javascript"> </head><body> <script> var s = "<"+"script"+">"+"no_such_function();"+"<"+"/"+"script"+">"; document.writeln(s); alert("main thread continuing"); </script> </body></html> Actual Results: JavaScript Console reports that no_such_function does not exist, correct. Then nothing happens. Note that if you instead insert a <script src="..."></script> (an external script that contains a script error), the dialog is shown. Expected Results: After the inserted script has failed, the "main thread" should continue, showing the alert(). Mozilla-1.5 and earlier does the expected, as does every other browser I currently test on: Netscape-4.8/Linux, Opera-7.51/Linux, Konqueror-3.2.2/Linux, Internet Explorer 4, 5, 5.5 and 6 on various Windows platforms. There was a bug before 1.6: If an inserted script was executed immediately, and it had a script error, it would not be reported to the JavaScript Console. This is fixed in 1.6. I suspect the fix may have introduced the new brokenness.
Please assign DOM bugs properly. /be
Assignee: general → general
Component: JavaScript Engine → DOM: Level 0
Confirming. jst, peterv, any clue what's up here?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Regression from bug 224549, or bug 226617. Brendan, we need an API to tell the JS engine to report and clear pending exceptions while code is still running on a context. Or is there a better way to fix this?
jst, the current API has JS_GetPendingException(JSContext *cx, jsval *vp); JS_SetPendingException(JSContext *cx, jsval v); JS_ClearPendingException(JSContext *cx); JS_IsExceptionPending(JSContext *cx); These ought to be enough to do the job. It seems that a generated script should not propagate any exception that it suffers to the generator. /be
Brendan, the problem is that we're in a situation here where we're executing script from within running JS, on the same context, and we want the errors in the script we're executing to not in any way interfere with the calling script. Currently the place where the JS engine reports script errors to the embedder is when the outer-most script is finished, and only if *it* failed. In this case that's at http://lxr.mozilla.org/mozilla/source/js/src/jsapi.c#3584. But when the actual error happens in the nested script, it's not reported when the nested script is finished since there's still running code on the context. To fix this bug, we'd need to clear pending exceptions after executing inline scripts from JS and make sure that those pending exceptions get reported too.
Duh, you want something like JS_ReportUncaughtException, doncha? Patch coming up. /be
Yah, that'd be fantastic!
The error-as-exception API and implementation is kinda busted (see bug 215173, bug 232182, and bug 243869). JS_ErrorFromException already exists, and maybe you could use it, but I thought I'd fix the bogus false return in jsexn.c (no error, should return true), and add JS_ReportPendingException while I was at it. /be
Attachment #153108 - Flags: review?(shaver)
Attachment #153108 - Flags: review?(shaver) → review+
Comment on attachment 153108 [details] [diff] [review] proposed js/src fix, for use by dom patch sr=jst fwiw.
Attachment #153108 - Flags: superreview+
This makes the script loader report pending exceptions after executing a script, and it also makes a bunch of code that used to get the XPConnect servive use the one we've got cached in nsContentUtils. Only the nsContentUtils.h and nsScriptLoader.cpp changes are needed to fix this bug.
Attachment #153117 - Flags: superreview?(bzbarsky)
Attachment #153117 - Flags: review?(bzbarsky)
Comment on attachment 153117 [details] [diff] [review] DOM patch to use the new JS_ReportPendingException(). Need a JS_ClearPendingException() call after the Report? /be
That's done at the end of NS_ScriptErrorReporter(): http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#303 With a nice XXX comment. Not something I'd want to go change w/o digging deep...
Oy. I'll try to look at this in the next few days, I guess, but I'm still digging out from the pile of review requests I picked up over the last month...
jst: I'll take care of that XXX -- it seems to me that once the engine has been told (JS_ReportPendingException), or has decided (unwinding from a JS_Evaluat*, JS_Execute*, or JS_Call* API with null cx->fp) to report a pending exception as an error, the engine should bloody well clear the pending exception. Shaver, what do you say? This error-as-exception code from years ago is still getting designed into shape, alas, but there is light at the end of the tunnel. /be
If the error report was successfully performed, I guess that's OK, though I worry about API misuse that just drops an exception somewhere. Though that's no worse than catch (e) { }, so yeah. Go for it.
Comment on attachment 153117 [details] [diff] [review] DOM patch to use the new JS_ReportPendingException(). Sorry this took me so long; I was thrown by the size of the patch and missed the comment about it being mostly cleanup. There's one problem here, I think -- nsContentUtils can init successfully even if sXPConnect is null. If that happens, some of the callers of XPConnect() you added will crash. Some would have crashed anyway, but some did proper null-checks on the pointer GetService returned.... We should either fail init of nsContentUtils if we can't get XPConnect, or check the retval of XPConnect() at all callers (and rename it to GetXPConnect(), perhaps, since XPConnect() implies non-nullness). Please fix that? I promise the next review will be _much_ faster!
Attachment #153117 - Flags: superreview?(bzbarsky)
Attachment #153117 - Flags: superreview-
Attachment #153117 - Flags: review?(bzbarsky)
Attachment #153117 - Flags: review-
(In reply to comment #17) > (From update of attachment 153117 [details] [diff] [review]) > Sorry this took me so long; I was thrown by the size of the patch and missed > the comment about it being mostly cleanup. No problem. > There's one problem here, I think -- nsContentUtils can init successfully even > if sXPConnect is null. If that happens, some of the callers of XPConnect() you > added will crash. Some would have crashed anyway, but some did proper > null-checks on the pointer GetService returned.... > > We should either fail init of nsContentUtils if we can't get XPConnect, or > check the retval of XPConnect() at all callers (and rename it to > GetXPConnect(), perhaps, since XPConnect() implies non-nullness). Yeah... I'll make nsContentUtils::Init() fail if we can't get the nsIXPConnect service. The only case where that might matter is, as the comment says, if someone uses our DOM code as a standalone library, and that's not likely to be possible any time soon, for lots of reasons. We'll worry about that once we get there. New patch coming up.
Comment on attachment 157584 [details] [diff] [review] Make sure nsContentUtils::Init() fails if the XPConnect service can't be loaded. r+sr=bzbarsky
Attachment #157584 - Flags: superreview+
Attachment #157584 - Flags: review+
FIXED.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: