415947 - Upgrade to sqlite 3.5.4.2

Status: RESOLVED FIXED

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

Description

[Shawn Wilsher :sdwilsh]

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

Comment 1

[Shawn Wilsher :sdwilsh]

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

Comment 2

[Robert Sayre]

renom if you disagree, and explain what the newer sqlite gets us.

Comment 3

[Peter Weilbacher]

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.

Comment 4

[(not currently active) Ted Mielczarek]

I still don't think we'd block on this. You may, of course, still submit a patch.

Comment 5

[Mike Shaver (:shaver emeritus)]

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.

Comment 6

[Shawn Wilsher :sdwilsh]

(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).

Comment 7

[Peter Weilbacher]

(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.

Comment 8

[Mike Shaver (:shaver emeritus)]

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?

Comment 9

[Shawn Wilsher :sdwilsh]

(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 ;)

Comment 10

[Peter Weilbacher]

Attachment: Upgrade to SQLite 3.5.7

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.

Comment 11

[Peter Weilbacher]

Shawn, I haven't heard anything about about 3.5.8 yet.

Comment 12

[Peter Weilbacher]

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 <[e-mail]> 01/2008
+-- Peter Weilbacher <[e-mail]> 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.

Comment 13

[Peter Weilbacher]

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:[e-mail]-sqlite357u/

The one for Linux runs fine as does an OS/2 build with the patch.

Comment 14

[Shawn Wilsher :sdwilsh]

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!

Comment 15

[Peter Weilbacher]

Attachment: only changes to moz-specific code

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.

Comment 16

[Shawn Wilsher :sdwilsh]

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).

Comment 17

[Peter Weilbacher]

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.

Comment 18

[Shawn Wilsher :sdwilsh]

It shouldn't break NSS - they use whatever version we use.

Comment 19

[Peter Weilbacher]

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.

Comment 20

[Peter Weilbacher]

Attachment: update configure.in, too (checked in 2008-03-19 15:40, backed out 2008-03-20 12:17)

Updated patch including the configure.in change.

Can someone else please run make check?

Comment 21

[Shawn Wilsher :sdwilsh]

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.

Comment 22

[Peter Weilbacher]

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.]

Comment 23

[Shawn Wilsher :sdwilsh]

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?

Comment 24

[Peter Weilbacher]

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.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 26

[Peter Weilbacher]

Great, thanks for the help! Checked into trunk, obviously still in time for beta 5. :-)

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Likely caused bug 424163, beep-beep-beep.

Comment 28

[Brendan Eich [:brendan]]

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

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
aye - I'm immediately regretting that statement.

Comment 30

[Jesse Ruderman]

Gavin backed this out a few hours ago to fix the regression (bug 424163).

Comment 31

[Peter Weilbacher]

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...

Comment 32

[Peter Weilbacher]

stupid mid-air stuff...

Comment 33

[Mike Shaver (:shaver emeritus)]

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.)

Comment 34

[Shawn Wilsher :sdwilsh]

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?

Comment 35

[Mike Shaver (:shaver emeritus)]

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.

Comment 36

[Shawn Wilsher :sdwilsh]

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.

Comment 37

[Peter Weilbacher]

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?

Comment 38

[Shawn Wilsher :sdwilsh]

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

[Mike Schroepfer]

(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?

Comment 40

[Peter Weilbacher]

mtschrep: help with testing/debugging the issue on Windows and Linux, see comment 36 and comment 37.

Comment 41

[Peter Weilbacher]

I really meant "testing on Windows and _Mac_"

Comment 42

[Peter Weilbacher]

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].

Comment 43

[Peter Weilbacher]

Shawn, Mike, ping? I guess no comments mean that you haven't done anything on this? Any suggestions?

Comment 44

[Mike Schroepfer]

(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

[Myk Melez [:myk] [@mykmelez]]

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

[Myk Melez [:myk] [@mykmelez]]

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

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 48

[Shawn Wilsher :sdwilsh]

Perhaps we've found a threadsafety bug since we are running the DB on more than one thread?

Comment 49

[Peter Weilbacher]

Attachment: Update to 3.5.4.2

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.)

Comment 50

[Shawn Wilsher :sdwilsh]

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

Comment 51

[D. Richard Hipp]

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.

Comment 52

[Peter Weilbacher]

Attachment: Update to 3.5.4.2, including configure.in change

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.

Comment 53

[Shawn Wilsher :sdwilsh]

right, but if that switch works on your OS, it either needs to be disabled or set to 3.5.4.2

Comment 54

[Peter Weilbacher]

(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

[D. Richard Hipp]

(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.

Comment 56

[Peter Weilbacher]

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)?

Comment 57

[Peter Weilbacher]

Attachment: Update to 3.5.4.2, try 3 (checked in, 2008-04-15 12:29)

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.

Comment 58

[D. Richard Hipp]

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.

Comment 59

[Peter Weilbacher]

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?

Comment 60

[Shawn Wilsher :sdwilsh]

I've been told by one driver that we'll take 3.5.4.2, but not anything newer.

Comment 61

[Shawn Wilsher :sdwilsh]

Comment on attachment 315387 [details] [diff] [review]
Update to 3.5.4.2, try 3 (checked in, 2008-04-15 12:29)

r=sdwilsh

Comment 62

[Mike Beltzner [:beltzner, not reading bugmail]]

sdwilsh, can you give us a risk/reward here? The "try #3" doesn't lend a ton of confidence ...

Comment 63

[Shawn Wilsher :sdwilsh]

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

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 65

[Peter Weilbacher]

Shawn, should I still check in 3.5.4.2 now?

Comment 66

[Shawn Wilsher :sdwilsh]

Yes

Comment 67

[Peter Weilbacher]

OK, done. I guess we should take a possible upgrade to 3.5.8 into a new bug, so I'll close this one.

Comment 68

[Walter Meinl]

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)?

Comment 69

[Peter Weilbacher]

Attachment: 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.

Comment 70

[D. Richard Hipp]

(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.

Comment 71

[Shawn Wilsher :sdwilsh]

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).