914005 - Many storage warnings on shutdown: "SQL statement ... should have been finalized before garbage-collection"

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Jesse Ruderman]

Attachment: SQL spew from a single shutdown

Bug 911109 added a new SQL warning, and it fires 11 times on every shutdown.  This kind of spew makes it hard to debug anything else in the browser.

The new warnings look like:

WARNING: SQL statement (12bbecf0) should have been finalizedbefore garbage-collection. For more details on this statement, setNSPR_LOG_MESSAGES=mozStorage:5 .: file ../../../storage/src/mozStorageStatement.cpp, line 412

If you can't fix this quickly, please put the warning behind a flag so it is only shown to developers working on this part of the browser.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I guess I'll add this behind a mozStorage LOG and file individual bugs.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Moving the warnings to mozStorage logging

Comment 3

[Marco Bonardo [:mak]]

(In reply to Jesse Ruderman from comment #0)
> If you can't fix this quickly, please put the warning behind a flag so it is
> only shown to developers working on this part of the browser.

This is unfortunately not possible, since storage is used by many different components and parts of the browser.

We must keep this as a warning, otherwise nobody would ever notice nor fix the problem. We all know this is what would happen.
But, we can take a temporary solution, like the suggested one, until the issue is investigated and fixed. That solution should be backed out once the fix lands, so that we can detect future regressions.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Marco, do we want to remove the warnings for FF26?

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 801529 [details] [diff] [review]
Moving the warnings to mozStorage logging

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

::: storage/src/mozStorageStatement.cpp
@@ -409,5 @@
>      // developers correlate with the more complete debug message triggered
>      // by AsyncClose().
>      //
> -    NS_WARNING(msg);
> -    ::PR_smprintf_free(msg);

let's do both (the warning and the PR_LOG), but for now comment out the warning and put a comment on top of it stating the warning is temporarily disabled while we investigate bug 914070. in bug 914070 add a comment note to re-enable the warning.

Yes, it's probably worth to uplift to Firefox 26.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Moving the warnings to mozStorage logging, v2

Is this what you had in mind?

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 805911 [details] [diff] [review]
Moving the warnings to mozStorage logging, v2

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

::: storage/src/mozStorageStatement.cpp
@@ +418,1 @@
>      NS_WARNING(msg);

well, I was thinking more of a very simple //NS_WARNING(...)

Comment 8

[David Teller [:Yoric] - still alive but not very active]

I seem to remember that we generally prefer |#if 0|/|#endif| to commenting out code. Is that ok with you?

Comment 9

[Marco Bonardo [:mak]]

I never heard of that specific, it may exist, I don't know. I suppose it's up to the module owner, in this case it's also temporary code that is soon going away, so I don't care about its code style.
r=me on whatever you prefer, I just want both the PR_LOG and the warning, with the latter temporarily disabled.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Just to clarify: once bug 914070 is fixed, should we have only the warning or both the warning and the log entry with the same text?

Comment 11

[Marco Bonardo [:mak]]

both, we will just remove the "//"

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Moving the warnings to mozStorage logging, v3

In that case, here we go.

Still with #if 0, because I believe that commenting out code is generally bad style.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ad57b713ac46

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Moving the warnings to mozStorage logging, v4

Same one, without the typo.
Sorry about that.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Original push backed out for bustage.
https://hg.mozilla.org/integration/fx-team/rev/58f289272bde

Fixed patch landed.
https://hg.mozilla.org/integration/fx-team/rev/2d5083a119eb

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2d5083a119eb

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 807181 [details] [diff] [review]
Moving the warnings to mozStorage logging, v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 911109.
User impact if declined: Console spam upon shutdown.
Testing completed (on m-c, etc.): This has been on m-c for a few days.
Risk to taking this patch (and alternatives if risky): This patch just changes logging. No risk I can imagine.
String or IDL/UUID changes made by this patch: None.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8de120cd08dc

Comment 19

[David Teller [:Yoric] - still alive but not very active]

I believe that this shouldn't be tracked anymore, shouldn't it?