741821 - Don't access a null sql stmt

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: don't crash

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

Comment 1

[Vladan Djeric (:vladan)]

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.

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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.

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=485381749e29

Comment 4

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/485381749e29

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: fix usage of sqlite3_prepare_v2

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 7

[Vladan Djeric (:vladan)]

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

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 613667 [details] [diff] [review]
fix usage of sqlite3_prepare_v2

changing r? to sdwilsh.

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

ping

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 613667 [details] [diff] [review]
fix usage of sqlite3_prepare_v2

Lets try Andrew Sutherland

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c3a3ef4f4f2e

Comment 12

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/c3a3ef4f4f2e

(Merged by Ed Morley)