1143885 - crash in mozilla::dom::indexedDB::IDBTransaction::SendAbort(tag_nsresult)

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[u279076]

This bug was filed from the Socorro interface and is 
report bp-52ca63e7-5fd1-4183-bf09-7a0e42150309.
=============================================================
0 	xul.dll 	mozilla::dom::indexedDB::IDBTransaction::SendAbort(tag_nsresult) 	dom/indexeddb/IDBTransaction.cpp
1 	xul.dll 	mozilla::dom::indexedDB::IDBTransaction::OnRequestFinished() 	dom/indexeddb/IDBTransaction.cpp
2 	xul.dll 	mozilla::dom::indexedDB::BackgroundRequestChild::MaybeFinishTransactionEarly() 	dom/indexeddb/ActorsChild.cpp
3 	xul.dll 	mozilla::dom::indexedDB::BackgroundRequestChild::ActorDestroy(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason) 	dom/indexeddb/ActorsChild.cpp
4 	xul.dll 	mozilla::dom::indexedDB::PIndexedDBPermissionRequestChild::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason) 	obj-firefox/ipc/ipdl/PIndexedDBPermissionRequestChild.cpp
5 	xul.dll 	mozilla::dom::indexedDB::PBackgroundIDBTransactionChild::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason) 	obj-firefox/ipc/ipdl/PBackgroundIDBTransactionChild.cpp
6 	xul.dll 	mozilla::dom::indexedDB::PBackgroundIDBDatabaseChild::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason) 	obj-firefox/ipc/ipdl/PBackgroundIDBDatabaseChild.cpp
7 	xul.dll 	mozilla::dom::indexedDB::PBackgroundIDBFactoryChild::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason) 	obj-firefox/ipc/ipdl/PBackgroundIDBFactoryChild.cpp
8 	xul.dll 	mozilla::ipc::PBackgroundChild::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason) 	obj-firefox/ipc/ipdl/PBackgroundChild.cpp
9 	xul.dll 	mozilla::ipc::PBackgroundChild::OnChannelClose() 	obj-firefox/ipc/ipdl/PBackgroundChild.cpp
10 	xul.dll 	mozilla::ipc::MessageChannel::NotifyChannelClosed() 	ipc/glue/MessageChannel.cpp
11 	xul.dll 	`anonymous namespace'::ChildImpl::ThreadLocalDestructor(void*) 	ipc/glue/BackgroundImpl.cpp
12 	nss3.dll 	PR_SetThreadPrivate 	nsprpub/pr/src/threads/prtpd.c
13 	nss3.dll 	PR_GetThreadPrivate 	nsprpub/pr/src/threads/prtpd.c
14 	xul.dll 	`anonymous namespace'::ChildImpl::Shutdown() 	ipc/glue/BackgroundImpl.cpp
15 	xul.dll 	`anonymous namespace'::ParentImpl::ShutdownObserver::Observe(nsISupports*, char const*, wchar_t const*) 	ipc/glue/BackgroundImpl.cpp
16 	xul.dll 	nsObserverList::NotifyObservers(nsISupports*, char const*, wchar_t const*) 	xpcom/ds/nsObserverList.cpp
17 	xul.dll 	nsObserverService::NotifyObservers(nsISupports*, char const*, wchar_t const*) 	xpcom/ds/nsObserverService.cpp
18 	xul.dll 	ScopedXPCOMStartup::~ScopedXPCOMStartup() 	toolkit/xre/nsAppRunner.cpp
19 	xul.dll 	ScopedXPCOMStartup::`scalar deleting destructor'(unsigned int) 	
20 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
21 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
22 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
23 	firefox.exe 	NS_internal_main(int, char**) 	browser/app/nsBrowserApp.cpp
24 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
25 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255
26 	kernel32.dll 	BaseThreadInitThunk 	
27 	ntdll.dll 	__RtlUserThreadStart 	
28 	ntdll.dll 	_RtlUserThreadStart 	
=============================================================
More reports: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Adom%3A%3AindexedDB%3A%3AIDBTransaction%3A%3ASendAbort%28tag_nsresult%29

Volume: 963 reports in the last week
Platform: All reports are on Windows with 61% on Windows 7.
Product: All reports are currently against Firefox 37.
Graphics: Most cards reported are Intel with some AMD in the mix.

Top URLs:
44 about:blank
21 http://6e2bd3848d2b73.se/?placement=400864&redirect
8 http://yxo.warmportrait.com/sd/dw32.html?...
7 https://www.facebook.com/
5 http://www.linkeyproject.com/app/?tag=newtab
5 chrome://quick_start/content/index.html
5 http://powerbundle.systweak.com/asp/afteruninstall/?...
5 http://yxo.warmportrait.com/sd/dw32.html?...
4 http://www.stamplive.com/apu.php?...
3 http://cdn.vm-corporate.com/...

There are no useful comments in all of the reports unfortunately.

Comment 1

[u279076]

[Tracking Requested - why for this release]: based on reports this could be a Firefox 37 regression.

Comment 2

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

Attachment: Patch, v1

Oops. Dumb.

Comment 3

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

Comment on attachment 8578412 [details] [diff] [review]
Patch, v1

Crap, this doesn't work correctly in some cases. New patch in a sec.

Comment 4

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

Attachment: Patch, v2

This refactors the interplay between BackgroundChildRequest and IDBTransaction::OnNewRequest/OnRequestFinished. Now it more closely resembles the cursor code, and it makes it easier to put the one important check for this case where it needs to go.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

From the crash reports it looks like this is all 37. Tracking+

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

We're at the end of the 37 cycle. As there is a fix, Ben and I agree that we should get this landed on m-c and review whether we take the fix in the RC that is scheduled to spin on Monday.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c55ce051cd

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57c55ce051cd

Comment 9

[u279076]

Thanks for the quick turnaround on this, Ben. 

Note to Release Management, since we do not have steps to reproduce this crash this will need to be "verified" based purely on crashstats. I'm flagging this qe-verify+ and assigning to myself as a reminder to check in a couple days. Unfortunately, I'm not sure that's going to be a fast enough turnaround time to get this into 37 RC.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> Note to Release Management, since we do not have steps to reproduce this
> crash this will need to be "verified" based purely on crashstats. I'm
> flagging this qe-verify+ and assigning to myself as a reminder to check in a
> couple days. Unfortunately, I'm not sure that's going to be a fast enough
> turnaround time to get this into 37 RC.

Understood. We'll have to make a call on Monday with the data that we have available.

Ben - Can you please request uplift so that we're good to go on Monday if we choose to proceed?

Comment 11

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

Comment on attachment 8578823 [details] [diff] [review]
Patch, v2

Approval Request Comment
[Feature/regressing bug #]: Bug 1092311 probably
[User impact if declined]: Occasional crashes on shutdown if IDB requests are in progress and we race just right
[Describe test coverage new/current, TreeHerder]: Testing this really hard, I'm not sure we can catch this race in a reliable way on automation.
[Risks and why]: The patch is pretty straightforward, anything broken would have broken many tests I think.
[String/UUID change made/needed]: None

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Ben, why do you request for uplift to aurora as it is marked as unaffected?

Comment 13

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Ben, why do you request for uplift to aurora as it is marked as unaffected?

I think that's just a mistake. The code that probably caused this bug is in 37, so 38 is most likely affected also.

Comment 14

[u279076]

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #13)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> > Ben, why do you request for uplift to aurora as it is marked as unaffected?
> 
> I think that's just a mistake. The code that probably caused this bug is in
> 37, so 38 is most likely affected also.

I'm inclined to side with Ben's assessment. I set Firefox 38 as unaffected because we weren't seeing reports against Firefox 38 builds. All that proves is that people are not reporting these crashes in other product versions, not that the code causing this isn't in Firefox 38. 

Sorry if that caused confusion.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8578823 [details] [diff] [review]
Patch, v2

No worries Anthony :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7b82c496e1e3

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Looking at the crash-stats date, even going back as far as 28 days I only see reports of this crash on 37. No new crashes with the fix is good but I don't know that they would show up here either. I expect that the only way that we're going to prove that this fix is good is to take it in 37 Beta. The overall stability of 37 still needs some work so I'm inclined to take this fix and the associated risk at this point. However, this being a shutdown crash, it won't impact the user of the browser until the user shuts down so is not as critical as some of the other crashes.

Kairo - Do you see this as necessary for uplift to 37 to help with the crash rate?

Ben - What do you think about uplifting this fix now that it's been on m-c and m-a for a few days?

Comment 18

[Robert Kaiser]

This is 0.3% on 37.0b7 so far, so eliminating this signature won't make a big dent - unless it improves stability for other cases than this specific signature as well.

Comment 19

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

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> Ben - What do you think about uplifting this fix now that it's been on m-c
> and m-a for a few days?

I haven't seen any fallout so I think it's pretty safe to uplift. But as kairo points out there's not a whole lot of benefit to be had...

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Given the limited benefit of uplifting and that we're at the end of the 37 cycle, let's ship this fix in 38. Thank you both for your feedback today.

Comment 21

[u279076]

Removing qe-verify+ since we have no data for Firefox 38/39 for this signature and 37 wont be fixed.