From: stephan Date: Thu, 3 Aug 2023 20:45:28 +0000 (+0000) Subject: Rework the fts5 db-close cleanup to avoid that fts5 internally calls a just-unbound... X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2c88b4aa2421a0cf17ca3e9d4289f8d2a65a56fd;p=thirdparty%2Fsqlite.git Rework the fts5 db-close cleanup to avoid that fts5 internally calls a just-unbound-from-wasm xDestroy() method. There is still work to do here to cover the case of fts5 functions being replaced (in which all xDestroy methods have to be retained until the db is closed). FossilOrigin-Name: 795f22421b96cea27b3883a580d8ce29cc50dfb7d4f852b3d36ede3766c7a24c --- diff --git a/ext/wasm/api/sqlite3-api-glue.js b/ext/wasm/api/sqlite3-api-glue.js index 7f05d37443..b68baabae9 100644 --- a/ext/wasm/api/sqlite3-api-glue.js +++ b/ext/wasm/api/sqlite3-api-glue.js @@ -1069,15 +1069,32 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ the being-destroyed (sqlite3*) object. */ __dbCleanupMap.extraCallbacks = []; + /** + Downstream code, namely sqlite3-fts5-helper.js, should add any + custom cleanup handlers to __dbCleanupMap.postCloseCallbacks. + Each function in this array will be called during + sqlite3_close_v2(), AFTER the db is closed, and passed a pointer + to the being-destroyed (sqlite3*) object. The memory is NOT A + VALID OBJECT but its address is still valid as a lookup key. + */ + __dbCleanupMap.postCloseCallbacks = []; {/* Binding of sqlite3_close_v2() */ const __sqlite3CloseV2 = wasm.xWrap("sqlite3_close_v2", "int", "sqlite3*"); + const __xArgDb = wasm.xWrap.argAdapter('sqlite3*'); capi.sqlite3_close_v2 = function(pDb){ if(1!==arguments.length) return __dbArgcMismatch(pDb, 'sqlite3_close_v2', 1); + pDb = __xArgDb(pDb); if(pDb){ try{__dbCleanupMap.cleanup(pDb)} catch(e){/*ignored*/} } - return __sqlite3CloseV2(pDb); + const rc = __sqlite3CloseV2(pDb); + if(pDb/*noting that it's not valid anymore*/){ + for(const f of __dbCleanupMap.postCloseCallbacks){ + try{f(pDb)}catch(e){/*ingored*/} + } + } + return rc; }; }/*sqlite3_close_v2()*/ diff --git a/ext/wasm/api/sqlite3-fts5-helper.js b/ext/wasm/api/sqlite3-fts5-helper.js index 061b7ca20f..09e3db8bed 100644 --- a/ext/wasm/api/sqlite3-fts5-helper.js +++ b/ext/wasm/api/sqlite3-fts5-helper.js @@ -16,14 +16,15 @@ 'use strict'; globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ const wasm = sqlite3.wasm, capi = sqlite3.capi, toss = sqlite3.util.toss3; - const __dbCleanupMap = sqlite3.__dbCleanupMap - || toss("Maintenance needed: can't reach sqlite3.__dbCleanupMap."); if(!capi.fts5_api_from_db){ return /*this build does not have FTS5*/; } const fts = sqlite3.fts5 = Object.create(null); + const xArgDb = wasm.xWrap.argAdapter('sqlite3*'); + /** - Move FTS-specific APIs from sqlite3.capi to sqlite3.fts. + Move FTS-specific APIs (installed via automation) from + sqlite3.capi to sqlite3.fts. */ for(const c of [ 'Fts5ExtensionApi', 'Fts5PhraseIter', 'fts5_api', @@ -33,8 +34,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ delete capi[c]; } - const cache = Object.create(null); - + /* JS-to-WASM arg adapter for xCreateFunction()'s xFunction arg. */ const xFunctionArgAdapter = new wasm.xWrap.FuncPtrAdapter({ name: 'fts5_api::xCreateFunction(xFunction)', signature: 'v(pppip)', @@ -56,6 +56,7 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ } }); + /* JS-to-WASM arg adapter for xCreateFunction()'s xDestroy arg. */ const xDestroyArgAdapter = new wasm.xWrap.FuncPtrAdapter({ name: 'fts5_api::xCreateFunction(xDestroy)', signature: 'v(p)', @@ -70,59 +71,62 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ } }); + + /** Map of (sqlite3*) to fts.fts5_api. */ + const __ftsApiToStruct = Object.create(null); + const __fts5_api_from_db = function(pDb, createIfNeeded){ + let rc = __ftsApiToStruct[pDb]; + if(!rc && createIfNeeded){ + rc = new fts.fts5_api(fts.fts5_api_from_db(pDb)); + __ftsApiToStruct[pDb] = rc; + } + return rc; + }; + /** Arrange for WASM functions dynamically created via this API to be uninstalled when the db they were installed for is closed... */ - const __fts5FuncsPerDb = Object.create(null); - const __cleanupForDb = function(pDb){ - return (__fts5FuncsPerDb[pDb] || (__fts5FuncsPerDb[pDb] = new Set)); - }; - __dbCleanupMap._addFts5Function = function ff(pDb, name){ - __cleanupForDb(pDb).add(name.toLowerCase()); + const __addCleanupForFunc = function(sfapi, name){ + if(!sfapi.$$funcNames){ + sfapi.$$funcNames = new Set; + } + sfapi.$$funcNames.add(name.toLowerCase()); }; - __dbCleanupMap.extraCallbacks.push(function(pDb){ - const list = __fts5FuncsPerDb[pDb]; - if(list){ - const fapi = fts.fts5_api_from_db(pDb) - const sfapi = new fts.fts5_api(fapi); - const scope = wasm.scopedAllocPush(); - //wasm.xWrap.FuncPtrAdapter.debugFuncInstall = true; - try{ - const xCF = wasm.functionEntry(sfapi.$xCreateFunction); - for(const name of list){ - //sqlite3.config.warn("Clearing FTS function",pDb,name); - try{ - const zName = wasm.scopedAllocCString(name); - /* In order to clean up properly we first have to invoke - the low-level (unwrapped) sfapi.$xCreateFunction and - then fake some calls into the Function-to-funcptr - apapters. If we just call cache.xcf(...) then the - adapter ends up uninstalling the xDestroy pointer which - sfapi.$xCreateFunction is about to invoke, leading to - an invalid memory access. */ - xCF(fapi, zName, 0, 0, 0); - // Now clean up our FuncPtrAdapters in a round-about way... - const argv = [fapi, zName, 0, 0, 0]; - xFunctionArgAdapter.convertArg(argv[3], argv, 3); - // xDestroyArgAdapter leads to an invalid memory access - // for as-yet-unknown reasons. For the time being we're - // leaking these. - //xDestroyArgAdapter.convertArg(argv[4], argv, 4); - }catch(e){ - sqlite3.config.error("Error removing FTS func",name,e); + + /** + Callback to be invoked via the JS binding of sqlite3_close_v2(), + after the db has been closed, meaing that the argument to this + function is not a valid object. We use its address only as a + lookup key. + */ + sqlite3.__dbCleanupMap.postCloseCallbacks.push(function(pDb){ + const sfapi = __fts5_api_from_db(pDb, false); + if(sfapi){ + delete __ftsApiToStruct[pDb]; + if(sfapi.$$funcNames){ + const fapi = sfapi.pointer; + const scope = wasm.scopedAllocPush(); + //wasm.xWrap.FuncPtrAdapter.debugFuncInstall = true; + try{ + for(const name of sfapi.$$funcNames){ + try{ + const zName = wasm.scopedAllocCString(name); + const argv = [fapi, zName, 0, 0, 0]; + xFunctionArgAdapter.convertArg(0, argv, 3); + xDestroyArgAdapter.convertArg(0, argv, 4); + }catch(e){ + sqlite3.config.error("Error removing FTS func",name,e); + } } + }finally{ + wasm.scopedAllocPop(scope); } - }finally{ - sfapi.dispose(); - delete __fts5FuncsPerDb[pDb]; - wasm.scopedAllocPop(scope); + //wasm.xWrap.FuncPtrAdapter.debugFuncInstall = false; } - //wasm.xWrap.FuncPtrAdapter.debugFuncInstall = false; + sfapi.dispose(); } }); - const xArgDb = wasm.xWrap.argAdapter('sqlite3*'); - /** Convenience wrapper to fts5_api::xCreateFunction. @@ -166,19 +170,19 @@ globalThis.sqlite3ApiBootstrap.initializers.push(function(sqlite3){ if( !name || 'string' !== typeof name ) toss("Invalid name argument."); const fapi = fts.fts5_api_from_db(db) || toss("Internal error - cannot get FTS5 API object for db."); - const sfapi = new fts.fts5_api(fapi); + const sfapi = __fts5_api_from_db(db, true); try{ - const xcf = cache.xcf || ( - cache.xcf = wasm.xWrap(sfapi.$xCreateFunction,'int',[ + const xcf = sfapi.$$xCreateFunction || ( + sfapi.$$xCreateFunction = wasm.xWrap(sfapi.$xCreateFunction, 'int', [ '*', 'string', '*', xFunctionArgAdapter, xDestroyArgAdapter ]) - )/* this cache entry is only legal as long as nobody replaces - the xCreateFunction() method */; + ); const rc = xcf(fapi, name, 0, xFunction || 0, xDestroy || 0 ); if(rc) toss(rc,"FTS5::xCreateFunction() failed."); - __dbCleanupMap._addFts5Function(db, name); - }finally{ + __addCleanupForFunc(sfapi, name); + }catch(e){ sfapi.dispose(); + throw e; } }; diff --git a/ext/wasm/tester1.c-pp.js b/ext/wasm/tester1.c-pp.js index 1e83d7e7c0..65e2be4a57 100644 --- a/ext/wasm/tester1.c-pp.js +++ b/ext/wasm/tester1.c-pp.js @@ -2921,7 +2921,7 @@ globalThis.sqlite3InitModule = sqlite3InitModule; fApi.dispose(); fApi = undefined; let destroyCalled = false; - fts.createFunction(db,{ + fts.createFunction(db, { name: 'mymatch', xFunction: function(pFtsX, pFtsCx, pCtx, argv){ //log("mymatch!", argv.length, argv); @@ -2939,8 +2939,6 @@ globalThis.sqlite3InitModule = sqlite3InitModule; }); let list = db.selectValues( "select mymatch(ft,a,b) from ft where b match 'b2'" - //^^^ results in a "no such cursor: 0" error - //"select a from ft where b match 'b2'" ); T.assert( 1 === list.length ) .assert( 'MY+a2:b2' === list[0] ); diff --git a/manifest b/manifest index 7db3bf87c7..8c61ea30a1 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Part\s2\sof\sthe\sfix\sfrom\s[a0f808363318c00fd1db78b]. -D 2023-08-03T20:02:49.776 +C Rework\sthe\sfts5\sdb-close\scleanup\sto\savoid\sthat\sfts5\sinternally\scalls\sa\sjust-unbound-from-wasm\sxDestroy()\smethod.\sThere\sis\sstill\swork\sto\sdo\shere\sto\scover\sthe\scase\sof\sfts5\sfunctions\sbeing\sreplaced\s(in\swhich\sall\sxDestroy\smethods\shave\sto\sbe\sretained\suntil\sthe\sdb\sis\sclosed). +D 2023-08-03T20:45:28.696 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F LICENSE.md df5091916dbb40e6e9686186587125e1b2ff51f022cc334e886c19a0e9982724 @@ -501,11 +501,11 @@ F ext/wasm/api/post-js-footer.js cd0a8ec768501d9bd45d325ab0442037fb0e33d1f3b4f08 F ext/wasm/api/post-js-header.js 47b6b281f39ad59fa6e8b658308cd98ea292c286a68407b35ff3ed9cfd281a62 F ext/wasm/api/pre-js.c-pp.js ad906703f7429590f2fbf5e6498513bf727a1a4f0ebfa057afb08161d7511219 F ext/wasm/api/sqlite3-api-cleanup.js 8489ac9933783f36807cfa5d79bfa366008c0443b42bf47bdef41bbfce4aa367 -F ext/wasm/api/sqlite3-api-glue.js c8fee0a75cf655bb830f01b6d0a53e3648a46be71d69256b2853c085004bf89e +F ext/wasm/api/sqlite3-api-glue.js 861ff5ccfbaa50579c46618be53ac374202ad73cc27eb5077967c35edbb1869d F ext/wasm/api/sqlite3-api-oo1.js 9678dc4d9a5d39632b6ffe6ea94a023119260815bf32f265bf5f6c36c9516db8 F ext/wasm/api/sqlite3-api-prologue.js 76258e160bf6a89cc75a7d3c05646a054c8cab7219cd1e10bc20cacaad022131 F ext/wasm/api/sqlite3-api-worker1.js 9f32af64df1a031071912eea7a201557fe39b1738645c0134562bb84e88e2fec -F ext/wasm/api/sqlite3-fts5-helper.js fd9a0cad447a56734b134eab57a832be8820e8cfb8a672c0b1b54c3805485c2e +F ext/wasm/api/sqlite3-fts5-helper.js 8d7a35e18463102fe2783657d5c59c3c0c7460f0eb699757ebfed2e44df2a001 F ext/wasm/api/sqlite3-license-version-header.js 0c807a421f0187e778dc1078f10d2994b915123c1223fe752b60afdcd1263f89 F ext/wasm/api/sqlite3-opfs-async-proxy.js 8cf8a897726f14071fae6be6648125162b256dfb4f96555b865dbb7a6b65e379 F ext/wasm/api/sqlite3-v-helper.js 8dc3da6e69d51f455b64cc88fec7977f528d79ec267cfcdd97b606c8cc4cd156 @@ -556,7 +556,7 @@ F ext/wasm/test-opfs-vfs.html 1f2d672f3f3fce810dfd48a8d56914aba22e45c6834e262555 F ext/wasm/test-opfs-vfs.js f09266873e1a34d9bdb6d3981ec8c9e382f31f215c9fd2f9016d2394b8ae9b7b F ext/wasm/tester1-worker.html ebc4b820a128963afce328ecf63ab200bd923309eb939f4110510ab449e9814c F ext/wasm/tester1.c-pp.html 1c1bc78b858af2019e663b1a31e76657b73dc24bede28ca92fbe917c3a972af2 -F ext/wasm/tester1.c-pp.js b1200f52a1b2ab51c0a2bc6be811090ee0a71e0d9661fcfe540073b80fb4d607 +F ext/wasm/tester1.c-pp.js 892d00acf7bf675e34766794dcc94bc26e5bf00abcfeeb9dbcc42eebac787bba F ext/wasm/tests/opfs/concurrency/index.html 0802373d57034d51835ff6041cda438c7a982deea6079efd98098d3e42fbcbc1 F ext/wasm/tests/opfs/concurrency/test.js a98016113eaf71e81ddbf71655aa29b0fed9a8b79a3cdd3620d1658eb1cc9a5d F ext/wasm/tests/opfs/concurrency/worker.js 0a8c1a3e6ebb38aabbee24f122693f1fb29d599948915c76906681bb7da1d3d2 @@ -2051,8 +2051,8 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0 -P a0f808363318c00fd1db78b4271cef8d05a046a36aab1a383e731e40603c6e2a -R f0e23b22b28a37c03d5b2048de2190dd +P ce0674b1925138f8f878b11aae0f8420bd968df0959f6dd7e208fb84bcbad07e +R 55a64755ba6bd7e4b55de6971891bfd0 U stephan -Z 9f51c4a5e0c22d8a19d776fe443c68e1 +Z 971bc1dc41287f5b57d1768c8e038762 # Remove this line to create a well-formed Fossil manifest. diff --git a/manifest.uuid b/manifest.uuid index 5044e3c62c..1163b9af10 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -ce0674b1925138f8f878b11aae0f8420bd968df0959f6dd7e208fb84bcbad07e \ No newline at end of file +795f22421b96cea27b3883a580d8ce29cc50dfb7d4f852b3d36ede3766c7a24c \ No newline at end of file