1256049 - crash in mozilla::dom::indexedDB::`anonymous namespace''::DatabaseConnection::Close

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-9fa004dd-a325-4820-ad19-3f1832160312.
=============================================================

About 200 reported crashes in the past 28 days, all on Windows.
Looks like a regression in v46.

Stack:
mozilla::dom::indexedDB::`anonymous namespace'::DatabaseConnection::Close()
mozilla::dom::indexedDB::`anonymous namespace'::ConnectionPool::CloseConnectionRunnable::Run()
nsThread::ProcessNextEvent(bool, bool*)
NS_ProcessNextEvent(nsIThread*, bool)
mozilla::dom::indexedDB::`anonymous namespace'::ConnectionPool::ThreadRunnable::Run()
nsThread::ProcessNextEvent(bool, bool*)
NS_ProcessNextEvent(nsIThread*, bool)
mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)
MessageLoop::RunHandler()
MessageLoop::Run()
nsThread::ThreadFunc(void*)
PR_NativeRunThread

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

This is about #40 on the beta 46 topcrash list. 47 and 48 are affected too.  
I'd like to find an owner to look into this bug but I'm not sure we need to track it for 46. We could still  uplift it if there is a good fix though.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Sure does look like a regression in 46 though. I added the keyword.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Ugh I feel too guilty not tracking it. Maybe we can find a regression range.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I made this assertion fatal in bug 1195149 so we could see it.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Diagnostics

This is evil.

Comment 6

[Jan Varga [:janv]]

Comment on attachment 8731494 [details] [diff] [review]
Diagnostics

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

Good old debugging methods.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6d3398c6e5

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35a5691320bc

Backed out for bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=24056333&repo=mozilla-inbound

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfa5a8fd015

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bfa5a8fd015

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

3 crashes have happened so far with that diagnostic.  All of them crashed at 0x5, pointing to ConnectionPool::CloseDatabaseWhenIdleInternal.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: More diagnostics

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0c5b558917

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b0c5b558917

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, our premature close is getting triggered from Database::CloseInternal, which calls ConnectionPool::CloseDatabaseWhenIdle, which seems to believe we are already idle.  Either we're not, or we get a late transaction constructor.  Either way, we have assertions, which we should enable.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 1: Revert previous patches

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Part 2: Enable two more assertions.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b94bf7ac393d
https://hg.mozilla.org/integration/mozilla-inbound/rev/097e6e67c4c6

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b94bf7ac393d
https://hg.mozilla.org/mozilla-central/rev/097e6e67c4c6

Comment 20

[Andrew McCreight [:mccr8]]

I don't see any crashes on Nightly after this landed, though maybe it is too early to say for sure. Is this ready to be backported?

Comment 21

[Jan Varga [:janv]]

(In reply to Andrew McCreight [:mccr8] from comment #20)
> I don't see any crashes on Nightly after this landed, though maybe it is too
> early to say for sure. Is this ready to be backported?

These patches are not real fixes, just helping to identify the issue.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, all I did was chase this signature elsewhere.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0ea82bf6e9

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out because the assertion occurs so often that many mochitests fail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/76134d6ad636

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well, that answers my question.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This should do it, I think.

Comment 27

[Jan Varga [:janv]]

Comment on attachment 8737985 [details] [diff] [review]
Patch

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

Looks good to me. I tested it locally on my mac, no failures.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d08f6e50f306

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d08f6e50f306

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ni? myself to look at crash stats next week.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8737985 [details] [diff] [review]
Patch

This is looking good so far. I'd like to take it to Aurora.

Approval Request Comment
[Feature/regressing bug #]: Possibly Bug 858680
[User impact if declined]: More crashes
[Describe test coverage new/current, TreeHerder]: IDB is pretty well tested in automation.
[Risks and why]: Low risk. This is a pretty simple patch and is unlikely to mess anything up.
[String/UUID change made/needed]: N/A

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03afa06c1272

Comment 37

[Ben Turner (not reading bugmail, use the needinfo flag!)]

\o/

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03afa06c1272

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Kyle, I was trying to check whether the crash signatures have appeared since this landed in Nightly before taking this to Aurora 47. If I am not wrong, this fix should have been in 4/13 Nightly build. Right? I was not able to find the first signature but the 2nd crash sig is still occurring on 4/13 Nightly build: https://crash-stats.mozilla.com/report/index/89a9a6c1-7cc7-43c3-85ee-941cf2160413. Am I reading the data wrong? Please let me know.

Comment 40

[Nicholas Nethercote [inactive]]

(In reply to Ritu Kothari (:ritu) from comment #39)
> Hi Kyle, I was trying to check whether the crash signatures have appeared
> since this landed in Nightly before taking this to Aurora 47. If I am not
> wrong, this fix should have been in 4/13 Nightly build. Right? I was not
> able to find the first signature but the 2nd crash sig is still occurring on
> 4/13 Nightly build:
> https://crash-stats.mozilla.com/report/index/89a9a6c1-7cc7-43c3-85ee-
> 941cf2160413. Am I reading the data wrong? Please let me know.

The "[@ PLDHashTable::Search | nsInterfaceHashtable<T>::Get ]" signature is a very generic one that can match lots of crashes. I've filed bug 1263781 to get an extra frame in that signature.

The crash report you cited appears to have a different cause. It doesn't involve DB connection closing, AFAICT.

Comment 41

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That crash looks related.  We did fix an actual problem here, and I'd like to get that into Aurora, even if there might be more things that still need to be fixed.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Kyle what do you think about taking this for beta 11? I would like to bring down our crash rate and this looks promising. If you think it's too risky then please wontfix for 46 though.

Comment 43

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think the downside risk here is limited to "doesn't fix all the crashes", so I'd be comfortable taking it for 46 if desired.

Comment 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

For posterity, this is number 35 and 0.32% of browser process crashes on 46.b.

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixing some of the crashes sounds great to me.  Kyle can you request beta uplift?

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8737985 [details] [diff] [review]
Patch

Crash fixes, looks good to me, and ritu says OK for aurora. Please uplift to aurora and beta, hoping to make it into beta 11.

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And to be precise, the changeset to be uplifted is in comment 30.  None of the other things that landed should be uplifted.

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8298e05ae1bd

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a094ec154fc8