408412 - Components.utils.import throws NS_ERROR_FILE_NOT_FOUND when there's a syntax error

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Dan Mills [:thunder]]

I'm getting this:

JS Component Loader: ERROR (null):0
                     uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: file:///Users/thunder/sources/hg.sandmill.org/weave/modules/weave.js :: <TOP_LEVEL> :: line 44"  data: no]

When one of the files I'm importing has a syntax error.  Fixing the error (which I track down by commenting out large chunks of code and uncommenting bit by bit) makes it all work again.

I just found out that it's because I import files which import other files.  Errors in the first one get reported OK, but errors in deep files don't.

Comment 1

[Robert Kaiser]

I just ran into this and it makes debugging quite hard :(

Comment 2

[Dan Mills [:thunder]]

In my case, I work around the problem by loading all of modules from one file in chrome js, in reverse order to how would normally be imported (i.e., bottom-up).  That way errors get reported correctly.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I suppose I avoid this problem by using a syntax checking editor.

Comment 4

[Justin Dolske [:Dolske]]

Gah, this just hit me to. This is hell to debug. :(

Comment 5

[John J. Barton]

This report needs a testcase.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

If someone gives me a concrete way to reproduce this (even if it's something like "add a syntax error in file 'x'") then I'll fix it.

Comment 7

[Dave Townsend [:mossop]]

Attachment: unit test

Here is a unit test that fails if the Component.utils.import of a js file with invalid syntax throws an exception. Might need to modify it, not sure exactly what your expecting to change this to.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix, v1

This includes Mossop's unit test. With this patch, we propagate exceptions from our context onto the outer context from C.u.import(), which propagates to the console when starting Firefox.

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 328493 [details] [diff] [review]
Proposed fix, v1

r=shaver.  Some pretty gross stylistic inconsistencis in there, all of them my fault!

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 328493 [details] [diff] [review]
Proposed fix, v1

>+        uint32 oldopts = 0;
>+        if (exception) {
>+            oldopts = JS_SetOptions(cx,
>+                            JS_GetOptions(cx) | JSOPTION_DONT_REPORT_UNCAUGHT);

Might be tidier to set oldopts = JS_GetOptions(cx); and then JS_SetOptions(cx, oldopts | JSOPTION_DONT...).

sr=me in any case.

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix pushed as http://hg.mozilla.org/index.cgi/mozilla-central/rev/90020c4ad446

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I had to back this out in http://hg.mozilla.org/index.cgi/mozilla-central/rev/d194c529cba0 because a unit test started failing.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

The functional difference here is that in the case where we don't find the file, we explicitly return NS_ERROR_FILE_NOT_FOUND (since on some systems we could return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST which breaks some unit tests).

I also noticed some early returns where we have to reset our context's options.

Comment 16

[Brendan Eich [:brendan]]

Benjamin, do you recall why we have NS_ERROR_FILE_TARGET_DOES_NOT_EXIST? It seems unnecessary and misnamed?

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 332229 [details] [diff] [review]
patch v2

>+        // If |exception| is non-null, then our caller wants to propagate any
>+        // exceptions out to our caller. Ensure that the engine doesn't

First sentence doesn't make sense -- either "our caller wants *us* to propagate ..." or "our caller wants to propagate any exceptions out to our grand-caller."

Thanks for the irc-pastebinned interdiff -- r+sr=me with above nit picked.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Pushed as http://hg.mozilla.org/mozilla-central/index.cgi/rev/5cd3b111caa8 and http://hg.mozilla.org/mozilla-central/index.cgi/rev/10c6fbeceb79 (since I missed brendan's review comment in the first commit).

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Latest patch contained a unit test => in-testsuite+

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Running test_bug408412.js:
*** test pending
*** test finished
*** exiting
*** PASS ***

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080807233207 Minefield/3.1a2pre ID:2008080620