Closed
Bug 439110
Opened 13 years ago
Closed 12 years ago
xpcshell's load() just silently fails for non-existent files
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: hwaara, Assigned: crowderbt)
References
Details
(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b4])
Attachments
(2 files, 7 obsolete files)
1.41 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
869 bytes,
patch
|
Details | Diff | Splinter Review |
If you use do_import_script(file) on a file that doesn't exist, no exception or anything is thrown. The only hint you get is "ReferenceError: theFunction not defined" errors for the symbols you try to use. The reason is because do_import_script() uses xpcshell's load() to load the JS file, which just returns false on error (see http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/shell/xpcshell.cpp#221).
Reporter | ||
Comment 1•13 years ago
|
||
Who's a good reviewer for this patch?
Attachment #325005 -
Flags: review?
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 325005 [details] [diff] [review] Make do_import_script() fail if load() fails Crowder, please punt to someone else if you're not a good reviewer for this. (I found you in CVS blame)
Attachment #325005 -
Flags: review? → review?(crowder)
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 325005 [details] [diff] [review] Make do_import_script() fail if load() fails I am not a great reviewer for this (because I don't think I am an XPCOM peer, and I -think- this qualifies as XPCOM code); I'm guessing jst would be? That said, the code is simple and the change is trivial and obvious. In my opinion, the patch is fine.
Attachment #325005 -
Flags: review?(crowder) → review?(jst)
Reporter | ||
Comment 4•13 years ago
|
||
Comment on attachment 325005 [details] [diff] [review] Make do_import_script() fail if load() fails Sorry, this patch seems to be wrong. :-( Looking for a better change.
Attachment #325005 -
Attachment is obsolete: true
Attachment #325005 -
Flags: review?(jst)
Assignee | ||
Comment 5•13 years ago
|
||
For my edification, what's wrong with it?
Reporter | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > For my edification, what's wrong with it? *Every* load() seems to return false, so I guess my assumption that it returned that only error was wrong.
Assignee | ||
Comment 7•13 years ago
|
||
Ah, looking at xpcshell.cpp, it's load routine never sets *rval. Also, it doesn't seem to check the result of fopen(). Basically, when Load fails, it stops JS execution. You may need more robust error-handling in this routine.
Reporter | ||
Comment 8•13 years ago
|
||
This patch makes the success status that we get from JS, propagate into the |rval| as a boolean. This makes load() return true or false, based on success status. load() now also correctly handles fopen() errors. Apart from that, there should be no difference in the control flow.
Attachment #325018 -
Flags: review?
Reporter | ||
Updated•13 years ago
|
Attachment #325018 -
Attachment description: Make xpcshell's load() propagate script erros by returning true/false → Make xpcshell's load() propagate script errors by returning true/false
Reporter | ||
Updated•13 years ago
|
Attachment #325018 -
Flags: review? → review?(jst)
Reporter | ||
Comment 9•13 years ago
|
||
OK, still one problem left. If Load() returns JS_FALSE, then the JS engine caller (js_Invoke) will not care about its return value (*rval). See http://mxr.mozilla.org/mozilla/source/js/src/jsinterp.c#1343 Please, I need some JSEng hand-holding here. Is the right thing to do to make Load() always return JS_TRUE and set *rv to whether we failed or succeeded? That seems to make everything work correctly here.
Assignee | ||
Comment 10•13 years ago
|
||
Comment on attachment 325018 [details] [diff] [review] Make xpcshell's load() propagate script errors by returning true/false The way this patch works, things that used to stop execution will now simply be reported as errors. You probably still want if (!str) return JS_FALSE, and if (!script) return JS_FALSE, since these are series error conditions. It would also be nice to be able to propogate the JS_ExecuteScript result in some fashion.
Assignee | ||
Comment 11•13 years ago
|
||
By returning JS_FALSE, you're signaling to the engine that it must stop executing. If you want your script to continue, and to be allowed to handle the return value you've placed in *vp, then you must always return JS_TRUE. I think in this case, you want a mix of both. For series errors like JS_ValueToString or script compilation errors, you likely want to either return JS_FALSE, or else actually throw an exception (and then return JS_TRUE). Alternatively, you might want to report an error (using JS_ReportError()) and then return JS_TRUE anyway (just to get an error in your error-log). Perhaps experimenting with these alternatives a little bit will show you what you want?
Reporter | ||
Comment 12•13 years ago
|
||
I think it would be ideal if I could figure out how to throw an exception in all error cases, and also stop execution for the "serious errors" you mention. Any idea how I do that?
Assignee | ||
Comment 13•13 years ago
|
||
Copied this off of m.d.t.js-eng, so excuse the formatting. Note that you can actually make your exception object be anything you like; this just makes an exception object that has all the attributes of a native JavaScript exception (ie., has a .stack member that you can peek at). JSBool throwException(JSContext *cx, const char *exception, const char *str) { jsval arg, myError; arg = STRING_TO_JSVAL(JS_NewStringCopyZ(cx, str)); JS_CallFunctionName(cx, globalObj, exception, 1, &arg, &myError); JS_SetPendingException(cx, myError); return JS_FALSE; } You must then return JS_FALSE from the function the engine is calling.
Assignee | ||
Comment 14•13 years ago
|
||
I lied before when I said (and then return JS_TRUE), sorry. :)
Comment 15•13 years ago
|
||
Should check for failure (null return) from JS_NewStringCopyZ, also for false return from JS_CallFunctionName. Bail with false return in either case from throwException (suggest ThrowException for C static naming convention observance in xpcshell.cpp). Is the same bug in js.cpp in js/src? /be
Comment 16•13 years ago
|
||
(In reply to comment #12) > I think it would be ideal if I could figure out how to throw an exception in > all error cases, and also stop execution for the "serious errors" you mention. > Any idea how I do that? Propagating a false or null return from one of the API functions you call will automatically preserve the pending exception set by that API (unless the error was out of memory or equivalent, which is reported right away). An uncaught exception is reported when control flow unwinds from the outermost API call active on a cx. /be
Assignee | ||
Comment 17•13 years ago
|
||
js.cpp's load does not use a file handle, but instead uses JS_CompileFile() and propogates its return value. That's probably a good mod for this case, also.
Reporter | ||
Updated•13 years ago
|
Attachment #325018 -
Attachment is obsolete: true
Attachment #325018 -
Flags: review?(jst)
Reporter | ||
Comment 18•13 years ago
|
||
Thanks for all the pointers. This patch throws an exception of type Error (is EvalError or something else more appropriate?) for any problems involving a load().
Attachment #325090 -
Flags: review?(brendan)
Reporter | ||
Comment 19•13 years ago
|
||
What is js.cpp? A new JS shell? I know it's something on mozilla-central only (which I do not have a tree for yet)... If it's simple to build and test, I can probably check it out to do the changes. If not, I'd prefer someone else do it.
Reporter | ||
Comment 20•13 years ago
|
||
Oops, fix too early return which would leak the error string.
Attachment #325090 -
Attachment is obsolete: true
Attachment #325092 -
Flags: review?(brendan)
Attachment #325090 -
Flags: review?(brendan)
Assignee | ||
Comment 21•13 years ago
|
||
Comment on attachment 325092 [details] [diff] [review] Make xpcshell's load() throw exception on error v2 Might be cleaner, long-term, to make ThrowException variadic: static JSBool ThrowException(JSContext *cx, JSObject *obj, const char exception, const char *fmt, ...) { va_list ap; char *str; jsval arg, myError; va_start(ap, fmt); str = JS_vsmprintf(fmt, ap); va_end(ap); if (!str) return JS_FALSE; // Using NewString here means we're yielding ownership // of the allocation made by JS_vsmprintf, except in case // of failure, in which case we must free (GrowStuff uses // malloc(), not JS_malloc(), so we free(), rather than // calling JS_free() JSString *errStr = JS_NewString(cx, str, strlen(str)); if (!errStr) { free(str); return JS_FALSE; } arg = STRING_TO_JSVAL(errStr); if (JS_CallFunctionName(cx, obj, exception, 1, &arg, &myError)) JS_SetPendingException(cx, myError); return JS_FALSE; } Then, when you need to throw: return ThrowException(cx, obj, "Error", "Failed to load(\"%s\")", filename); I haven't tested all this to compile, but it should be close.
Comment 22•13 years ago
|
||
I don't think what I wrote in comment 16 was attended to: "Propagating a false or null return from one of the API functions you call will automatically preserve the pending exception set by that API (unless the error was out of memory or equivalent, which is reported right away)." Thus there's no need for ok = JS_FALSE; break if JS_ValueToString(cx, argv[i]) fails, and it's wrong to clobber the pending exception already set (unless OOM) by JS_ValueToString with a generic one you create at the bottom of Load. I also do not know why we need to diverge from js.cpp (js.c in cvs.mozilla.org), the original shell here. Let's try to make them match, so we can unify them one way or another (bug 209176). Crowder, can you handle review chores here? Or get mrbkap to do it, he's a friend of the shells. /be
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → crowder
Assignee | ||
Comment 23•13 years ago
|
||
Comment on attachment 325092 [details] [diff] [review] Make xpcshell's load() throw exception on error v2 New patch shortly.
Attachment #325092 -
Flags: review?(brendan)
Assignee | ||
Comment 24•13 years ago
|
||
This is the meat of Load() from js.cpp, which propagates an exception when the file does not exist by simply carrying over the error reported by JS_CompileFile.
Attachment #325092 -
Attachment is obsolete: true
Attachment #325450 -
Flags: review?(brendan)
Assignee | ||
Comment 25•13 years ago
|
||
Bugzilla barfed on me while I was submitting the patch, so I have no idea if anyone got emailed.
Updated•13 years ago
|
Attachment #325450 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 26•13 years ago
|
||
Comment on attachment 325450 [details] [diff] [review] identical to js.cpp:Load() Not sure if I need sr+ here or not. If I don't, feel free to just bounce this, jst.
Attachment #325450 -
Flags: superreview?(jst)
Updated•13 years ago
|
Attachment #325450 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 27•13 years ago
|
||
Landed: 15424:f201baf7bf04
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Summary: do_import_script("non_existing_file.js") just silently fails → xpcshell's load() just silently fails for non-existent files
Assignee | ||
Comment 29•13 years ago
|
||
This introduced some unit-test problems, I think, so I backed it out. Brief log: http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla2/1213825343.1213827694.28907.gz Full log: http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla2/1213825343.1213827694.28907.gz&fulltext=1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 30•13 years ago
|
||
http://hg.mozilla.org/index.cgi/mozilla-central/rev/3a2a9e0c5c63
Updated•12 years ago
|
Status: REOPENED → NEW
Flags: wanted1.9.1?
Updated•12 years ago
|
Component: Testing → TUnit
Flags: wanted1.9.1?
Product: Core → Testing
QA Contact: testing → tunit
Version: Trunk → unspecified
Updated•12 years ago
|
Component: TUnit → XPConnect
Product: Testing → Core
QA Contact: tunit → xpconnect
Updated•12 years ago
|
Flags: wanted1.9.1?
Version: unspecified → Trunk
Comment 31•12 years ago
|
||
(In reply to comment #29) > This introduced some unit-test problems, I think, so I backed it out. Is the patch wrong somehow ? Or is it "only" that tests need to be updated to fix/catch the new exception ? > http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla2/1213825343.1213827694.28907.gz > http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla2/1213825343.1213827694.28907.gz&fulltext=1 These links show "empty" logs nowadays :-<
Assignee | ||
Comment 32•12 years ago
|
||
Serge: I don't know for sure, and I didn't have time to debug it in June. I'll try to come back to it in the next few days.
Comment 33•12 years ago
|
||
Could you unbitrot your patch, now that bug 446689 is checked in ?
Blocks: 384339
Assignee | ||
Comment 34•12 years ago
|
||
Refactored slightly to clean up the goofy-looking if (!script) { ... } else { } nonsense. We'll just return false if (!script), now. I have to say I'm still not sure whether landing this will break tests again or not, and I haven't got time to test, sorry. Serge, can you test?
Attachment #325450 -
Attachment is obsolete: true
Attachment #363142 -
Flags: review?(mrbkap)
Comment 35•12 years ago
|
||
Comment on attachment 363142 [details] [diff] [review] unbitrotted r=mrbkap after someone verifies that this doesn't regress unit tests.
Attachment #363142 -
Flags: review?(mrbkap) → review+
Comment 36•12 years ago
|
||
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090219 Minefield/3.2a1pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/46a99365dd85 + bug 439110 patch) |make -C objdir/netwerk/test check| fails. For example: { test_authentication.js TypeError: httpserv.registerPathHandler is not a function test_bug263127.js TypeError: server.start is not a function test_traceable_channel.js TypeError: observer.register is not a function } I'll try to debug it a (little) more later.
Comment 37•12 years ago
|
||
(In reply to comment #36) > (http://hg.mozilla.org/mozilla-central/rev/46a99365dd85 + bug 439110 patch) > > |make -C objdir/netwerk/test check| fails. First, I confirm that, without this patch, these tests succeed.
Comment 38•12 years ago
|
||
(In reply to comment #36) > (http://hg.mozilla.org/mozilla-central/rev/46a99365dd85 + bug 439110 patch) > > |make -C objdir/netwerk/test check| fails. Second, I added fprintf()s, per brian's suggestion: the only file processed is 'httpd.js': JS_CompileFile() succeeds then JS_ExecuteScript() fails.
Comment 39•12 years ago
|
||
(In reply to comment #36) > (http://hg.mozilla.org/mozilla-central/rev/46a99365dd85 + bug 439110 patch) > > |make -C objdir/netwerk/test check| fails. Third, if I remove all lines after |nsHttpServer.prototype = {...}| (line 624) and comment out its |get identity()| then JS_ExecuteScript() succeeds. Same with at least "ServerIdentity.prototype > get primaryScheme()". That's about all the narrowing I know how to do...
Comment 40•12 years ago
|
||
Fwiw, this new |Load()| code seems to be +/- copied (from) at least TestXPC.cpp and js.cpp... http://mxr.mozilla.org/mozilla-central/search?string=JS_CompileFile%5C%28®exp=on&case=on&find=%2Fjs%2Fsrc%2F
Assignee | ||
Comment 41•12 years ago
|
||
This is so much simpler, I had to review this bug again to remind myself why I didn't recommend this in the first place. The assumption that we should not diverge from js.cpp here is bogus, because we need principals to function properly (other routines will later assert that they've been provided). This is simple and correct, I think.
Attachment #363142 -
Attachment is obsolete: true
Attachment #363222 -
Flags: review?(mrbkap)
Assignee | ||
Comment 42•12 years ago
|
||
With the if/else cleanup I wrote last time, and we don't need to check if (file) anymore, thanks sgautherie.
Attachment #363222 -
Attachment is obsolete: true
Attachment #363222 -
Flags: review?(mrbkap)
Assignee | ||
Updated•12 years ago
|
Attachment #363228 -
Flags: review?(mrbkap)
Updated•12 years ago
|
Attachment #363228 -
Flags: review?(mrbkap) → review+
Comment 43•12 years ago
|
||
Comment on attachment 363228 [details] [diff] [review] betterer [Checkin: Comment 46 & 48] This patch passes the TUnit test suite for trunk Firefox :-) As a test, I can get the new error if I change the name of the file in the do_import_script() call: { .../testing/xpcshell/head.js:160: Error: cannot open file '.../netwerk/test/httpserver/httpdSG.js' for reading } Maybe you could add a part of comment 41 as a comment in the file too ?
Assignee | ||
Comment 44•12 years ago
|
||
Thanks for testing, sgautherie, sorry for not doing this right months ago.
Updated•12 years ago
|
Whiteboard: [needs landing !?]
Assignee | ||
Comment 45•12 years ago
|
||
If someone gets a moment to land the "betterer" patch, that would be helpful. Thanks.
Keywords: checkin-needed
Whiteboard: [needs landing !?]
Comment 46•12 years ago
|
||
Comment on attachment 363228 [details] [diff] [review] betterer [Checkin: Comment 46 & 48] http://hg.mozilla.org/mozilla-central/rev/8c5d11efd452
Attachment #363228 -
Attachment description: betterer → betterer
[Checkin: Comment 46]
Comment 47•12 years ago
|
||
"Reminder": (In reply to comment #43) > Maybe you could add a part of comment 41 as a comment in the file too ?
Status: NEW → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Comment 48•12 years ago
|
||
Comment on attachment 363228 [details] [diff] [review] betterer [Checkin: Comment 46 & 48] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e32789c85946
Attachment #363228 -
Attachment description: betterer
[Checkin: Comment 46] → betterer
[Checkin: Comment 46 & 48]
Updated•12 years ago
|
Flags: wanted1.9.1?
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [needs 1.9.1 landing] → [fixed1.9.1b4]
Updated•12 years ago
|
Flags: in-testsuite?
Comment 50•12 years ago
|
||
Here's an xpcshell test. I'll just check this in whenever the tree is green enough.
Comment 51•12 years ago
|
||
Pushed the xpcshell test: http://hg.mozilla.org/mozilla-central/rev/9cac836012c9
Flags: in-testsuite? → in-testsuite+
Updated•12 years ago
|
Attachment #372846 -
Attachment description: test → test
[Checkin: Comment 51]
Comment 52•12 years ago
|
||
Comment on attachment 372846 [details] [diff] [review] test [Checkin: Comment 51] >+ do_check_eq(ex.message.substring(0,16), "cannot open file"); >+ do_check_false(subscriptLoaded, "load() should throw an error"); These functions are not supposed to take a text argument: use dump(). This patch is wanted on 1.9.1 too.
You need to log in
before you can comment on or make changes to this bug.
Description
•