Ensure that Sqlite's AsyncShutdown phase displays the name of opened databases in case of crash

NEW
Unassigned

Status

()

defect
5 years ago
a year ago

People

(Reporter: Yoric, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

At the moment, if we fail to close a database opened with Sqlite.jsm, the error looks like:

«
ERROR: At least one completion condition failed to complete within a reasonable amount of time. Causing a crash to ensure that we do not leave the user with an unresponsive process draining resources. Conditions: [{"name":"Sqlite.jsm shutdown blocker","state":{"description":"Waiting for connections to close"}}] Barrier: profile-before-change
»

The problem is that this error message does not specify the name of the offending databases.

To solve the issue, it would be sufficient to change the call to `Barriers.connections.client.addBlocker` to add a third argument returning the name of the connection.
(In reply to David Rajchenbach Teller [:Yoric] from comment #0)

Does this mean 'adding this._connectionIdentifier to this line: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm#88' as it is in the line you mentioned above ?
Flags: needinfo?(dteller)
Actually, it's here: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm#365
And the idea is to add as third argument a function that returns `this._connectionIdentifier`.
Flags: needinfo?(dteller)
Comment on attachment 8432132 [details] [diff] [review]
BUG1017237.patch -  Add a Connection Identifier to the 'failed closing database' erro\ r message r=Yoric

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

This doesn't look syntactically correct.

Can you fix it and make sure that both
  ./mach xpcshell-test toolkit/modules/tests/xpcshell/test_sqlite.js
and
  ./mach xpcshell-test toolkit/modules/tests/xpcshell/test_sqlite_shutdown.js
work correctly before submitting another patch?
Attachment #8432132 - Flags: review?(dteller)
Earlier I didn't knew about the xpcshell-tests; thanks for the heads-up..
Now this passes the tests but I am not able to check if this serves the purpose.

Yoric: How do I reproduce the bug?
Attachment #8432132 - Attachment is obsolete: true
Attachment #8432240 - Flags: review?(dteller)
Comment on attachment 8432240 [details] [diff] [review]
BUG 1017237 - Add a Connection Identifier to the 'failed closing database' error message r=Yoric

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

Good question. Before I proceed with an actual review, let's find a way for you to test it.

To test this, you'll need to customize test_sqlite_shutdown.js.

1. You'll need to access the internals of Sqlite.jsm. To do so, define `SqliteInternals` as follows:

  let SqliteInternals = Cu.import("resource://gre/modules/Sqlite.jsm");

2. Using these internals, you will be able to access an object `SqliteInternals.Barriers.connections.state` which contain all the information on the connections that have not been closed yet. You can safely use this object just before the line
    do_print("Now shutdown Sqlite.jsm synchronously");

If you take a look at the structure of this object, you should see that it has fields `state`, which contain the state you pass as third argument to `addBlocker`. You should test that both of the databases that have been opened appear in these fields.
Attachment #8432240 - Flags: review?(dteller)
Flags: needinfo?(dteller)
Assignee: nobody → shashank
Mentor: dteller
Whiteboard: [mentor=Yoric][lang=js][good first bug] → [lang=js][good first bug]
Any news, sashank?
Flags: needinfo?(shashank)
This is what I have done after implementing - "Cu.import..."

+  do_print(SqliteInternals.Barriers.connections.state);
   do_print("Now shutdown Sqlite.jsm synchronously");

Then I rebuild and run --
./mach xpcshell-test toolkit/modules/tests/xpcshell/test_sqlite_shutdown.js 

to no avail!
What is the new result I am supposed to see due to my patch?
Flags: needinfo?(shashank) → needinfo?(dteller)
Well, you need to check the contents of the object.
To get an idea of what the object contains, try 

do_print(JSON.stringify(SqliteInternals.Barriers.connections.state));
Flags: needinfo?(dteller)
We recently found out bug 1034977, and to debug it I needed to implement and land this feature in emergency. However, I have not written the tests, since you were in the process of doing so.

You should pull and merge the latest version of mozilla-central before proceeding.
Depends on: 1034977
Assignee: shashank → nobody
Yoric, can you confirm that this is an ongoing need? I'd like to ask another community member to carry it over the lines.
Flags: needinfo?(dteller)
There is still a need for tests, as described in comment 6.
Flags: needinfo?(dteller)

Comment 13

a year ago
Hello,
I think I understand what I have to do for the tests.

1. Open a database connection
2. Do not close it
3. Sqlite.jsm will crash after a warning message and then a timeout, displaying useful information
4. Verify that the information provided in 1. matches with 3.


Am I going in the correct direction?
Flags: needinfo?(dteller)
Yes, that would be the correct direction.
Flags: needinfo?(dteller)

Comment 15

a year ago
Hello,

I stumbled across this file while looking for an answer to how to crash-test:
https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/unit/test_crash_AsyncShutdown.js

This file has a test which adds a non satisfiable blocker, wait for the crash report, checks if the name matches.

I wanted to know if the test that I am supposed to write is any different from the one in the file. Or am I missing something here?

Thanks,
Flags: needinfo?(dteller)
This would definitely work, but I would like to avoid putting too many tests in that directory, as it is reserved for testing a highly critical feature (the crash reporter).

Also, I realize that I misled you when answering comment 13. What you suggest would work, but would require patching test_crash_AsyncShutdown.js as you mentioned above, so let's forget my comment 14 :)

So, I'd prefer if you could customize test_sqlite_shutdown.js, as I initially suggested on comment 6.
Flags: needinfo?(dteller)
You need to log in before you can comment on or make changes to this bug.