630193 - Crash [@ PL_DHashTableOperate | nsTHashtable<nsUniCharEntry>::PutEntry(unsigned short const*)][@ PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsUint32HashKey, nsAutoPtr<`anonymous namespace''::DatabaseInfoHash> > >::PutEntry(unsigned int const&)]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Scoobidiver (away)]

It is a new crash signature in 4.0b10 (does not exist in 4.0b9).
It is #13 top crasher in 4.0b10 over the last week.

Signature	PL_DHashTableOperate | nsTHashtable<nsUniCharEntry>::PutEntry(unsigned short const*)
UUID	2b8d85be-96a6-4052-947e-6cc022110131
Time 	2011-01-31 03:48:51.438510
Uptime	108
Last Crash	3452235 seconds (5.7 weeks) before submission
Install Age	55369 seconds (15.4 hours) since version was first installed.
Product	Firefox
Version	4.0b10
Build ID	20110121161358
Branch	2.0
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 13
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x9
App Notes 	AdapterVendorID: 8086, AdapterDeviceID: 2a02, AdapterDriverVersion: 8.15.10.1930

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	PL_DHashTableOperate 	obj-firefox/xpcom/build/pldhash.c:615
1 	xul.dll 	nsTHashtable<nsUniCharEntry>::PutEntry 	obj-firefox/dist/include/nsTHashtable.h:188
2 	xul.dll 	nsBaseHashtable<nsStringHashKey,int,int>::Put 	obj-firefox/dist/include/nsBaseHashtable.h:163
3 	xul.dll 	nsGlobalWindow::Observe 	
4 		@0x15f 	
5 	mozcrt19.dll 	arena_dalloc_small 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:4153
6 	xul.dll 	nsTArray_base<nsTArrayDefaultAllocator>::ShiftData 	obj-firefox/dist/include/nsTArray-inl.h:169
7 	xul.dll 	nsObserverList::FillObserverArray 	xpcom/ds/nsObserverList.cpp:119
8 	xul.dll 	PL_DHashTableOperate 	obj-firefox/xpcom/build/pldhash.c:625
9 	xul.dll 	xul.dll@0xbbda57 	
10 		@0x2e0076 	
11 	xul.dll 	nsAString_internal::Assign 	xpcom/string/src/nsTSubstring.cpp:391
12 	xul.dll 	nsAString_internal::Finalize 	xpcom/string/src/nsTSubstring.cpp:188
13 	xul.dll 	DOMStorageImpl::SetValue 	dom/src/storage/nsDOMStorage.cpp:1235
14 	xul.dll 	nsDOMStorage::SetItem 	dom/src/storage/nsDOMStorage.cpp:1633
15 	xul.dll 	nsStorageSH::SetProperty 	dom/base/nsDOMClassInfo.cpp:10324
16 	xul.dll 	XPC_WN_Helper_SetProperty 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:987
17 	mozjs.dll 	js::Shape::set 	js/src/jsscopeinlines.h:275
18 	mozjs.dll 	js_NativeSet 	js/src/jsobj.cpp:5199
19 	mozjs.dll 	js_SetPropertyHelper 	js/src/jsobj.cpp:5674
20 	mozjs.dll 	js_SetProperty 	js/src/jsobj.cpp:5685
21 	mozjs.dll 	js::mjit::stubs::SetElem<0> 	js/src/methodjit/StubCalls.cpp:591

More reports at:
https://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=PL_DHashTableOperate%20|%20nsTHashtable%3CnsUniCharEntry%3E%3A%3APutEntry%28unsigned%20short%20const*%29

Comment 1

[Josh Matthews [:jdm]]

That is a very strange stack.  The bit up to DOMStorageImpl::SetValue makes sense, but I don't really understand where the rest of it comes from.

Comment 2

[Josh Matthews [:jdm]]

Ok, the hashtable frames could be coming from nsGlobalWindow::Observe, ie.

>      if (!mPendingStorageEventsObsolete) {
>        mPendingStorageEventsObsolete = new nsDataHashtable<nsStringHashKey, PRBool>;
>        NS_ENSURE_TRUE(mPendingStorageEventsObsolete, NS_ERROR_OUT_OF_MEMORY);
>
>        rv = mPendingStorageEventsObsolete->Init();
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>
>      mPendingStorageEventsObsolete->Put(domain, PR_TRUE);

but it's still weird how the frames between the DOMStorage value setting and the observer notification are screwed up/nonexistent in some reports.

Comment 3

[Josh Matthews [:jdm]]

Interestingly enough, every single report I've looked at so far looks to be from some European version of Windows.

Comment 4

[Honza Bambas (:mayhemer)]

Please see also bug 630242.  My tip is that a window is used after CleanUp() has been called.

Comment 5

[Sheila Mooney]

#6 crash on Beta10.

Comment 6

[Benjamin Smedberg]

The stack above nsGlobalWindow::Observe is probably completely bogus. Honza seems to have a clue what's going on, so he gets the bug ;-)

Comment 7

[Olli Pettay [:smaug]]

If this is #6 crasher on b10, this sure should be a hardblocker.

Comment 8

[Johnny Stenback (:jst)]

I think bsmedberg meant to assign this to Honza, and having him take a closer look makes sense to me.

Comment 9

[Honza Bambas (:mayhemer)]

My theory is that it is related to bug 630242.  We are sending DOM storage events to an nsGlobalWindow instance that is both:
- CleanUp()'ed
- Freeze()'ed

Going to check if I really am able to reproduce the first condition with test case from bug 630242.  If so, then fixing it will probably fix this bug as well.

Comment 10

[Honza Bambas (:mayhemer)]

This could be a dupe of some nsGlobalWindow leak.  We remove observers in the destructor.  Could this be that fix for bug 622361 is not sufficient?

Comment 11

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

Well, the fix for Bug 622361 doesn't really mess with old style storage events.  (I don't think moving the delete call should have changed anything.)

Comment 12

[Honza Bambas (:mayhemer)]

OK, I think I can say it is what I describe in comment 9, with additional conditions, enumerating them rather all again:

- nsGlobalWindow must receive an event from globalStorage change while it is frozen (to create mPendingStorageEventsObsolete hash table)
- it's Freeze()'ed and it's CleanUp()'ed (mPendingStorageEventsObsolete deleted but not nullified) ; doesn't matter in what order
- receive globalStorage change event again

I just checked I was able to receive globalStorage event in a window that has been CleanUp()'ed.


To have a simple fix for this bug, I'd return early from ::Observe() method if mCleanedUp flag is set.  Also nullifying mPendingStorageEventsObsolete would be good.

Someone more experienced with lifetime of nsGlobalWindow instance should confirm it is OK.

Kyle, I don't think anymore it could be related to your changes, this bug is there IMO forever, just raised because of some other change that makes the four conditions appear more often.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1

Patch according to comment 12.

Who's gonna review it?

Comment 14

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

Should probably switch mPendingStorageEventsObsolete to be an nsAutoPtr.

Maybe nsGlobalWindow should just stop observing these events when it is cleaned up.  Bailing on *all* notifications when the window has been cleaned up seems risky both now and in the future.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 509151 [details] [diff] [review]
v1

I think this would fix this, and I'd be ok with this change for now, but I think we might as well do what Kyle suggests and make this more future proof.

Comment 16

[Honza Bambas (:mayhemer)]

I agree.

I am just testing the patch as suggested by Kyle, and have following issues:

I'm getting a different scenario now.  I can still see an nsGlobalWindow instance, that has no outer window, is NOT CleanUp()'ed - the flag is not set - and in general seems 'empty', i.e. all members of nsPIDOMWindow are null.

However, in the log (DOMLeak:5) I can see that the window has been created and assigned an outer window in the constructor, and so far not destroyed.

As I'm looking to mxr, I cannot find a place where mOuterWindow should be released (nullified).

Could the cycle collector free it up?  Though, there are other inner windows referring the same outer window that are still alive.

Could it be some saved state reversal?

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to comment #16)
> and in general seems 'empty', i.e. all members of nsPIDOMWindow are null.

Except:
- mIsInnerWindow
- mWindowID
- mHasNotifiedGlobalCreated

Comment 18

[Johnny Stenback (:jst)]

When the cycle collector unlinks an inner window mOuterWindow is nulled out (at http://hg.mozilla.org/mozilla-central/annotate/bc9d7938cdc7/dom/base/nsGlobalWindow.cpp#l1390).

Comment 19

[Honza Bambas (:mayhemer)]

Yes, that's it, just adding a simple log ("DOMWINDOW %p UNLINK", static_cast<nsGlobalWindow*>(tmp)) to NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsGlobalWindow) section shows it is the case.

I'll move comments 16 to 19 to bug 630242 where those belong.  It seems unrelated to this bug now, just confused me.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v2

- mPendingStorageEventsObsolete turned to nsAutoPtr
- observer removal moved to CleanUp() method

Comment 21

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

Comment on attachment 509778 [details] [diff] [review]
v2

I think this is ok, but it really should be reviewed by jst or another dom peer.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 509778 [details] [diff] [review]
v2

Looks good.

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/d907bea5f40f