Closed Bug 630193 Opened 13 years ago Closed 13 years ago

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

Categories

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

x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: scoobidiver, Assigned: mayhemer)

References

Details

(Keywords: crash, regression, topcrash, Whiteboard: [hardblocker][has patch])

Crash Data

Attachments

(1 file, 1 obsolete file)

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
blocking2.0: --- → ?
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.
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.
Interestingly enough, every single report I've looked at so far looks to be from some European version of Windows.
Please see also bug 630242.  My tip is that a window is used after CleanUp() has been called.
#6 crash on Beta10.
The stack above nsGlobalWindow::Observe is probably completely bogus. Honza seems to have a clue what's going on, so he gets the bug ;-)
blocking2.0: ? → final+
Component: General → DOM
Keywords: topcrash
QA Contact: general → general
If this is #6 crasher on b10, this sure should be a hardblocker.
I think bsmedberg meant to assign this to Honza, and having him take a closer look makes sense to me.
Whiteboard: [hardblocker]
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.
Assignee: nobody → honzab.moz
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?
Status: NEW → ASSIGNED
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.)
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.
Attached patch v1 (obsolete) — Splinter Review
Patch according to comment 12.

Who's gonna review it?
Attachment #509151 - Flags: review?
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 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.
Attachment #509151 - Flags: review? → review-
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?
(In reply to comment #16)
> and in general seems 'empty', i.e. all members of nsPIDOMWindow are null.

Except:
- mIsInnerWindow
- mWindowID
- mHasNotifiedGlobalCreated
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).
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.
Attached patch v2Splinter Review
- mPendingStorageEventsObsolete turned to nsAutoPtr
- observer removal moved to CleanUp() method
Attachment #509151 - Attachment is obsolete: true
Attachment #509778 - Flags: review?(khuey)
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.
Attachment #509778 - Flags: review?(khuey)
Attachment #509778 - Flags: review?(jst)
Attachment #509778 - Flags: feedback+
Comment on attachment 509778 [details] [diff] [review]
v2

Looks good.
Attachment #509778 - Flags: review?(jst) → review+
Whiteboard: [hardblocker] → [hardblocker][has patch]
http://hg.mozilla.org/mozilla-central/rev/d907bea5f40f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
Summary: Crash [@ PL_DHashTableOperate | nsTHashtable<nsUniCharEntry>::PutEntry(unsigned short const*) ] → nsAutoPtr<`anonymous namespace''::DatabaseInfoHash> > >::PutEntry(unsigned int const&)] Crash [@ PL_DHashTableOperate | nsTHashtable<nsUniCharEntry>::PutEntry(unsigned short const*)][@ PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsUint32HashKey
Depends on: 654707
Crash Signature: [@ PL_DHashTableOperate | nsTHashtable<nsUniCharEntry>::PutEntry(unsigned short const*)] [@ PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsUint32HashKey, nsAutoPtr<`anonymous namespace''::DatabaseInfoHash> > >::PutEntry(unsigned int const&)]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.