Closed
Bug 415947
Opened 16 years ago
Closed 15 years ago
Upgrade to sqlite 3.5.4.2
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
People
(Reporter: sdwilsh, Assigned: mozilla)
Details
Attachments
(2 files, 5 obsolete files)
2.08 KB,
patch
|
mozilla
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
15.68 KB,
patch
|
sdwilsh
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
3.5.5 had some issues, but I think we should take this one for b4 ====== There is very little change from version 3.5.5. The reason for this release is that there was a regression in the virtual table mechanism that prevented virtual tables from being used in LEFT OUTER JOINs. And there were some updates to the OS/2 driver. Also, I bungled the release of version 3.5.5 such that some users downloaded copies of 3.5.4 that were labeled 3.5.5. We have still not encountered any bugs in new register-based virtual machine. You might recall that this was a huge change moving from version 3.5.4 to 3.5.5 and there was some concern that the change my destablize the core. But such concern is now past. Version 3.5.6 is recommended for all users and is consider stable and ready for production release. -drh
Flags: blocking1.9?
Reporter | ||
Comment 1•16 years ago
|
||
Let's go for 3.5.7 instead. Note, the bug they found seems super edge-casey (I know we don't do any query like that in our code). ====== I had been saying in release announcements that no bugs have been found in the new register-based virtual machine introduced in SQLite version 3.5.5. That changed with ticket #2927. We have now observed our first register-VM bug. http://www.sqlite.org/cvstrac/tktview?tn=2927 There will likely be a new release (version 3.5.7) within a few days in order to fix the problem discovered by ticket #2927. -drh
Summary: Upgrade to sqlite 3.5.6 → Upgrade to sqlite 3.5.7
Comment 2•16 years ago
|
||
renom if you disagree, and explain what the newer sqlite gets us.
Flags: blocking1.9? → blocking1.9-
Assignee | ||
Comment 3•15 years ago
|
||
FWIW, for OS/2 we really need a newer SQLite than is currently in the tree. We fixed a fairly drastic bug (http://www.sqlite.org/cvstrac/tktview?tn=2905) which would have made it impossible to run two instances of Firefox at the same time (or running another app that uses mozStorage together with Firefox). That fix was put into SQLite in time for 3.5.6. In addition to that critical bug we also fixed a few more which will be in 3.5.7. I hear that SQLite 3.5.7 will only be out on Monday or Tuesday which badly conflicts with the beta 5 code freeze, so at least let's put it after the beta.
Flags: blocking1.9?
Comment 4•15 years ago
|
||
I still don't think we'd block on this. You may, of course, still submit a patch.
Flags: blocking1.9? → blocking1.9-
We might also consider giving the OS/2 guys permission to use a newer sqlite in their builds, while retaining the ability to use the trademark. That would isolate the risk; I'm not sure taking this after the last beta is a great idea.
Reporter | ||
Comment 6•15 years ago
|
||
(In reply to comment #5) > We might also consider giving the OS/2 guys permission to use a newer sqlite in > their builds, while retaining the ability to use the trademark. That would > isolate the risk; I'm not sure taking this after the last beta is a great idea. Why not? Our components that use storage have fairly extensive unit tests on the parts that use storage, not the mention our test suite on the storage service itself. What would we do if the sqlite folks find a security vulnerability in some release - try to fix just that in our version? I understand not wanting to take a major version change, but a point release I don't exactly agree with (but then I'm also not a driver so my opinion isn't worth much).
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #5) > We might also consider giving the OS/2 guys permission to use a newer sqlite in > their builds, while retaining the ability to use the trademark. Like building SQLite from another directory on OS/2? I don't like to have to add another SQLite version to CVS but if that's what you decide, I'm OK with it.
Don't point releases of sqlite sometimes include wholesale rewrites of the interpreter? If you think the specific update doesn't require beta exposure (meaning that it's unlikely to have bugs that we need users and their extensions to find), that's good to know -- I was concerned about bugs that impacted extensions, since I don't think we want to be counting on finding those with RC1. Are there try server builds or anything with the new sqlite?
Reporter | ||
Comment 9•15 years ago
|
||
(In reply to comment #8) > Don't point releases of sqlite sometimes include wholesale rewrites of the > interpreter? If you think the specific update doesn't require beta exposure > (meaning that it's unlikely to have bugs that we need users and their > extensions to find), that's good to know -- I was concerned about bugs that > impacted extensions, since I don't think we want to be counting on finding > those with RC1. The sqlite folks actually have near 100% test-coverage across their code to make sure that they don't regress behaviors. If they do want to change a behavior, they do more than a minor version bump. Yeah, sometimes they do some big things in their point releases, so I'd still want to evaluate it each time. With that said, I don't think we should automatically rule out upgrading sqlite once we get past the betas. If we really wanted the latest version of sqlite in the tree, it'd probably take me about 20 minutes to generate the patch, which I could do if we really wanted it. Peter - do you know when 3.5.8 is going to be out? Upgrading is *really* easy to do (instructions here: http://mxr.mozilla.org/seamonkey/source/db/sqlite3/README.MOZILLA) > Are there try server builds or anything with the new sqlite? That'd require someone to put together a patch first ;)
Assignee | ||
Comment 10•15 years ago
|
||
OK, this is the patch. Had to compress it with bzip2 to get it to upload to Bugzilla. Will submit it to the try server in a minute.
Assignee | ||
Comment 11•15 years ago
|
||
Shawn, I haven't heard anything about about 3.5.8 yet.
Assignee | ||
Comment 12•15 years ago
|
||
Ah, the "signature" in README.MOZILLA is also about the version, so that part of the patch should change it to my name, like this: @@ -1,12 +1,12 @@ -This is sqlite 3.5.4.1 +This is sqlite 3.5.7 --- Shawn Wilsher <me@shawnwilsher.com> 01/2008 +-- Peter Weilbacher <mozilla@weilbacher.org> 03/2008 See http://www.sqlite.org/ for more info. We have a mozilla-specific Makefile.in in src/ (normally no Makefile.in there) that we use to build.
Assignee | ||
Comment 13•15 years ago
|
||
OK, the patch was also created on OS/2 and so has DOS-like linebreaks which caused a problem with the Win try server build the first time. Once I converted it to Unix-like linebreaks all three machines finished fine. The builds are here: https://build.mozilla.org/tryserver-builds/2008-03-18_03:08-mozilla@weilbacher.org-sqlite357u/ The one for Linux runs fine as does an OS/2 build with the patch.
Target Milestone: mozilla1.9beta4 → mozilla1.9beta5
Reporter | ||
Comment 14•15 years ago
|
||
Peter - could you do me a favor and seperate the changes to the sqlite files and the mozilla files (I don't need to see the changes to sqlite3.c or sqlite3.h, just everything else). I'll be able to get to your review later today between/during classes, but it'll be a big help if you can split those two up. Thanks!
Assignee | ||
Comment 15•15 years ago
|
||
Sure, with that the patch is trivial. Note that sqlite3_apis was renamed to sqlite3Apis and marked SQLITE_PRIVATE. As it's not used directly anywhere in the Mozilla sources, I don't think that is a problem.
Attachment #310198 -
Attachment is obsolete: true
Attachment #310261 -
Flags: review?(sdwilsh)
Attachment #310198 -
Flags: review?(sdwilsh)
Reporter | ||
Comment 16•15 years ago
|
||
Comment on attachment 310261 [details] [diff] [review] only changes to moz-specific code r=sdwilsh, once you update this too: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/configure.in&rev=1.1962#131 That's also assuming you've ran |make check| on the storage tests too :) I don't know why it's only requiring 3.5 as it is... Drivers: I consider this low-risk given ours and sqlite's test coverage on this stuff, but it is upgrading an external library used by many parts of our code *and* by add-on authors. If we do want to take this, it'd probably be better to get this out with a beta than a release candidate (although I don't really think that matters either given our test infrastructure with storage).
Attachment #310261 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 17•15 years ago
|
||
If I change configure.in to require 3.5.7 will that not break NSS? Will try make check but I somehow doubt that runs on OS/2. I don't currently have diskspace to compile it on Linux.
Reporter | ||
Comment 18•15 years ago
|
||
It shouldn't break NSS - they use whatever version we use.
Assignee | ||
Comment 19•15 years ago
|
||
OK, but if we just update it in configure.in every time then README.MOZILLA isn't really correct any more, as that says If mozstorage or nss code need changes due to API changes in sqlite3, be sure to update SQLITE_VERSION accordingly in $(topsrcdir)/configure.in. Well, I now ran make check, I get mostly PASS but some FAILures due to path issues that are probably OS/2 only.
Assignee | ||
Comment 20•15 years ago
|
||
Updated patch including the configure.in change. Can someone else please run make check?
Attachment #310261 -
Attachment is obsolete: true
Attachment #310285 -
Flags: review+
Reporter | ||
Comment 21•15 years ago
|
||
Sorry - the important tests to run are: make -C storage/test/ check make -C toolkit/components/downloads/test/ check make -C toolkit/components/places/tests/ check Those three hit the storage code extensively. (In reply to comment #19) > OK, but if we just update it in configure.in every time then README.MOZILLA > isn't really correct any more, as that says It probably should be changed. I actually wanted the minimum to be what we compiled with - not sure why it didn't land that way in the first place. Feel free to update that before checking in.
Assignee | ||
Comment 22•15 years ago
|
||
Nice to know which tests to run but as I said somebody else needs to do that. [On OS/2 I get stuff like X:\trunk\fx_d\storage\test\X:\trunk\mozilla\storage\test\unit\corruptDB.sqlite doesn't exist and I don't really have time to debug that double-path thing now to get the tests to actually test what they are supposed to.]
Reporter | ||
Comment 23•15 years ago
|
||
eww - that's a test harness bug that you should probably file (function that is used for that is do_get_file). I won't have time to do this today - can anyone else get to this by chance?
Assignee | ||
Comment 24•15 years ago
|
||
Comment on attachment 310285 [details] [diff] [review] update configure.in, too (checked in 2008-03-19 15:40, backed out 2008-03-20 12:17) OK, I finally found a Linux machine that had enough disk space and is new enough to compile trunk. |make check| in the directories pointed out by Shawn showed me all PASSes. So I think this is good to go.
Attachment #310285 -
Flags: approval1.9?
Comment 25•15 years ago
|
||
Comment on attachment 310285 [details] [diff] [review] update configure.in, too (checked in 2008-03-19 15:40, backed out 2008-03-20 12:17) a1.9=beltzner
Attachment #310285 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 26•15 years ago
|
||
Great, thanks for the help! Checked into trunk, obviously still in time for beta 5. :-)
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 27•15 years ago
|
||
Likely caused bug 424163, beep-beep-beep.
Comment 28•15 years ago
|
||
Here may be where the "near" in "The sqlite folks actually have near 100% test-coverage across their code" is not good enough. I don't know for sure, but when I saw that statement I cringed. It's very hard to get all-paths coverage on non-trivially small code. It's an active research problem how far you can go with approaches such as CUTE/jCUTE and Engler's Disks of Death work. /be
Reporter | ||
Comment 29•15 years ago
|
||
(In reply to comment #28) aye - I'm immediately regretting that statement.
Comment 30•15 years ago
|
||
Gavin backed this out a few hours ago to fix the regression (bug 424163).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9beta5 → ---
Assignee | ||
Comment 31•15 years ago
|
||
brendan, sdwilsh: I don't think this is something to cringe about. Of course it's always possible to miss things, as we have seen now. But AFAIK their tests have still much better coverage than Mozilla's, for a much smaller codebase. Their full testsuite has more than 180000 tests! (Don't know, how many does Mozilla have?) Btw, did anybody test the try-server builds at all that I created because shaver asked about that? Turns out the crash occurred on the two platforms that I couldn't test myself...
Target Milestone: --- → mozilla1.9beta5
We're not cringing about there being bugs; as you say, it happens. We're cringing about the reasoning that their test suite was so good that we didn't need beta coverage. We know that our tests aren't exhaustive, which is why we want beta coverage for invasive changes, like upgrading sqlite. (Though in this case it didn't quite make it that far, at least yet.)
Reporter | ||
Comment 34•15 years ago
|
||
after a lot more testcase writing, sqlite folks have figured out our crash. Also told me that it should have caused an assertion, but I'm guessing we compile with assertions turned off (or non-fatal)? Can we change that on a per module basis by chance?
Assertions should be fatal in debug builds, absent entirely in release. JS and NSPR's assertions follow those rules (and are infrequently encountered because of it), while XPCOM's are more lax and therefore treated like error checking or debugging notes.
Reporter | ||
Comment 36•15 years ago
|
||
From drh:
> As I indicated, I now have 100% condition/decision test coverage
> on the Bitvec module. But looking at my diffs, I don't see
> how this has fixed anything. The only way something could have
> been fixed is if sqlite3BitvecTest() were called with the 2nd
> argument of 0. I handle that case now, whereas before it would
> segfault. But there was previously a
>
> assert( i>0 );
>
> at the beginning of the procedure. And none of the SQLite
> regression tests hit that assert, so it is not clear how this
> might be happening.
>
> Two other routines in the Bitvec module, sqlite3BitvecSet()
> and sqlite3BitvecClear(), continue to have the potential to
> segfault if given an illegal i==0 input. They both assert
> i>0. But if Firefox is managing to induce inputs that I
> cannot replicate, and firefox has asserts turned off, then
> there is certainly no guarantee that the illegal input
> will not occur.
>
> Bottom line: There is a fix in the CVS tree. But it is unclear
> if the fix will resolve your issue. I need more data.
So, we need to figure out how to reproduce that crash we were seeing. I think it might be possible that we have a bug on our end of things.
Keywords: qawanted
Assignee | ||
Comment 37•15 years ago
|
||
So, somebody on Mac and/or Windows needs to compile debug with SQLite 3.5.7 and set a breakpoint in sqlite3BitvecTest() for the case of i = 0. Who can do that? If I set up a try-server build with debug option for that, will somebody use it? Otherwise, the safer option for FF 3 would be to merge the two important OS/2 fixes into the SQLite amalgamation that we already have (and that calls itself 3.5.4.1). It's no problem for me to do that, but do we need drh again to provide such a version or will I be allowed to do that?
Reporter | ||
Comment 38•15 years ago
|
||
We have a support contract with them, so that's the preferred way to do this. What is the likelyhood of someone else being able to help on this? I do not have cycles for it...
Comment 39•15 years ago
|
||
(In reply to comment #38) > We have a support contract with them, so that's the preferred way to do this. > What is the likelyhood of someone else being able to help on this? I do not > have cycles for it... > Help with what?
Assignee | ||
Comment 40•15 years ago
|
||
mtschrep: help with testing/debugging the issue on Windows and Linux, see comment 36 and comment 37.
Assignee | ||
Comment 41•15 years ago
|
||
I really meant "testing on Windows and _Mac_"
Assignee | ||
Comment 42•15 years ago
|
||
Hmm, so nobody wants to do testing on the problematic platforms. Can you (Shawn or Mike?) then please request from drh to create a 3.5.4.1-based amalgamation for us that at contains least the following check-ins from SQLite CVS: [4784], [4771], [4770], and [4766].
Assignee | ||
Comment 43•15 years ago
|
||
Shawn, Mike, ping? I guess no comments mean that you haven't done anything on this? Any suggestions?
Updated•15 years ago
|
Attachment #310198 -
Attachment is patch: false
Attachment #310198 -
Attachment mime type: text/plain → application/x-Bzip
Comment 44•15 years ago
|
||
(In reply to comment #43) > Shawn, Mike, ping? I guess no comments mean that you haven't done anything on > this? Any suggestions? > Myk is testing for the crash now - I'll reach out to the SQLite folks...
Comment 45•15 years ago
|
||
So far I haven't been able to reproduce the crash on Mac. I spent a couple hours last night browsing a variety of web sites first on my regular profile and then in a fresh profile. I also tried running a pageload test and leaving the browser running all night. I also tried building optimized in case the problem doesn't show up in debug builds, but no luck. I built from the tip with only the db/sqlite3/ files reverted to their state on March 20 (plus the configure.in change). I also tried building after applying the patch in this bug as well as downloading a fresh amalgamation from SQLite. The one thing I haven't done is reverted the entire tree to its March 20 state. It's possible the bug has been fixed since then elsewhere in the code, so I'll try that next.
Comment 46•15 years ago
|
||
Note: I looked at reports of the problem <http://crash-stats.mozilla.com/report/list?range_unit=days&query_search=signature&query_type=contains&product=Firefox&signature=sqlite3BitvecSet&date=2008-03-21&range_value=1>, and several mentioned Basecamp, so I signed up for an account there and did some browsing around and creating of a To Do list there, but Firefox didn't crash.
Comment 47•15 years ago
|
||
I finally crashed with EXC_BAD_ACCESS: #0 0x13b78ca1 in sqlite3BitvecSet (p=0x342c3131, i=18) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:22769 #1 0x13b78dcf in sqlite3BitvecSet (p=0x1f17e808, i=18) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:22783 #2 0x13b7de70 in sqlite3PagerDontRollback (pPg=0x1e842aa8) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:27282 #3 0x13b84b18 in allocateBtreePage (pBt=0x1cc24268, ppPage=0xb0650e80, pPgno=0xb0650e7c, nearby=0, exact=0 '\0') at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:33057 #4 0x13b85c6d in balance_quick (pPage=0x1e7aa7e8, pParent=0x1e8b1ef8) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:33678 #5 0x13b8601d in balance_nonroot (pPage=0x1e7aa7e8) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:33835 #6 0x13b87887 in balance (pPage=0x1e7aa7e8, insert=1) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:34516 #7 0x13b87c7c in sqlite3BtreeInsert (pCur=0x198140c8, pKey=0x0, nKey=47574, pData=0x1da46208, nData=14, nZero=0, appendBias=0) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:34648 #8 0x13b964ab in sqlite3VdbeExec (p=0x1cc2cd78) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:43801 #9 0x13b901af in sqlite3Step (p=0x1cc2cd78) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:39788 #10 0x13b90490 in sqlite3_step (pStmt=0x1cc2cd78) at /Users/myk/Mozilla/source/mozilla/db/sqlite3/src/sqlite3.c:39850 #11 0x13efb5b6 in mozStorageStatement::ExecuteStep (this=0x1cc2c030, _retval=0xb0651990) at /Users/myk/Mozilla/source/mozilla/storage/src/mozStorageStatement.cpp:472 #12 0x13efab20 in mozStorageStatement::Execute (this=0x1cc2c030) at /Users/myk/Mozilla/source/mozilla/storage/src/mozStorageStatement.cpp:448 #13 0x13f86be4 in nsUrlClassifierStore::WriteEntry (this=0x1cc22e08, entry=@0x17651ec8) at /Users/myk/Mozilla/source/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:1788 #14 0x13f909de in nsUrlClassifierDBServiceWorker::AddChunk (this=0x1cc22df0, tableId=3, chunkNum=980, entries=@0xb0651cb8) at /Users/myk/Mozilla/source/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:2316 #15 0x13f90d5c in nsUrlClassifierDBServiceWorker::ProcessChunk (this=0x1cc22df0, done=0xb0651d3c) at /Users/myk/Mozilla/source/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:2439 #16 0x13f91029 in nsUrlClassifierDBServiceWorker::UpdateStream (this=0x1cc22df0, chunk=@0x1e7d6590) at /Users/myk/Mozilla/source/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp:2806 #17 0x003bd007 in NS_InvokeByIndex_P (that=0x1cc22df0, methodIndex=8, paramCount=1, params=0x19a4ffb0) at /Users/myk/Mozilla/source/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp:179 #18 0x003ac2dc in nsProxyObjectCallInfo::Run (this=0x187d87b0) at /Users/myk/Mozilla/source/mozilla/xpcom/proxy/src/nsProxyEvent.cpp:181 #19 0x003a28fa in nsThread::ProcessNextEvent (this=0x1cc22990, mayWait=1, result=0xb0651ecc) at /Users/myk/Mozilla/source/mozilla/xpcom/threads/nsThread.cpp:510 #20 0x0032de4c in NS_ProcessNextEvent_P (thread=0x1cc22990, mayWait=1) at nsThreadUtils.cpp:227 #21 0x003a2b09 in nsThread::ThreadFunc (arg=0x1cc22990) at /Users/myk/Mozilla/source/mozilla/xpcom/threads/nsThread.cpp:254 #22 0x0056c2f0 in _pt_root (arg=0x1cc22ba0) at /Users/myk/Mozilla/source/mozilla/nsprpub/pr/src/pthreads/ptthread.c:221 #23 0x907bfc55 in _pthread_start () #24 0x907bfb12 in thread_start () I wasn't using the browser at the time, so I have no steps to reproduce besides "wait for it to crash," but gdb reports that i is 18, not 0, in the last frame, so it doesn't seem like that i==0 issue previously discussed. Not being familiar with this code, I'm not sure what to investigate further, but I've got the crashed process sitting in my debugger and can dig deeper if someone can suggest avenues for productive exploration.
Reporter | ||
Comment 48•15 years ago
|
||
Perhaps we've found a threadsafety bug since we are running the DB on more than one thread?
Assignee | ||
Comment 49•15 years ago
|
||
OK, in case the problem with SQLite 3.5.7 can't be found, here is the patch to just update from 3.5.4.1 to 3.5.4.2 as created by the SQLite team. (To make it manageable I removed the hunks that just contained $Id:...$ differences.)
Attachment #315084 -
Flags: review?(sdwilsh)
Reporter | ||
Updated•15 years ago
|
Summary: Upgrade to sqlite 3.5.7 → Upgrade to sqlite 3.5.4.2
Reporter | ||
Comment 50•15 years ago
|
||
Comment on attachment 315084 [details] [diff] [review] Update to 3.5.4.2 We also need to update the configure.in to at least check for 3.5.4, but if the system-sqlite stuff works on os/2, please set it to 3.5.4.2
Attachment #315084 -
Flags: review?(sdwilsh) → review-
Comment 51•15 years ago
|
||
Look at the top two entries in Myk's stack trace above. The Bitvec object is obtained from malloc() and from the second entry down on the stack trace we can see that malloc() is returning addresses in the vicinity of 0x1f000000. The sqlite3BitvecSet() routine sometimes calls itself recursively (in order to walk a tree of Bitvec objects) and you an see this is what happened above. But the interesting thing is that the pointer to the recursive Bitvec object (which should be the second layer down in the tree) is no longer in the range of addresses that malloc() is giving out. In fact, the address looks like it might be part of an ASCII text string: Either "4,11" or "11,4" depending on byte order. This pointer would have been taken from bytes 12-15 of the original Bitvec structure, so I'm guessing that the four bytes shown are likely part of a larger string. It might help in tracking down the problem to know what the complete string is. I can imagine a number of things might be going wrong here. It could be that SQLite is trying to use a buffer that it has already freed and that same buffer has been picked up by some other module and is currently being used for something different. Or, it could be that another module, unrelated to SQLite, is writing into a buffer that it had previously freed and that SQLite just happened to allocate and begin using for a Bitvec object. There are other possibiilities, of course, but these seem to me to be the most likely explanations. We (the SQLite developers) will run the latest SQLite code through valgrind to see if we can find any places where it is using memory buffers after they are being freed. During debugging, we use an alternative malloc implementation that overwrites memory buffers when they are freed, in order to detect this kind of problem early, and we haven't seen anything, but sometimes valgrind will uncover problems that other debugging aids miss. Other than that, I'm not sure what else we can do to work on this problem without additional clues. Anybody who does see additional clues should contact me right away. Please note that there is a significant possibility that this problem has nothing whatsoever to do with SQLite. If that is the case, then reverting to SQLite version 3.5.4.2 is only treating the symptom - the underlying problem still remains and might well be causing other seemingly unrelated crashes.
Assignee | ||
Comment 52•15 years ago
|
||
Sorry Shawn, you are right of course. configure.in should be updated, too, but better update it only to 3.5.4 to not cause confusion for e.g. Linux distros.
Attachment #315084 -
Attachment is obsolete: true
Attachment #315369 -
Flags: review?(sdwilsh)
Reporter | ||
Comment 53•15 years ago
|
||
right, but if that switch works on your OS, it either needs to be disabled or set to 3.5.4.2
Assignee | ||
Comment 54•15 years ago
|
||
(In reply to comment #53) > right, but if that switch works on your OS, it either needs to be disabled or > set to 3.5.4.2 As there is no 3.5.4.2 release that other people could get (it's not listed on http://www.sqlite.org/download.html or anywhere else on sqlite.org) I don't see how that could be fulfilled for a --system-sqlite, so setting that for any platform doesn't make sense. And yes, it should work on OS/2 but I don't know anybody who builds with that switch, so we really shouldn't worry about that. If people get broken builds because of that switch they know where to ask.
Comment 55•15 years ago
|
||
(In reply to comment #54) > > As there is no 3.5.4.2 release that other people could get (it's not listed on > http://www.sqlite.org/download.html or anywhere else on sqlite.org) There is a branch for version 3.5.4 in a separate configuration management system at http://www.sqlite.org/ Log in as user "anonymous" with password "anonymous" and you will be able to download a ZIP archive containing the latest source code.
Assignee | ||
Comment 56•15 years ago
|
||
Richard, sure, I know that's the code we plan to be using (in the amalgamation created by Dan). But is that something that is known in general (it was new to me)?
Assignee | ||
Comment 57•15 years ago
|
||
Sorry, I should have updated README.Mozilla from the start, that change is included now. As we now try to update the version number in configure.in every time, I changed the text about that, too.
Attachment #315369 -
Attachment is obsolete: true
Attachment #315387 -
Flags: review?(sdwilsh)
Attachment #315369 -
Flags: review?(sdwilsh)
Comment 58•15 years ago
|
||
Myk Melez captured an instance of this crash in gdb and allowed me to poke around in the resulting image. This enabled me to isolate the problem. It is a bug in SQLite. The bug is only manifest when SQLite is compiled with SQLITE_SECURE_DELETE=1, which is a configuration we have not tested lately. When that compile-time option is enabled, the bug is easily detected at multiple points in our test suite. The problem was introduced by SQLite check-in [4733] http://www.sqlite.org/cvstrac/chngview?cn=4733 just prior to SQLite release 3.5.5. The problem is fixed, we believe, by SQLite check-in [4999] http://www.sqlite.org/chngview?cn=4999 The problem was introduced when we attempted to remove some dead code. (It seems the code was not really dead when SQLITE_SECURE_DELETE was defined.) The problem had nothing whatsoever to do with the refactoring of the virtual machine or the introduction of the Bitvec object which were the major changes in release 3.5.5. I think the best way to avoid a repeat of this kind of problem is for the SQLite team to always run a full set of tests using the exact same configuration options as are used by Mozilla.
Assignee | ||
Comment 59•15 years ago
|
||
Richard, that's great news. So let me suggest a way to proceed for Mozilla: when 3.5.8 is out I'll create a new patch and try-server builds with that. Then somebody (Myk?) can test with that, at least on the Mac, and if nothing more shows up we can try to get it into the tree. And watch crash reports closely until the next day. Alternatively, I could try to build an amalgamation from the current CVS source of SQLite and make builds with that, to get testing done even faster. Shawn, Mike, does that sound reasonable or is it too late already?
Reporter | ||
Comment 60•15 years ago
|
||
I've been told by one driver that we'll take 3.5.4.2, but not anything newer.
Reporter | ||
Comment 61•15 years ago
|
||
Comment on attachment 315387 [details] [diff] [review] Update to 3.5.4.2, try 3 (checked in, 2008-04-15 12:29) r=sdwilsh
Attachment #315387 -
Flags: review?(sdwilsh)
Attachment #315387 -
Flags: review+
Attachment #315387 -
Flags: approval1.9?
Comment 62•15 years ago
|
||
sdwilsh, can you give us a risk/reward here? The "try #3" doesn't lend a ton of confidence ...
Reporter | ||
Comment 63•15 years ago
|
||
It's try number three because he did update comments in the necessary files. This has only OS/2 fixes from what we have in tree now, so virtually no risk! Note: in discussion with schrep, we are likely to take 3.5.8 since the issue was found out, and the sqlite test suite will now be run, in addition to how they normally compile it, with the options that we compile it with.
Comment 64•15 years ago
|
||
Comment on attachment 315387 [details] [diff] [review] Update to 3.5.4.2, try 3 (checked in, 2008-04-15 12:29) a1.9=beltzner
Attachment #315387 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 65•15 years ago
|
||
Shawn, should I still check in 3.5.4.2 now?
Reporter | ||
Comment 66•15 years ago
|
||
Yes
Assignee | ||
Comment 67•15 years ago
|
||
OK, done. I guess we should take a possible upgrade to 3.5.8 into a new bug, so I'll close this one.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Attachment #315387 -
Attachment description: Update to 3.5.4.2, try 3 → Update to 3.5.4.2, try 3 (checked in, 2008-04-15 12:29)
Assignee | ||
Updated•15 years ago
|
Attachment #310285 -
Attachment description: update configure.in, too → update configure.in, too (checked in 2008-03-19 15:40, backed out 2008-03-20 12:17)
Comment 68•15 years ago
|
||
There's a problem on linux, when building with --with-system-sqlite3, as the sqlite3.pc file reports as version only 3.5. Name: SQLite Description: SQL database engine Version: 3.5 Libs: -L${libdir} -lsqlite3 Thus, the build bails out during configure time: checking for sqlite3 >= 3.5.4... Requested 'sqlite3 >= 3.5.4' but version of SQL ite is 3.5 configure: error: Library requirements (sqlite3 >= 3.5.4) not met; consider adju sting the PKG_CONFIG_PATH environment variable if your libraries are in a nonsta ndard prefix so pkg-config can find them. I have installed sqlite-3.5.6 Wasn't the version check in configure(.in) for sqlite just implemented for building against system-sqlite3 (bug263381)?
Assignee | ||
Comment 69•15 years ago
|
||
Right, the SQLite build system will only ever write major.minor version into the .pc file, so I guess we have to restrict ourselves to that granularity, too. And change the update instructions accordingly.
Attachment #315918 -
Flags: review?(sdwilsh)
Comment 70•15 years ago
|
||
(In reply to comment #69) > Created an attachment (id=315918) [details] > fix for system sqlite > > Right, the SQLite build system will only ever write major.minor version into > the .pc file, so I guess we have to restrict ourselves to that granularity, > too. And change the update instructions accordingly. > I don't know what the "sqlite3.pc" files is. It is certainly not something that we SQLite developers care about. Peter, if you want to check in patches (presumably to the autoconf files) to fix this, please feel free.
Reporter | ||
Comment 71•15 years ago
|
||
Comment on attachment 315918 [details] [diff] [review] fix for system sqlite This issue is being addressed in bug 424063. We have a good reason to check against 3.5.4 and not 3.5 (we should also probably be bounding it with a max version because of issues we hit with 3.5.7, but that's not this bug).
Attachment #315918 -
Attachment is obsolete: true
Attachment #315918 -
Flags: review?(sdwilsh)
You need to log in
before you can comment on or make changes to this bug.
Description
•