Last Comment Bug 463122 - Components.utils.import throws NS_ERROR_FILE_NOT_FOUND for components that fail to import other components
: Components.utils.import throws NS_ERROR_FILE_NOT_FOUND for components that fa...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla12
Assigned To: Josh Matthews [:jdm]
:
Mentors:
Depends on: 408412 731093
Blocks: 435025
  Show dependency treegraph
 
Reported: 2008-11-04 14:53 PST by Myk Melez [:myk] [@mykmelez]
Modified: 2012-02-27 18:35 PST (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add new error code to return a more sensible guess as to why a JSM import failed. (2.16 KB, patch)
2011-12-09 15:10 PST, Josh Matthews [:jdm]
benjamin: review-
Details | Diff | Review
Propagate mozJSComponentLoader exceptions that occur while executing the script. (2.43 KB, patch)
2011-12-15 09:02 PST, Josh Matthews [:jdm]
no flags Details | Diff | Review
Improve xpcshell error reporting for generic exceptions caught at the top level. (2.18 KB, patch)
2011-12-15 09:17 PST, Josh Matthews [:jdm]
no flags Details | Diff | Review
Propagate mozJSComponentLoader exceptions that occur while executing the script. (2.62 KB, patch)
2011-12-15 11:10 PST, Josh Matthews [:jdm]
mrbkap: review+
Details | Diff | Review
Improve xpcshell error reporting for generic exceptions caught at the top level. (1.42 KB, patch)
2011-12-15 11:10 PST, Josh Matthews [:jdm]
ted: review+
Details | Diff | Review

Description Myk Melez [:myk] [@mykmelez] 2008-11-04 14:53:53 PST
When a JS component imports another component that contains a syntax error or does not exist, Components.utils.import throws NS_ERROR_FILE_NOT_FOUND for the first component, even though that component does exist.  It should throw an error that reflects the actual problem.

For example, I created two components in an extension, foo.js and bar.js.  foo.js imports bar.js, and bar.js contains a syntax error:

foo.js:
---
Components.utils.import("resource://snowl/modules/bar.js");
---

bar.js:
---
foo bar;
---

Then I made browser chrome import foo.js and started Firefox, whereupon it reported the following errors to the console:

---
Error: missing ; before statement
Source file: file:///mnt/hgfs/myk/Projects/snowl/modules/bar.js
Line: 1
Source code:
foo bar; 
 ----------
Error: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
Source file: chrome://snowl/content/browser.js
Line: 38
---

Afterwards, I deleted bar.js and restarted Firefox, whereupon it reported the following errors to the console:

---
Error: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
Source file: file:///mnt/hgfs/myk/Projects/snowl/modules/foo.js
Line: 1
 ----------
Error: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
Source file: chrome://snowl/content/browser.js
Line: 38
---

In both of these cases, the initial error (the syntax error in the first case, and the FILE_NOT_FOUND error on bar.js in the second case) is correct, but the second reported error (the FILE_NOT_FOUND error on foo.js in both cases) is not correct, as foo.js does exist.

Note: this is related to bug 408412, although I think it's a different problem.
Comment 1 Jonathan Protzenko [:protz] 2011-02-09 14:05:33 PST
That also happens for files that contain runtime errors, such as access to undefined properties in strict mode. Although the file is syntactically correct, a run-time error results in a NS_ERROR_FILE_NOT_FOUND.
Comment 2 Glenn Washburn 2011-10-03 00:31:08 PDT
This caused me a lot of headaches because I didn't realize there was a runtime error issue, and thought the problem was in my manifest or uri being imported.

This bug should be able to be fixed by changing the return value returned at js/src/xpconnect/loader/mozJSComponentLoader.cpp:1381 in function mozJSComponentLoader::ImportInto.  The comment above that return statement currently says "Something failed, but we don't know what it is, guess."

I suggest one of two solutions:

1. Return the error NS_ERROR_FILE_EXECUTION_FAILED.  I think technically this error is meant to be used when failing to execute an executable file, but I think it conveys the right idea much better than NS_ERROR_FILE_NOT_FOUND.

2. Create a new error code, maybe NS_ERROR_FILE_COMPILATION_FAILED, and return that.  I think this route is preferable, but it might be harder to get a change that adds a new error code.

This bug has a relatively easy solution and has been open for quite some time.  Please let's get this fixed.  Thanks.
Comment 3 Josh Matthews [:jdm] 2011-10-03 21:24:10 PDT
I dug into a simple xpcshell testcase in which a JSM is imported that imports another JSM containing a syntax error. The exception is correctly set in the mCallContext in the CallMethodHelper::Call that invokes the second nsXPCComponents_Utils::Import, but that doesn't seem to propagate back through the JS interpreter.
Comment 4 Josh Matthews [:jdm] 2011-10-03 21:38:19 PDT
I've confirmed that at mozJSComponentLoader.cpp:1021, when the JS_ExecuteScriptVersion call fails (in the first, correctly-formed jsm), there is no exception pending on the context. It's lodged somewhere inside the XPConnect machinery that called the second import.
Comment 5 Josh Matthews [:jdm] 2011-10-04 21:11:05 PDT
I have figured out where the exception is cleared, and it makes sense. I now understand that we don't necessarily want the inner exception to propagate out all the way, and the current setup is pretty correct. But yes, Glenn is correct that the FILE_NOT_FOUND error that is the default is really not a great choice.
Comment 6 Gregory Szorc [:gps] 2011-12-09 14:07:16 PST
I will buy quality beer, scotch, bourbon, or similar bounty for whoever makes the error message actionable: I've lost too much time over the lack of a helpful error message and want to see this addressed.
Comment 7 Allison Naaktgeboren :ally 2011-12-09 14:17:18 PST
I would contribute gps' offered bounty.
Comment 8 Josh Matthews [:jdm] 2011-12-09 15:10:25 PST
Created attachment 580571 [details] [diff] [review]
Add new error code to return a more sensible guess as to why a JSM import failed.
Comment 9 Benjamin Smedberg [:bsmedberg] 2011-12-12 13:03:37 PST
Comment on attachment 580571 [details] [diff] [review]
Add new error code to return a more sensible guess as to why a JSM import failed.

You should not be declaring a new xpconnect message code here. They are all declared in nsIXPConnect.idl and you're duplicating NS_ERROR_XPC_NOT_ENOUGH_ARGS. You should probably also add this to xpc.msg.
Comment 10 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-12-15 07:04:20 PST
Comment on attachment 580571 [details] [diff] [review]
Add new error code to return a more sensible guess as to why a JSM import failed.

Does anybody know why the exception from the nested compilation doesn't end up getting successfully propagated out here?
Comment 11 Josh Matthews [:jdm] 2011-12-15 07:19:21 PST
Gregory, it looks like the loader should be throwing a very actionable error in the JS error console (note: the global one, not the webpage one). Is this true?
Comment 12 Josh Matthews [:jdm] 2011-12-15 09:01:52 PST
I did some more digging, and I can answer comment 10. The exception doesn't propagate out because it occurs inside the JS_ExecuteScript call in GlobalForLocation, and at that point we don't have DONT_REPORT_UNCAUGHT set, so the exception is handled by the mozJSLoaderErrorReporter (see comment 11). With the patch I will attach, I can make the exception bubble out to the outermost level, ie. the xpcshell test shows this output:

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | SyntaxError: missing ; before statement

This is significantly less actionable in my opinion, as it's lacking the relevant file information.
Comment 13 Josh Matthews [:jdm] 2011-12-15 09:02:58 PST
Created attachment 582000 [details] [diff] [review]
Propagate mozJSComponentLoader exceptions that occur while executing the script.
Comment 14 Josh Matthews [:jdm] 2011-12-15 09:07:13 PST
For reference, this is what I see for the xpcshell test without my changes:

JS Component Loader: ERROR resource://test/syntax_error.jsm:1
                     SyntaxError: missing ; before statement

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: /Users/jdm/src/mozilla-central/obj-x86_64-apple-darwin10.8.0/_tests/xpcshell/js/xpconnect/tests/unit/test_import_fail.js :: run_test :: line 3"  data: no]
Comment 15 Florian Quèze [:florian] [:flo] 2011-12-15 09:09:29 PST
(In reply to Josh Matthews [:jdm] from comment #12)
> I did some more digging, and I can answer comment 10. The exception doesn't
> propagate out because it occurs inside the JS_ExecuteScript call in
> GlobalForLocation

Is this code path also used when loading XPCOM components (rather than JS modules)? Could the root cause be the same for bug 700421?
Comment 16 Josh Matthews [:jdm] 2011-12-15 09:17:39 PST
Created attachment 582005 [details] [diff] [review]
Improve xpcshell error reporting for generic exceptions caught at the top level.

Nevermind, I can make the xpcshell harness error reporting better.
Comment 17 Josh Matthews [:jdm] 2011-12-15 09:18:36 PST
With the xpcshell harness patch and my mozJSComponentLoader patch, the output now reads:

TEST-UNEXPECTED-FAIL | (resource://test/syntax_error.jsm:1) | SyntaxError: missing ; before statement
Comment 18 Josh Matthews [:jdm] 2011-12-15 11:10:15 PST
Created attachment 582040 [details] [diff] [review]
Propagate mozJSComponentLoader exceptions that occur while executing the script.

Tested by adding a failing import to NetUtil.jsm. The error messages in the console were significantly more reasonable, and nary a trace of FILE_NOT_FOUND was seen.
Comment 19 Josh Matthews [:jdm] 2011-12-15 11:10:58 PST
Created attachment 582041 [details] [diff] [review]
Improve xpcshell error reporting for generic exceptions caught at the top level.
Comment 20 Mozilla RelEng Bot 2011-12-15 15:20:31 PST
Try run for d203eee17989 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d203eee17989
Results (out of 76 total builds):
    success: 55
    warnings: 19
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-d203eee17989
Comment 21 Mozilla RelEng Bot 2011-12-16 15:47:07 PST
Try run for d203eee17989 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d203eee17989
Results (out of 76 total builds):
    success: 55
    warnings: 19
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-d203eee17989
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2011-12-16 15:55:58 PST
(In reply to Mozilla RelEng Bot from comment #21)
 sorry for the double bot post - slight differences in string caused this.
Comment 23 Mozilla RelEng Bot 2011-12-22 07:10:31 PST
Try run for c72d1a2ba108 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c72d1a2ba108
Results (out of 55 total builds):
    success: 42
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-c72d1a2ba108
Comment 24 Mozilla RelEng Bot 2011-12-22 16:40:32 PST
Try run for 021e6f733a85 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=021e6f733a85
Results (out of 55 total builds):
    success: 48
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-021e6f733a85
Comment 25 Ted Mielczarek [:ted.mielczarek] 2011-12-28 07:53:16 PST
Comment on attachment 582041 [details] [diff] [review]
Improve xpcshell error reporting for generic exceptions caught at the top level.

Review of attachment 582041 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/head.js
@@ +341,5 @@
> +          msg += ":" + e.lineNumber;
> +        }
> +        msg += ")";
> +      } else {
> +        msg += "(xpcshell/head.js)";

I'd just get rid of the parentheses. I'm not sure where they came from anyway.

Note You need to log in before you can comment on or make changes to this bug.