Closed Bug 446689 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: richard.vestal, Assigned: Seno.Aiko)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b4])

Attachments

(1 file, 1 obsolete file)

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
Line 234 of xpcshell.cpp.
The patch in bug 439110 would fix the load() problem but not two other missing fclose() calls (the -f option and "xpcshell.js").
Status: UNCONFIRMED → NEW
Component: Core → XPConnect
Ever confirmed: true
OS: Windows Vista → All
Product: Rhino → Core
QA Contact: core → xpconnect
Hardware: x86 → All
Summary: xpcshell's Load() function doesn't close file handle → xpcshell's load() function doesn't close file handle
Version: other → Trunk
Attachment #361993 - Flags: review?(mrbkap)
Attachment #361993 - Flags: review?(mrbkap)
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!
Attached patch fail early [See comment 6] (obsolete) — Splinter Review
Assignee: nobody → Seno.Aiko
Attachment #361993 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #362248 - Flags: review?(mrbkap)
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.
Attachment #362248 - Flags: review?(mrbkap)
Comment on attachment 361993 [details] [diff] [review]
add missing fclose calls
[Checkin: Comment 8 & 9]

This is the way to go for now.
Attachment #361993 - Attachment is obsolete: false
Attachment #361993 - Flags: superreview+
Attachment #361993 - Flags: review+
Keywords: checkin-needed
Comment on attachment 361993 [details] [diff] [review]
add missing fclose calls
[Checkin: Comment 8 & 9]


http://hg.mozilla.org/mozilla-central/rev/0a341c0bfc3f
Attachment #361993 - Attachment description: add missing fclose calls → add missing fclose calls [Checkin: Comment 8]
Blocks: 439110
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #362248 - Attachment description: fail early → fail early [See comment 6]
Attachment #362248 - Attachment is obsolete: true
Flags: wanted1.9.1?
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
Attachment #361993 - Attachment description: add missing fclose calls [Checkin: Comment 8] → add missing fclose calls [Checkin: Comment 8 & 9]
Flags: wanted1.9.1?
Keywords: fixed1.9.1
Whiteboard: [fixed1.9.1b4]
You need to log in before you can comment on or make changes to this bug.