855331 - TransactionThreadPool doing sketchy things with mCompleteCallbacks array that can mutate

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

See https://tbpl.mozilla.org/php/getParsedLog.php?id=21144311&tree=Mozilla-Inbound

TransactionThreadPool::FinishTransaction() calls MaybeFireCallback with a DatabasesCompleteCallback& reference that comes out of mCompleteCallbacks. It then calls Run() on the callback, which could do something like call WaitForAllDatabasesToComplete which appends an element to mCompleteCallbacks. If that causes reallocation then the DatabasesCompleteCallback& on our stack is now garbage.

Somehow the log says we're jumping to HasTransactionsForDatabase (line 472), but I can't see how that is happening. Maybe we're calling a virtual Run() on one of our callback pointers that is now garbage?

Comment 1

[Al Billings [:abillings - ex-MoCo]]

When we figure this out, we'd like to know how far back it goes.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

bent says he's not working on bug 855276, which presumably means the same for this bug. Can we please get an owner? It's a frequent orange and s-s.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Kyle you lost our coin toss :) (Please find another owner if appropriate)

Comment 4

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

We suspect that this isn't actually a security issue, but we're not confident enough to open it up yet.  In particular, comment 0 is wrong, and every instance of this we've seen is a null deref.

Comment 5

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

We're pretty convinced this is a security bug again ;-)

Still haven't managed to reproduce.

Comment 6

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

I landed a patch to change from pass-by-reference to pass-by-value when calling this function and that didn't change anything, so this looks like actual heap corruption.

Comment 7

[Andrew McCreight [:mccr8]]

I'm going to assume this affects everything.  Adjust as needed.

Comment 8

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

I strongly suspect this is a regression for bug 767944.

Comment 9

[Andrew McCreight [:mccr8]]

Great, that would match up with the time this started happening.

Comment 10

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

Yes that's my primary piece of evidence ;-)

Comment 11

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

I think we should consider backing out Bug 767944.  I haven't managed to reproduce this locally :-/

Comment 12

[Jan Varga [:janv]]

Please give me a few days, I'll try to run IDB tests under valgrind.

Comment 13

[Jan Varga [:janv]]

Attachment: idb mochitests under valgrind (on mac)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jan Varga [:janv] from comment #12)
> Please give me a few days, I'll try to run IDB tests under valgrind.

Where do we stand on this? Are we considering a backout at this point? We're nearing the end of the Fx23 cycle and I don't think we want to bring this over to Aurora.

Comment 15

[Jan Varga [:janv]]

The centralized quota manager is already on Aurora, but it seems the crash doesn't happen there so often.
Could the crash somehow relate to the fact that dom/indexedDB tests now run in mochitest-3 ? They used to run in mochitest-2.

Comment 16

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

It's important to keep in mind that this patch cannot be simply backed out. It changed the directory structure of idb databases so a complete backout would effectively lose all idb data. If we decide to back this out we'll need to handle that migration as well.

Comment 17

[Jan Varga [:janv]]

I'm testing a patch here:
https://tbpl.mozilla.org/?tree=Try&rev=01b6bfd06fd7

It looks promising so far ...

Comment 18

[Jan Varga [:janv]]

16 * 40 mochitest-3 runs, no crashes in MaybeFireCallback()
Should we land it on m-c and see if it really helps ?

Comment 19

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

Definitely.

Comment 20

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/093f7b379757

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/093f7b379757

Looking very good to me! Please request Aurora uplift ASAP :)

Comment 22

[Jan Varga [:janv]]

Ben, it looks to me that we were firing a callback for some other database instance and that database instance didn't exist anymore.
Not sure how can that happen, but that's the only explanation I have for now.

Comment 23

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

We really need to understand why this happened before we move on or uplift or anything else here. Otherwise we risk simply hiding the problem or (worse) moving it somewhere else.

Comment 24

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

(In reply to ben turner [:bent] from comment #23)
> We really need to understand why this happened before we move on or uplift
> or anything else here. Otherwise we risk simply hiding the problem or
> (worse) moving it somewhere else.

So are we going to do that ...?

We need to do something on beta.

Comment 25

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

Jan, are you looking into this?

Comment 26

[Jan Varga [:janv]]

(In reply to ben turner [:bent] from comment #25)
> Jan, are you looking into this?

well, I can take a look ...
do you think it could be something like the thing described in comment 22 ?

Comment 27

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

I haven't been able to come up with any explanation of why this patch seems to work. Since you came up with it I'm hoping you might have a better idea ;)

Comment 28

[Alex Keybl [:akeybl]]

There hasn't been enough traction here to get into Beta 22, and comment 27 doesn't make me feel good about getting this into our final two betas.

Comment 29

[Daniel Veditz [:dveditz]]

This is a "kungFuDeathGrip" type patch. The worst risk of uplifting it would seem to be memory leaks (but those should show up as unexpected leaks in tbpl and don't seem to be).

Comment 30

[Daniel Veditz [:dveditz]]

I don't know how to appeal the "wontfix" because there's no attachment on which to request approval :-( but that is what the previous comment was attempting to be.

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to ben turner [:bent] from comment #23)
> We really need to understand why this happened before we move on or uplift
> or anything else here. Otherwise we risk simply hiding the problem or
> (worse) moving it somewhere else.

Whether we uplift to Fx22 or not the above is true since the patch is on its way to release regardless.

Comment 32

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

Can we back this out of trunk before we branch? There's no reason to think that this patch is what we actually want.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Can we just back this out from beta after the merge rather than inflicting the nearly once per push crash back on the rest of the tree? This was one of our most frequent oranges when we were hitting it and the rate at which this and associated bugs has moved along doesn't leave me brimming with confidence that the pain of a trunk backout will be short-lived.

Comment 34

[Jan Varga [:janv]]

Ok, Ben and I suspect that those crashes we used to have before the "noone knows why it works" patch could be caused by the issue described in the bug 878703 and that the centralized manager somehow made the crashes to happen more often.
I'm going to backout the "noone knows why it works" patch on the try server this weekend. I need to retrigger the tests like hundreds of times to be sure that the crashes don't happen anymore, that's why I'll do it on the weekend, when the load is lower.

Comment 35

[Jan Varga [:janv]]

no, bug 878703 seems to be unrelated
when I backed out the hacky patch, crashes reappeared:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24186335&tree=Try

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Tagging this since we seem to be shipping this in two weeks.