Closed Bug 1108328 (SQLite3.8.7.4) Opened 8 years ago Closed 8 years ago

Upgrade to SQLite


(Toolkit :: Storage, defect)

Not set



Tracking Status
firefox36 --- fixed
firefox37 --- fixed


(Reporter: ionnv, Assigned: RyanVM)





(1 file, 1 obsolete file)

"This release fixes two obscure bugs that can result in incorrect query results and/or application crashes, but not (as far as we can tell) security vulnerabilities. Both bugs have been latent in the code across multiple prior releases and have never before been encountered, so they are unlikely to cause problems. Nevertheless it seems prudent to publish fixes for them both. See the change log for details."
Depends on: SQLite3.8.7.2
I think we're going to want this on Aurora as well.
Alias: SQLite3.8.7.3
Assignee: nobody → ryanvm
Ever confirmed: true
Looks like we omitted a necessary mutex on the change.  Can somebody please do a test run with to see if that clears the problem.  (All of the existing SQLite test cases pass, so our own tests are of no help in determining whether or not the problem has been fixed.)

The functional change in the snapshot above is:

--- ../historical/	2014-12-08 10:25:36.223141678 -0500
+++ sqlite3.c	2014-12-08 12:45:20.075284800 -0500
@@ -125896,11 +125896,13 @@
     if( pDb->pBt ){
       if( pDb->pSchema ){
         /* Must clear the KeyInfo cache.  See ticket [e4a18565a36884b00edf] */
+        sqlite3BtreeEnter(pDb->pBt);
         for(i=sqliteHashFirst(&pDb->pSchema->idxHash); i; i=sqliteHashNext(i)){
           Index *pIdx = sqliteHashData(i);
           pIdx->pKeyInfo = 0;
+        sqlite3BtreeLeave(pDb->pBt);
       pDb->pBt = 0;

So you can just apply the patch above to the SQLite rather than downloading a new amalgamation file, if that is easier.

If the above change works for Firefox, then it will become SQLite version later this week.
Alias: SQLite3.8.7.3 → SQLite3.8.7.4
Summary: Upgrade to SQLite → Upgrade to SQLite
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #6)

No crashes like we were getting with Better yet, Win7 talos appears to like it too!
A source-code audit, based on the prior failure, reveals another place where a mutex is needed, though apparently not a place that is important to FF as the tests are working without this extra mutex.  There is a new snapshot at which contains both fixes, and also updates the version number to  Ryan, if you can put this new snapshot through the try server once, we'll release it as SQLite, probably early tomorrow morning EST.  We'd probably be OK releasing it without the extra try-server run, but we want to avoid having to issue a version, so we'd feel better knowing that the new code works well in FF.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #7)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #6)
> >
> No crashes like we were getting with Better yet, Win7 talos appears
> to like it too!

However, I did manage to hit an instance of bug 1101419 still.

We'll see what the build from comment 8 does.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #9)
> (In reply to D. Richard Hipp from comment #8)
> (PGO
> enabled on supported platforms)

I managed to hit both bug 1103966 and bug 1101419 in these runs, so while version doesn't seem to make things any worse, it doesn't seem to have fixed anything either.
Yeah.  The signatures of but 1103966 and bug 1101419 did not match the problems that fixed.  So it was a long shot.  But worth trying.
Attachment #8533185 - Attachment is obsolete: true
Attachment #8533903 - Flags: review?(mak77)
Attachment #8533903 - Flags: review?(mak77) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8533903 [details] [diff] [review]
Upgrade to SQLite

Approval Request Comment
[Feature/regressing bug #]: Pre-existing SQLite bugs
[User impact if declined]: Possible memory corruption and crashes
[Describe test coverage new/current, TBPL]: On trunk and green Try runs and passes SQLite's test suites
[Risks and why]: Low risk, we've already seen with the patches in this bug that our test infra can detect problems caused by this code.
[String/UUID change made/needed]: None
Attachment #8533903 - Flags: approval-mozilla-aurora?
Attachment #8533903 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1109956
Looks like 3.8.8 just got released:
You need to log in before you can comment on or make changes to this bug.