Closed Bug 463122 Opened 11 years ago Closed 8 years ago

Components.utils.import throws NS_ERROR_FILE_NOT_FOUND for components that fail to import other components

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: myk, Assigned: jdm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
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.
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.
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.
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.
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.
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.
I would contribute gps' offered bounty.
Attachment #580571 - Flags: review?(mrbkap)
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.
Attachment #580571 - Flags: review?(benjamin) → review-
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?
Attachment #580571 - Flags: review?(mrbkap)
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?
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.
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]
(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?
Nevermind, I can make the xpcshell harness error reporting better.
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
Attachment #580571 - Attachment is obsolete: true
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.
Attachment #582040 - Flags: review?(mrbkap)
Attachment #582000 - Attachment is obsolete: true
Attachment #582005 - Attachment is obsolete: true
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
Attachment #582040 - Flags: review?(mrbkap) → review+
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
(In reply to Mozilla RelEng Bot from comment #21)
 sorry for the double bot post - slight differences in string caused this.
Assignee: nobody → josh
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
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 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.
Attachment #582041 - Flags: review?(ted.mielczarek) → review+
https://hg.mozilla.org/mozilla-central/rev/ec375c688831
https://hg.mozilla.org/mozilla-central/rev/c99ee2ec1016
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 731093
You need to log in before you can comment on or make changes to this bug.