Closed Bug 439110 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set

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)

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).
Who's a good reviewer for this patch?
Attachment #325005 - Flags: review?
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)
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)
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)
For my edification, what's wrong with it?
(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.
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.
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?
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
Attachment #325018 - Flags: review? → review?(jst)
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 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.
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?
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?
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.
I lied before when I said (and then return JS_TRUE), sorry.  :)
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
(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
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.
Attachment #325018 - Attachment is obsolete: true
Attachment #325018 - Flags: review?(jst)
Attached patch Throw exception too (obsolete) — Splinter Review
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)
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.
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)
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.
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: nobody → crowder
Comment on attachment 325092 [details] [diff] [review]
Make xpcshell's load() throw exception on error v2

New patch shortly.
Attachment #325092 - Flags: review?(brendan)
Attached patch identical to js.cpp:Load() (obsolete) — Splinter Review
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)
Bugzilla barfed on me while I was submitting the patch, so I have no idea if anyone got emailed.
Attachment #325450 - Flags: review?(brendan) → review+
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)
Attachment #325450 - Flags: superreview?(jst) → superreview+
Landed: 15424:f201baf7bf04
Status: NEW → RESOLVED
Closed: 11 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
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 → ---
Status: REOPENED → NEW
Flags: wanted1.9.1?
Component: Testing → TUnit
Flags: wanted1.9.1?
Product: Core → Testing
QA Contact: testing → tunit
Version: Trunk → unspecified
Component: TUnit → XPConnect
Product: Testing → Core
QA Contact: tunit → xpconnect
Flags: wanted1.9.1?
Version: unspecified → Trunk
(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 :-<
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.
Depends on: 446689
Could you unbitrot your patch, now that bug 446689 is checked in ?
Blocks: 384339
Attached patch unbitrotted (obsolete) — Splinter Review
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 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+
[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.
(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.
(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.
(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...
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
Attached patch simpler (obsolete) — Splinter Review
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)
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)
Attachment #363228 - Flags: review?(mrbkap)
Attachment #363228 - Flags: review?(mrbkap) → review+
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 ?
Thanks for testing, sgautherie, sorry for not doing this right months ago.
Whiteboard: [needs landing !?]
If someone gets a moment to land the "betterer" patch, that would be helpful.  Thanks.
Keywords: checkin-needed
Whiteboard: [needs landing !?]
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]
"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: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Depends on: 482112
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]
Flags: wanted1.9.1?
Whiteboard: [needs 1.9.1 landing] → [fixed1.9.1b4]
V.Fixed, as it detected bug 482112 ;->
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Here's an xpcshell test. I'll just check this in whenever the tree is green enough.
Pushed the xpcshell test:
http://hg.mozilla.org/mozilla-central/rev/9cac836012c9
Flags: in-testsuite? → in-testsuite+
Attachment #372846 - Attachment description: test → test [Checkin: Comment 51]
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.