Closed
Bug 463122
Opened 16 years ago
Closed 12 years ago
Components.utils.import throws NS_ERROR_FILE_NOT_FOUND for components that fail to import other components
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: myk, Assigned: jdm)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 3 obsolete files)
2.62 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
1.42 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
I would contribute gps' offered bounty.
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #580571 -
Flags: review?(benjamin)
Assignee | ||
Updated•12 years ago
|
Attachment #580571 -
Flags: review?(mrbkap)
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
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?
Assignee | ||
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
(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?
Assignee | ||
Comment 16•12 years ago
|
||
Nevermind, I can make the xpcshell harness error reporting better.
Assignee | ||
Comment 17•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #580571 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #582000 -
Attachment is obsolete: true
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #582041 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Attachment #582005 -
Attachment is obsolete: true
Comment 20•12 years ago
|
||
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
Updated•12 years ago
|
Attachment #582040 -
Flags: review?(mrbkap) → review+
Comment 21•12 years ago
|
||
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•12 years ago
|
||
(In reply to Mozilla RelEng Bot from comment #21) sorry for the double bot post - slight differences in string caused this.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → josh
Comment 23•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Comment 26•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec375c688831 http://hg.mozilla.org/integration/mozilla-inbound/rev/c99ee2ec1016
Comment 27•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ec375c688831 https://hg.mozilla.org/mozilla-central/rev/c99ee2ec1016
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•