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

Status: NEW

(Toolkit :: Async Tooling, task, P5)

Description

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

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.

Comment 1

[Shashank VRSN Sabniveesu]

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

Comment 2

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

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`.

Comment 3

[Shashank VRSN Sabniveesu]

Attachment: BUG1017237.patch - Add a Connection Identifier to the 'failed closing database' erro\ r message r=Yoric

Comment 4

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

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?

Comment 5

[Shashank VRSN Sabniveesu]

Attachment: BUG 1017237 - Add a Connection Identifier to the 'failed closing database' error message r=Yoric

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?

Comment 6

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

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:
```js
  let SqliteInternals = Cu.import("resource://gre/modules/Sqlite.jsm");
```

2. In `test_shutdown_clients`, before the shutdown, we should open a new database for testing:
```js
  let db = await getDummyDatabase("opened before shutdown");
```

3. Just a after `info("Making sure that the database we opened before shutdown shows up in the blockers");`, we need to get the shutdown of databases started. For this purpose, add

```js
SqliteInternals.Barriers.connections.wait();
```

4. Using `SqliteInternals`, 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 after the line we just added.

5. We need to make sure that one of the objects in this object contains the name `"opened before shutdown"`, as we haven't closed the database yet.

6. Just after that, you need to close the database
```js
  await db.close();
```

Comment 7

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

Any news, sashank?

Comment 8

[Shashank VRSN Sabniveesu]

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?

Comment 9

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

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));

Comment 10

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

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.

Comment 11

[Mike Hoye [:mhoye]]

Yoric, can you confirm that this is an ongoing need? I'd like to ask another community member to carry it over the lines.

Comment 12

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

There is still a need for tests, as described in comment 6.

Comment 13

[MMR]

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?

Comment 14

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

Yes, that would be the correct direction.

Comment 15

[MMR]

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,

Comment 16

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

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.

Comment 17

[aarushivij]

Hey, Can I work on this?
Thanks :)
Aarushi

Comment 18

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

Sure, you can!
What needs to be done is in comment 6!

Don't hesitate to ping me over https://chat.mozilla.org/#/room/#introduction:mozilla.org if you have questions. I'm Yoric over there, too!

Comment 19

[aarushivij]

Hey David,
I will start working on it, and shall ask you on introduction room if I got stuck .
Thanks :)

Comment 20

[SSH]

Hey David,
May I work on this issue?
I read the comments above, and I believe the task at hand is to create and run the tests successfully since the third parameter that was required in 'Barriers.connections.client.addBlocker' has already been added.

Comment 21

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

Turns out that there seems to be a pre-existing bug that prevents this from working.

Comment 22

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

Edited comment 6 with additional instructions. This should now be sufficient to work on this bug!

Comment 23

[aarushivij]

Hey @SSH, I was working on the issue :)
@Yoric, shall start working on it.
Thanks
Aarushi

Comment 24

[aarushivij]

Attachment: Bug 1017237 - Ensure that Sqlite's AsyncShutdown phase displays the name of opened databases in case of crash r=Yoric

Comment 25

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

Ok, I'm trying to land it on Firefox. If it doesn't break anything, it should be part of Firefox Nightly within 1 or 2 days!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1a36d34c2cc
Ensure that Sqlite's AsyncShutdown phase displays the name of opened databases in case of crash r=Yoric

Comment 27

[Andreea Pavel [:apavel]]

backed out for failing xpcshell at test_sqlite_shutdown.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=Ut-2S9iLRpOZ4f-FDG8uxw-0&resultStatus=testfailed%2Cbusted%2Cexception&revision=c1a36d34c2cc3b8d42e615a4891335123af48007

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299587262&repo=autoland&lineNumber=3231

Backout: https://hg.mozilla.org/integration/autoland/rev/9111ae1bddd7ad39daa82d7409db15515e86ae60 ***

*** test mentioned in commit is a wrong paste, sorry for that

Comment 28

[aarushivij]

Hey @Yoric
I ran ./mach xpcshell-test toolkit/modules/tests/xpcshell/test_sqlite_shutdown.js and the test passed.
https://paste.mozilla.org/AiLCPVYz

Comment 29

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

Yeah, it works locally for me, too. I'm trying to understand what's going on.

Comment 30

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

So, I think it's somehow a collision between two tests.

@aarushivij, what happens when you run ./mach xpcshell-test toolkit/modules/tests/xpcshell ?

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 32

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:aarushivij, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

Clear needinfos that are pending on inactive users.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.