446689 - xpcshell's load() function doesn't close file handle

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Rick Vestal]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 6.0; SLCC1; .NET CLR 2.0.50727; .NET CLR 3.0.04506; .NET CLR 3.5.21022)
Build Identifier: xulrunner cvs source from 20080624

Line 234 opens a file for processing but does not close it after the JS_CompileFileHandleForPrincipals call.

Reproducible: Always

Steps to Reproduce:
1. Call load function from within javascript
2. Immediately try to delete the file passed load
3. Observe lock issue on file

Comment 1

[Rick Vestal]

Line 234 of xpcshell.cpp.

Comment 2

[Aiko]

The patch in bug 439110 would fix the load() problem but not two other missing fclose() calls (the -f option and "xpcshell.js").

Comment 3

[Aiko]

Attachment: add missing fclose calls [Checkin: Comment 8 & 9]

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 361993 [details] [diff] [review]
add missing fclose calls
[Checkin: Comment 8 & 9]

>diff -r 688c44602a55 -r dc7117ff7d38 js/src/xpconnect/shell/xpcshell.cpp
>@@ -444,16 +444,18 @@ Load(JSContext *cx, JSObject *obj, uintN
>         file = fopen(filename, "r");
>         script = JS_CompileFileHandleForPrincipals(cx, obj, filename, file,
>                                                    gJSPrincipals);
>+        if (file)
>+            fclose(file);

It seems like it would be more consistent to check for failure and fail early there instead of this null check (like what JS_CompileFile does).

Unsetting the review request for now, but I'll stamp the next one, I'm sure!

Comment 5

[Aiko]

Attachment: fail early [See comment 6]

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 362248 [details] [diff] [review]
fail early
[See comment 6]

>diff -r 688c44602a55 js/src/xpconnect/shell/xpcshell.cpp
>+             * FIXME bug 439110: we should report an error when load() fails
>+             * but that seems to cause some unit test failures.

Eek. I didn't realize this part! Given this, it seems better to keep the current behavior and work on throwing an exception in that bug. Sorry for the extra work.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 361993 [details] [diff] [review]
add missing fclose calls
[Checkin: Comment 8 & 9]

This is the way to go for now.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 361993 [details] [diff] [review]
add missing fclose calls
[Checkin: Comment 8 & 9]


http://hg.mozilla.org/mozilla-central/rev/0a341c0bfc3f

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 361993 [details] [diff] [review]
add missing fclose calls
[Checkin: Comment 8 & 9]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/89df940af254