Closed Bug 914005 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Storage, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: jruderman, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

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.
Assignee: Jan.Varga → nobody
Component: SQL → Storage
Product: Core → Toolkit
QA Contact: owen.marshall+bmo
I guess I'll add this behind a mozStorage LOG and file individual bugs.
Assignee: nobody → dteller
Depends on: 914070
(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.
Marco, do we want to remove the warnings for FF26?
Flags: needinfo?(mak77)
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.
Attachment #801529 - Flags: review?(mak77)
Flags: needinfo?(dteller)
Flags: needinfo?(mak77)
Is this what you had in mind?
Attachment #801529 - Attachment is obsolete: true
Flags: needinfo?(dteller)
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(...)
I seem to remember that we generally prefer |#if 0|/|#endif| to commenting out code. Is that ok with you?
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.
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?
both, we will just remove the "//"
In that case, here we go.

Still with #if 0, because I believe that commenting out code is generally bad style.
Attachment #805911 - Attachment is obsolete: true
Attachment #806579 - Flags: review?(mak77)
Attachment #806579 - Flags: review?(mak77) → review+
Same one, without the typo.
Sorry about that.
Attachment #806579 - Attachment is obsolete: true
Attachment #807181 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2d5083a119eb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
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.
Attachment #807181 - Flags: approval-mozilla-aurora?
Attachment #807181 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I believe that this shouldn't be tracked anymore, shouldn't it?
You need to log in before you can comment on or make changes to this bug.