630000 - crash [@ NotificationController::Shutdown()]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

>	xul.dll!NotificationController::Shutdown()  Line 103 + 0x3 bytes	C++
 	xul.dll!nsDocAccessible::cycleCollection::Unlink(void * p)  Line 150	C++
 	xul.dll!nsCycleCollector::CollectWhite()  Line 1912 + 0x1a bytes	C++
 	xul.dll!nsCycleCollector::FinishCollection()  Line 2720 + 0x8 bytes	C++
 	xul.dll!nsCycleCollectorRunner::Collect(nsICycleCollectorListener * aListener)  Line 3355 + 0xe bytes	C++
 	xul.dll!nsCycleCollector_collect(nsICycleCollectorListener * aListener)  Line 3470 + 0x14 bytes	C++
 	xul.dll!nsJSContext::CC(nsICycleCollectorListener * aListener)  Line 3388 + 0x9 bytes	C++
 	xul.dll!nsJSContext::IntervalCC()  Line 3491 + 0x7 bytes	C++
 	xul.dll!nsJSContext::MaybeCC(int aHigherProbability)  Line 3460 + 0x5 bytes	C++
 	xul.dll!nsJSContext::CCIfUserInactive()  Line 3470 + 0x7 bytes	C++
 	xul.dll!GCTimerFired(nsITimer * aTimer, void * aClosure)  Line 3518	C++
 	xul.dll!nsTimerImpl::Fire()  Line 425 + 0xe bytes	C++
 	xul.dll!nsTimerEvent::Run()  Line 519	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait, int * result)  Line 633 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread, int mayWait)  Line 250 + 0x16 bytes	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate)  Line 134 + 0xe bytes	C++
 	xul.dll!MessageLoop::RunInternal()  Line 220	C++
 	xul.dll!MessageLoop::RunHandler()  Line 203	C++
 	xul.dll!MessageLoop::Run()  Line 177	C++
 	xul.dll!nsBaseAppShell::Run()  Line 198	C++
 	xul.dll!nsAppShell::Run()  Line 258 + 0x9 bytes	C++
 	xul.dll!nsAppStartup::Run()  Line 218 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData)  Line 3775 + 0x25 bytes	C++
 	firefox.exe!NS_internal_main(int argc, char * * argv)  Line 158 + 0x12 bytes	C++
 	firefox.exe!wmain(int argc, wchar_t * * argv)  Line 128 + 0xd bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 583 + 0x19 bytes	C
 	firefox.exe!wmainCRTStartup()  Line 403	C
 	kernel32.dll!77743677() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!77cf9d72() 	
 	ntdll.dll!77cf9d45()

Comment 1

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

I only see to crash stacks reported. This (linux) one has a comment:
https://crash-stats.mozilla.com/report/index/49d04864-def1-4028-b0a2-336552110124
"just installed QuickRestart. It's incompatible with Minefield but I have Add-on Compatibility Reporter installed."

Comment 2

[alexander :surkov (:asurkov)]

I've got it locally and I was able to reproduce it without a fix and I wasn't able to reproduce it with a fix.

Comment 3

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

Comment on attachment 508215 [details] [diff] [review]
patch

>-  tmp->Shutdown();
>+  if (tmp->mDocument)
>+    tmp->Shutdown();

OK

> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsDocAccessible, nsAccessible)
>-  tmp->mNotificationController->Shutdown();

Why is this ok?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsDocAccessible, nsAccessible)
> >-  tmp->mNotificationController->Shutdown();
> 
> Why is this ok?

If notification controller is hold by document accessible only then mNotificationController unlink (line below) will destroy the it: destructor will do shutdown. If notification controller is in middle of WillRefresh call by presshell (if that's possible) then it doesn't look right to shutdown notification controller (i.e. remove it from observer list since preshell might not expect this). When WillRefresh will be finished, presshell will release notification controller and it will be destroyed (destructor will shutdown it).

Comment 5

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

Comment on attachment 508215 [details] [diff] [review]
patch

r=me. a=me pending neil's review.

Comment 6

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

Thanks for the explanation.

Comment 7

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

Just clearing blocker request. Let's renominate if this crash rises in frequency.

Comment 8

[[e-mail]]

(In reply to comment #4)
> (destructor will shutdown it).
What about the assertion in the destructor?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (In reply to comment #4)
> > (destructor will shutdown it).
> What about the assertion in the destructor?

I'm not sure about it. Ideally we should shutdown ourself without cycle collection but that doesn't happen due to some reason. Do you think it's worth to remove it?

Comment 10

[[e-mail]]

Comment on attachment 508215 [details] [diff] [review]
patch

I hadn't spotted a normal shutdown route but I'll take your word for it.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Comment on attachment 508215 [details] [diff] [review]
> patch
> 
> I hadn't spotted a normal shutdown route but I'll take your word for it.

Normally nsAccDocManager destroys the nsDocAccessible (nsDocAccessible::Shutdown() what calls NotificationController::Shutdown) or nsOuterDocAccessible destroys nsDocAccessible if the new one appears.

Comment 12

[[e-mail]]

(In reply to comment #11)
> (nsDocAccessible::Shutdown() what calls NotificationController::Shutdown)
Ah yes, I see it now.

By the way, there seems to be a logic bug in your code to trim substrings from the end; the code seems to try to compare the trailing nulls.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)

> By the way, there seems to be a logic bug in your code to trim substrings from
> the end; the code seems to try to compare the trailing nulls.

I'm sorry but it seems I've lost context, do you talk about bug 626660 or something else?

Comment 14

[[e-mail]]

(In reply to comment #13)
> (In reply to comment #12)
> > By the way, there seems to be a logic bug in your code to trim substrings
> > from the end; the code seems to try to compare the trailing nulls.
> 
> I'm sorry but it seems I've lost context, do you talk about bug 626660 or
> something else?

Yes, I mixed the two bugs up by mistake. Sorry about that.

Comment 15

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/83e9acc504c2