Closed Bug 741821 Opened 8 years ago Closed 8 years ago
Don't access a null sql stmt
I have noticed this crash on try: https://tbpl.mozilla.org/php/getParsedLog.php?id=10577572&tree=Try&full=1#error1 What I think is happening is * regress-617935.js makes us allocate a lot of memory * another thread is calling sqlite3_prepare_v2 which indirectly calls sqlite3VdbeSetSql. * sqlite3VdbeSetSql tries to init zSql by calling sqlite3DbStrNDup * sqlite3DbStrNDup calls sqlite3DbMallocRaw which returns null * the machine is still under load because of regress-617935.js, so we try to report a slow sql statement * sqlite3_sql returns zSql which is null * We create a nsDependentCString and then a nsACString with the null pointer. This crashes trying to compute its length. I am amazed that the sqlite api is eating this error. Should we change it? This patch instead just avoids the crash :-(
8 years ago
Attachment #611832 - Flags: review?(vdjeric)
Comment on attachment 611832 [details] [diff] [review] don't crash There isn't much we can do in general if we're out of memory, Firefox likely can't keep running. I'm ok with this patch just avoiding the segfault here.
Attachment #611832 - Flags: review?(vdjeric) → review+
OK, I will check this in. I reported the issue to sqlite in http://thread.gmane.org/gmane.comp.db.sqlite.general/73439 and if they fix it we can revert this.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Reopening since the problem was with us passing around a null stmt. The root problem was fixed in 743650. A dummy try with an assert instead of an error code is at https://tbpl.mozilla.org/?tree=Try&rev=efb3a59cfb65 I will attach the real patch in a sec.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch does 3 things: *) Revert the previous "fix", it should never see a NULL *) Add an assert about it, so that we don't need to get lucky with a timeout again to find this. *) Fix our usage of sqlite3_prepare_v2 to not assume that an OK result means it produced an statement. https://tbpl.mozilla.org/?tree=Try&rev=b4a35f4a1251
Comment on attachment 613667 [details] [diff] [review] fix usage of sqlite3_prepare_v2 Looks good to me but I don't know if I am the right reviewer since I'm not module owner/peer for storage :P
Attachment #613667 - Flags: review?(vdjeric) → review+
Comment on attachment 613667 [details] [diff] [review] fix usage of sqlite3_prepare_v2 changing r? to sdwilsh.
Attachment #613667 - Flags: review+ → review?(sdwilsh)
Comment on attachment 613667 [details] [diff] [review] fix usage of sqlite3_prepare_v2 Lets try Andrew Sutherland
Attachment #613667 - Flags: review?(sdwilsh) → review?(bugmail)
Attachment #613667 - Flags: review?(bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/c3a3ef4f4f2e (Merged by Ed Morley)
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.