394789 - Add a finalize() method to mozStorageStatement

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[[e-mail]]

+++ This bug was initially created as a clone of Bug #394241 +++

>Given all the warnings about how the database can be corrupted if you open it
>from multiple threads, it's a little scary that there's no way to explicitly
>close the database.  Since you're at the mercy of the semi-random nature of
>the garbage collector to get the database closed, this could be dangerous.

Bug #394241 added a close() method to mozIStorageConnection. However, this isn't very useful as there's no way to close a mozIStorageStatement except via the undocumented .initialize(connection, null)

Comment 1

[Shawn Wilsher :sdwilsh]

ah, so is it that you have a statement open still?  You should always be calling .reset() on a statement once you are done with it.

Comment 2

[[e-mail]]

(In reply to comment #1)
>ah, so is it that you have a statement open still?  You should always be
>calling .reset() on a statement once you are done with it.
Sure, but that doesn't actually close it. Should it?

Comment 3

[Shawn Wilsher :sdwilsh]

er, no.  I see the issue.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 5

[[e-mail]]

Comment on attachment 280186 [details] [diff] [review]
v1.0

>+    (void)Finalize();
Why the (void)? (Not really fussed, but we only tend to use this when we know that the method may return an error.)

>+        mDBStatement = NULL;
nsnull? (Not really fussed but that's what's used everywhere else e.g. in the constructor or Initialize().)

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Why the (void)? (Not really fussed, but we only tend to use this when we know
> that the method may return an error.)
Err, mostly because I should be checking sqlite's return result...shame on me!

> >+        mDBStatement = NULL;
> nsnull? (Not really fussed but that's what's used everywhere else e.g. in the
> constructor or Initialize().)
So, this code isn't horrible consistent here, but what I have been trying to unify it to is this:
If it's a sqlite data structure or is going to be passed to sqlite, we use NULL.  Otherwise we use nsnull.  I'm pretty sure it doesn't matter, but...

New patch with the first comment fixed.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 280201 [details] [diff] [review]
v1.1

Neil pointed out something I should be testing, revising...

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Neil, could you please run the storage tests.  I'm just worried about windows still not liking to remove the file since because of issues I've had in the past.

Comment 10

[[e-mail]]

Comment on attachment 280250 [details] [diff] [review]
v1.2

I don't test tests, but I can verify that I can use .finalize() instead of .initialize(connection, null) and I am still able to delete the file on Windows.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 280250 [details] [diff] [review]
v1.2

Seth, can you look over the tests and run them on windows please?

Comment 12

[(not reading, please use [e-mail] instead)]

shawn, on windows, we don't pass all the tests:

>>>>>>>
*** Registering components in: nsPlacesModule
*** Registering components in: mozStorageModule
*** Storage Tests: Trying to close!
*** Storage Tests: Trying to remove file!
*** test pending
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VAL
UE) [mozIStorageService.openSpecialDatabase]"  nsresult: "0x80070057 (NS_ERROR_I
LLEGAL_VALUE)"  location: "JS frame :: ../../_tests/xpcshell-simple/test_storage
/unit/test_storage_service.js :: test_openSpecialDatabase_invalid_arg :: line 43
"  data: no]
e.result is 2147942487
*** Storage Tests: Trying to close!
*** Storage Tests: Trying to remove file!
[Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS
_DENIED) [nsILocalFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DEN
IED)"  location: "JS frame :: ../../_tests/xpcshell-simple/test_storage/unit/hea
d_storage.js :: cleanup :: line 66"  data: no]
*** FAIL ***
*** registering nsTaggingService.js: [ Places Tagging Service ]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file c:/
builds/trunk/mozilla/xpcom/base/nsExceptionService.cpp, line 191
nsStringStats
 => mAllocCount:           2241
 => mReallocCount:            1
 => mFreeCount:            2241
 => mShareCount:           2456
 => mAdoptCount:            166
 => mAdoptFreeCount:        166

<<<<<<<
../../_tests/xpcshell-simple/test_storage/unit/test_storage_statement.js: PASS
../../_tests/xpcshell-simple/test_storage/unit/test_storage_statement_wrapper.js
: PASS
../../_tests/xpcshell-simple/test_storage/unit/test_storage_value_array.js: PASS

../../_tests/xpcshell-simple/test_storage/unit/test_unicode.js: PASS
make[1]: *** [check] Error 1
make[1]: Leaving directory `/cygdrive/c/builds/trunk/mozilla/ff-debug/storage/te
st'
make: *** [check] Error 2

Comment 13

[Shawn Wilsher :sdwilsh]

hmm, can you make sure there is no .sqlite files in obj/dist/bin please?
I don't think that is the same error we used to get.

Comment 14

[(not reading, please use [e-mail] instead)]

> can you make sure there is no .sqlite files in obj/dist/bin please?

there was a places.sqlite file in by dist/bin.  But I've removed it and re-ran make check, and I still fail:

*** test pending
[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VAL
UE) [mozIStorageService.openSpecialDatabase]"  nsresult: "0x80070057 (NS_ERROR_I
LLEGAL_VALUE)"  location: "JS frame :: ../../_tests/xpcshell-simple/test_storage
/unit/test_storage_service.js :: test_openSpecialDatabase_invalid_arg :: line 43
"  data: no]
e.result is 2147942487
*** Storage Tests: Trying to close!
*** Storage Tests: Trying to remove file!
[Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS
_DENIED) [nsILocalFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DEN
IED)"  location: "JS frame :: ../../_tests/xpcshell-simple/test_storage/unit/hea
d_storage.js :: cleanup :: line 66"  data: no]
*** FAIL ***
*** registering nsTaggingService.js: [ Places Tagging Service ]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file c:/
builds/trunk/mozilla/xpcom/base/nsExceptionService.cpp, line 191
nsStringStats
 => mAllocCount:           2241
 => mReallocCount:            1
 => mFreeCount:            2241
 => mShareCount:           2456
 => mAdoptCount:            166
 => mAdoptFreeCount:        166

<<<<<<<
../../_tests/xpcshell-simple/test_storage/unit/test_storage_statement.js: PASS
../../_tests/xpcshell-simple/test_storage/unit/test_storage_statement_wrapper.js
: PASS
../../_tests/xpcshell-simple/test_storage/unit/test_storage_value_array.js: PASS

../../_tests/xpcshell-simple/test_storage/unit/test_unicode.js: PASS
make[1]: *** [check] Error 1
make[1]: Leaving directory `/cygdrive/c/builds/trunk/mozilla/ff-debug/storage/te
st'
make: *** [check] Error 2

I'll debug and see what's going on.

Comment 15

[(not reading, please use [e-mail] instead)]

sorry for the delay.

did a little debugging, and we're failing when we call remove().

Specifically, we fail deep in nsLocalFile::Remove(), our call to _wremove() failes with errno set to 13 (EACCES, which we convert to NS_ERROR_FILE_ACCESS_DENIED)

here's my theory: we can't remove test_storage.sqlite because we're still writing out to disk.

I broke in the debugger on the Remove() failure, and check out these two threads:

thread #1 where we are are writing out asynchronously to disk.

 	ntdll.dll!7c90eb94() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
 	ntdll.dll!7c90e9c0() 	
 	kernel32.dll!7c8025cb() 	
 	kernel32.dll!7c802532() 	
 	nspr4.dll!_PR_MD_WAIT_CV(_MDCVar * cv=0x00d1820c, _MDLock * lock=0x00d18104, unsigned int timeout=4294967295)  Line 280 + 0x14 bytes	C
 	nspr4.dll!_PR_WaitCondVar(PRThread * thread=0x00d14450, PRCondVar * cvar=0x00d18198, PRLock * lock=0x00d180e8, unsigned int timeout=4294967295)  Line 204 + 0x17 bytes	C
 	nspr4.dll!PR_WaitCondVar(PRCondVar * cvar=0x00d18198, unsigned int timeout=4294967295)  Line 551 + 0x17 bytes	C
>	strgcmps.dll!ProcessAsyncMessages()  Line 1528 + 0xf bytes	C++
 	strgcmps.dll!AsyncWriteThread::Run()  Line 475	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x01cbff1c)  Line 491	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00d18288, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	xpcom_core.dll!nsThread::ThreadFunc(void * arg=0x00d18288)  Line 254 + 0xb bytes	C++
 	nspr4.dll!_PR_NativeRunThread(void * arg=0x00d14450)  Line 436 + 0xf bytes	C
 	nspr4.dll!pr_root(void * arg=0x00d14450)  Line 122 + 0xf bytes	C
 	msvcr80d.dll!_callthreadstartex()  Line 348 + 0xf bytes	C
 	msvcr80d.dll!_threadstartex(void * ptd=0x00d14d60)  Line 331	C
 	kernel32.dll!7c80b683() 	


thread #2, where the Remove() call fails:

 	xpcom_core.dll!nsLocalFile::Remove(int recursive=0)  Line 1817 + 0x15 bytes	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000011, unsigned int methodIndex=1, unsigned int paramCount=1238132, nsXPTCVariant * params=0x0000000c)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=17)  Line 2326 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2326 + 0x1e bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x00d28800, JSObject * obj=0x00d3d940, unsigned int argc=1, long * argv=0x00d19d9c, long * vp=0x0012e714)  Line 1467 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x00d28800, unsigned int argc=1, unsigned int flags=0)  Line 1378 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x00d28800, unsigned char * pc=0x00d373c2, long * result=0x0012ed34)  Line 4111 + 0xf bytes	C
 	js3250.dll!js_Execute(JSContext * cx=0x00d28800, JSObject * chain=0x00d13120, JSScript * script=0x00d39800, JSStackFrame * down=0x00000000, unsigned int flags=0, long * result=0x0012fde0)  Line 1645 + 0x13 bytes	C
 	js3250.dll!JS_ExecuteScript(JSContext * cx=0x00d28800, JSObject * obj=0x00d13120, JSScript * script=0x00d39800, long * rval=0x0012fde0)  Line 4777 + 0x19 bytes	C
 	xpcshell.exe!ProcessFile(JSContext * cx=0x00d28800, JSObject * obj=0x00d13120, const char * filename=0x00b68621, _iobuf * file=0x10310c50, int forceTTY=0)  Line 660 + 0x16 bytes	C++
 	xpcshell.exe!Process(JSContext * cx=0x00d28800, JSObject * obj=0x00d13120, const char * filename=0x00b68621, int forceTTY=0)  Line 739 + 0x19 bytes	C++
 	xpcshell.exe!ProcessArgs(JSContext * cx=0x00d28800, JSObject * obj=0x00d13120, char * * argv=0x00b6849c, int argc=11)  Line 858 + 0x19 bytes	C++
 	xpcshell.exe!main(int argc=11, char * * argv=0x00b6849c, char * * envp=0x00b63810)  Line 1432 + 0x15 bytes	C++
 	xpcshell.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	xpcshell.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816fd7() 	
 	nspr4.dll!pr_LoadLibraryByPathname(const char * name=0x0031002e, int flags=3276846)  Line 862 + 0x9 bytes	C

Comment 16

[Shawn Wilsher :sdwilsh]

OK, so I still cannot remove the dang try catch because of windows :(

One of these days I'll get rid of it!

Comment 17

[[e-mail]]

Is there some flag you can set to force sync writes for test purposes?

Comment 18

[Shawn Wilsher :sdwilsh]

There is a PRAGMA for it, but I'm still worried that it might not be fast enough at times :/

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 280250 [details] [diff] [review]
v1.2

clearing review request.

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Last try before I put the remove in a try catch block again.

Comment 21

[(not reading, please use [e-mail] instead)]

Comment on attachment 280733 [details] [diff] [review]
v1.3

shawn:  sorry man, but this this still doesn't pass tests on win32.

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

Back to wrapping it in a try-catch block :(

Maybe if we ever get rid of the helper thread for disk IO, we can actually do this correctly.

Comment 23

[(not reading, please use [e-mail] instead)]

Comment on attachment 281093 [details] [diff] [review]
v1.4

r=sspitzer, this version passes "make check" on windows.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 281093 [details] [diff] [review]
v1.4

This patch is only so big because of the test changes.  They are not actually necessary either since we still cannot remove the file cleanly on windows.

The actual code changes haven't been modified since v1.1

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281093 [details] [diff] [review]
v1.4

a=bzbarsky

Comment 26

[Shawn Wilsher :sdwilsh]

Checking in storage/public/mozIStorageStatement.idl;
new revision: 1.11; previous revision: 1.10
Checking in storage/src/mozStorageStatement.cpp;
new revision: 1.27; previous revision: 1.26
Checking in storage/test/unit/head_storage.js;
new revision: 1.5; previous revision: 1.4
Checking in storage/test/unit/test_bug-365166.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_bug-393952.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_like.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_like_escape.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_storage_aggregates.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_storage_function.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_storage_progresshandler.js;
new revision: 1.4; previous revision: 1.3
Checking in storage/test/unit/test_storage_statement.js;
new revision: 1.5; previous revision: 1.4
Checking in storage/test/unit/test_storage_statement_wrapper.js;
new revision: 1.3; previous revision: 1.2
Checking in storage/test/unit/test_storage_value_array.js;
new revision: 1.2; previous revision: 1.1
Checking in storage/test/unit/test_unicode.js;
new revision: 1.2; previous revision: 1.1

Comment 28

[Eric Shepherd [:sheppy]]

Removing doc needed keyword in favor of a new bug that covers the fact that in general we actually need proper documentation for Storage; that bug (bug 399535) is set as blocked by this one.

Comment 29

[Eric Shepherd [:sheppy]]

Removing doc needed keyword in favor of a new bug that covers the fact that in general we actually need proper documentation for Storage; that bug (bug 399535) is set as blocked by this one.