439110 - xpcshell's load() just silently fails for non-existent files

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Håkan Waara]

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).

Comment 1

[Håkan Waara]

Attachment: Make do_import_script() fail if load() fails

Who's a good reviewer for this patch?

Comment 2

[Håkan Waara]

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)

Comment 3

[Brian Crowder]

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.

Comment 4

[Håkan Waara]

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.

Comment 5

[Brian Crowder]

For my edification, what's wrong with it?

Comment 6

[Håkan Waara]

(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.

Comment 7

[Brian Crowder]

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.

Comment 8

[Håkan Waara]

Attachment: Make xpcshell's load() propagate script errors by returning true/false

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.

Comment 9

[Håkan Waara]

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.

Comment 10

[Brian Crowder]

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.

Comment 11

[Brian Crowder]

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?

Comment 12

[Håkan Waara]

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?

Comment 13

[Brian Crowder]

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.

Comment 14

[Brian Crowder]

I lied before when I said (and then return JS_TRUE), sorry.  :)

Comment 15

[Brendan Eich [:brendan]]

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

[Brendan Eich [:brendan]]

(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

Comment 17

[Brian Crowder]

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.

Comment 18

[Håkan Waara]

Attachment: Throw exception too

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().

Comment 19

[Håkan Waara]

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.

Comment 20

[Håkan Waara]

Attachment: Make xpcshell's load() throw exception on error v2

Oops, fix too early return which would leak the error string.

Comment 21

[Brian Crowder]

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

[Brendan Eich [:brendan]]

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

Comment 23

[Brian Crowder]

Comment on attachment 325092 [details] [diff] [review]
Make xpcshell's load() throw exception on error v2

New patch shortly.

Comment 24

[Brian Crowder]

Attachment: identical to js.cpp:Load()

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.

Comment 25

[Brian Crowder]

Bugzilla barfed on me while I was submitting the patch, so I have no idea if anyone got emailed.

Comment 26

[Brian Crowder]

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.

Comment 27

[Brian Crowder]

Landed: 15424:f201baf7bf04

Comment 28

[Martijn Wargers (dead)]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/f201baf7bf04

Comment 29

[Brian Crowder]

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

Comment 30

[Brian Crowder]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/3a2a9e0c5c63

Comment 31

[Serge Gautherie (:sgautherie)]

(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 :-<

Comment 32

[Brian Crowder]

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

[Serge Gautherie (:sgautherie)]

Could you unbitrot your patch, now that bug 446689 is checked in ?

Comment 34

[Brian Crowder]

Attachment: unbitrotted

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?

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 363142 [details] [diff] [review]
unbitrotted

r=mrbkap after someone verifies that this doesn't regress unit tests.

Comment 36

[Serge Gautherie (:sgautherie)]

[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

[Serge Gautherie (:sgautherie)]

(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

[Serge Gautherie (:sgautherie)]

(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

[Serge Gautherie (:sgautherie)]

(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

[Serge Gautherie (:sgautherie)]

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&regexp=on&case=on&find=%2Fjs%2Fsrc%2F

Comment 41

[Brian Crowder]

Attachment: simpler

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.

Comment 42

[Brian Crowder]

Attachment: betterer [Checkin: Comment 46 & 48]

With the if/else cleanup I wrote last time, and we don't need to check if (file) anymore, thanks sgautherie.

Comment 43

[Serge Gautherie (:sgautherie)]

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 ?

Comment 44

[Brian Crowder]

Thanks for testing, sgautherie, sorry for not doing this right months ago.

Comment 45

[Brian Crowder]

If someone gets a moment to land the "betterer" patch, that would be helpful.  Thanks.

Comment 46

[Serge Gautherie (:sgautherie)]

Comment on attachment 363228 [details] [diff] [review]
betterer
[Checkin: Comment 46 & 48]


http://hg.mozilla.org/mozilla-central/rev/8c5d11efd452

Comment 47

[Serge Gautherie (:sgautherie)]

"Reminder":

(In reply to comment #43)
> Maybe you could add a part of comment 41 as a comment in the file too ?

Comment 48

[Serge Gautherie (:sgautherie)]

Comment on attachment 363228 [details] [diff] [review]
betterer
[Checkin: Comment 46 & 48]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e32789c85946

Comment 49

[Serge Gautherie (:sgautherie)]

V.Fixed, as it detected bug 482112 ;->

Comment 50

[(not currently active) Ted Mielczarek]

Attachment: test [Checkin: Comment 51]

Here's an xpcshell test. I'll just check this in whenever the tree is green enough.

Comment 51

[(not currently active) Ted Mielczarek]

Pushed the xpcshell test:
http://hg.mozilla.org/mozilla-central/rev/9cac836012c9

Comment 52

[Serge Gautherie (:sgautherie)]

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.