Closed Bug 743650 Opened 8 years ago Closed 8 years ago

Don't use nsPrintfCString

Categories

(Toolkit :: Storage, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: espindola, Assigned: espindola)

References

Details

(Keywords: regression)

Attachments

(1 file)

nsPrintfCString silently truncates, so in this code pageSizeStmt is null ever since we prefixed the string with MOZ_STORAGE_UNIQUIFY_QUERY_STR.
https://tbpl.mozilla.org/?tree=Try&rev=002b402ef7c2
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #613277 - Flags: review?(nfroyd)
Comment on attachment 613277 [details] [diff] [review]
Don't use nsPrintfCString

Review of attachment 613277 [details] [diff] [review]:
-----------------------------------------------------------------

r+'ing since this is essentially the same patch mak approved in bug 743056 comment 15.

Please remove the #include of nsPrintfCString.h prior to commit.
Attachment #613277 - Flags: review?(nfroyd) → review+
Component: Places → Storage
QA Contact: places → storage
should this be backported? Did we actually break vacuum in Firefox 12/13 or am I wrong? If so this is worth to make at least FF13.

fwiw, you should also have removed #include "nsPrintfCString.h" from the top.
Blocks: 712427
Keywords: regression
Target Milestone: --- → mozilla14
(In reply to Marco Bonardo [:mak] from comment #4)
> should this be backported? Did we actually break vacuum in Firefox 12/13 or
> am I wrong? If so this is worth to make at least FF13.

I am sure it broke the pragma, I am not sure about the impact that had.

> fwiw, you should also have removed #include "nsPrintfCString.h" from the top.

I did in the final push. Nathan noticed it too.
(In reply to Marco Bonardo [:mak] from comment #4)
> should this be backported? Did we actually break vacuum in Firefox 12/13 or
> am I wrong? If so this is worth to make at least FF13.

It looks like it's been broken for a long time...just "PRAGMA page_size" (16 characters) is already longer than the 15 character default nsPrintfCString uses.  Or am I being dense?
> It looks like it's been broken for a long time...just "PRAGMA page_size" (16
> characters) is already longer than the 15 character default nsPrintfCString
> uses.  Or am I being dense?

I think this bug just made it more interesting. With the comment at the start sqlite3_prepare_v2 returns OK but sets the statement to NULL.
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #5)
> I did in the final push. Nathan noticed it too.

nice! sorry, didn't read everything, just had a quick look at the attached patch.

(In reply to Nathan Froyd (:froydnj) from comment #6)
> It looks like it's been broken for a long time...just "PRAGMA page_size" (16
> characters) is already longer than the 15 character default nsPrintfCString
> uses.  Or am I being dense?

You are damn right.  And likely even if that query is broken vacuum should happen correctly, so no need to backport. The net effect is that we likely don'try to set the page_size, but it's a minor deal cause the only vacuum manager user atm is Places, and using wal means its page_size can't be fixed.
At least this should fix the "not an error" cryptic sqlite error we sometimes saw here.
https://hg.mozilla.org/mozilla-central/rev/147324800246
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.