575348 - e10s: Fennectrolysis crashes in cycle collection after geolocation test

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Josh Matthews [:jdm]]

Attachment: Testcase

After browsing to the attached testcase, f10s crashes about 5 seconds later.  No geolocation prompt appears, either.

Comment 1

[Josh Matthews [:jdm]]

Backtrace:


(gdb) bt
#0  0x017ce257 in CallQueryInterface<nsISupports, nsXPCOMCycleCollectionParticipant> (aSource=0xae4f0ec0, aDestination=0xbfffa5f8) at ../../../dist/include/nsISupportsUtils.h:205
#1  0x01f65c29 in WrapperIsNotMainThreadOnly (wrapper=0xb42d6600) at /home/t_mattjo/src/firefox/mobilebase/js/src/xpconnect/src/nsXPConnect.cpp:673
#2  0x01f65d72 in nsXPConnect::Traverse (this=0xb7d15b30, p=0xb3ab6c40, cb=...) at /home/t_mattjo/src/firefox/mobilebase/js/src/xpconnect/src/nsXPConnect.cpp:710
#3  0x026742bf in GCGraphBuilder::Traverse (this=0xbfffa7f4, aPtrInfo=0xad7076f4) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsCycleCollector.cpp:1484
#4  0x0267489a in nsCycleCollector::MarkRoots (this=0xb7d3b800, builder=...) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsCycleCollector.cpp:1706
#5  0x026751ea in nsCycleCollector::BeginCollection (this=0xb7d3b800) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsCycleCollector.cpp:2645
#6  0x0267556e in nsCycleCollector_beginCollection () at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsCycleCollector.cpp:3232
#7  0x01f65745 in XPCCycleCollectGCCallback (cx=0xb399e800, status=JSGC_MARK_END) at /home/t_mattjo/src/firefox/mobilebase/js/src/xpconnect/src/nsXPConnect.cpp:361
#8  0x0310747d in GC (cx=0xb399e800) at /home/t_mattjo/src/firefox/mobilebase/js/src/jsgc.cpp:2797
#9  0x03107cbc in GCUntilDone (cx=0xb399e800, gckind=GC_NORMAL) at /home/t_mattjo/src/firefox/mobilebase/js/src/jsgc.cpp:3156
#10 0x03107df4 in js_GC (cx=0xb399e800, gckind=GC_NORMAL) at /home/t_mattjo/src/firefox/mobilebase/js/src/jsgc.cpp:3207
#11 0x030a99bd in JS_GC (cx=0xb399e800) at /home/t_mattjo/src/firefox/mobilebase/js/src/jsapi.cpp:2317
#12 0x01f658b6 in nsXPConnect::Collect (this=0xb7d15b30) at /home/t_mattjo/src/firefox/mobilebase/js/src/xpconnect/src/nsXPConnect.cpp:448
#13 0x0267504d in nsCycleCollector::Collect (this=0xb7d3b800, aTryCollections=1) at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsCycleCollector.cpp:2523
#14 0x02675500 in nsCycleCollector_collect () at /home/t_mattjo/src/firefox/mobilebase/xpcom/base/nsCycleCollector.cpp:3220
#15 0x01ac0999 in nsJSContext::CC () at /home/t_mattjo/src/firefox/mobilebase/dom/base/nsJSEnvironment.cpp:3589
#16 0x01ac0b8d in nsJSContext::IntervalCC () at /home/t_mattjo/src/firefox/mobilebase/dom/base/nsJSEnvironment.cpp:3677
#17 0x01ab79bd in nsUserActivityObserver::Observe (this=0xb3829340, aSubject=0x0, aTopic=0x2ae5c78 "user-interaction-inactive", aData=0x0) at /home/t_mattjo/src/firefox/mobilebase/dom/base/nsJSEnvironment.cpp:270
#18 0x0260ffa3 in nsObserverList::NotifyObservers (this=0xb3841d58, aSubject=0x0, aTopic=0x2ae5c78 "user-interaction-inactive", someData=0x0) at /home/t_mattjo/src/firefox/mobilebase/xpcom/ds/nsObserverList.cpp:130
#19 0x02611941 in nsObserverService::NotifyObservers (this=0xb7dcfa90, aSubject=0x0, aTopic=0x2ae5c78 "user-interaction-inactive", someData=0x0) at /home/t_mattjo/src/firefox/mobilebase/xpcom/ds/nsObserverService.cpp:182
#20 0x018e32b0 in nsUITimerCallback::Notify (this=0xb38469a0, aTimer=0xb39aefc0) at /home/t_mattjo/src/firefox/mobilebase/content/events/src/nsEventStateManager.cpp:282
#21 0x02664e8f in nsTimerImpl::Fire (this=0xb39aefc0) at /home/t_mattjo/src/firefox/mobilebase/xpcom/threads/nsTimerImpl.cpp:430
#22 0x026650ab in nsTimerEvent::Run (this=0xad86a5c0) at /home/t_mattjo/src/firefox/mobilebase/xpcom/threads/nsTimerImpl.cpp:519
#23 0x0265e45c in nsThread::ProcessNextEvent (this=0xb7d61f60, mayWait=1, result=0xbfffeb6c) at /home/t_mattjo/src/firefox/mobilebase/xpcom/threads/nsThread.cpp:547
#24 0x025f8b7d in NS_ProcessNextEvent_P (thread=0xb7d61f60, mayWait=1) at nsThreadUtils.cpp:250
#25 0x024dfa13 in mozilla::ipc::MessagePump::Run (this=0xb7dafb80, aDelegate=0xb7d216e0) at /home/t_mattjo/src/firefox/mobilebase/ipc/glue/MessagePump.cpp:142
#26 0x026c478d in MessageLoop::RunInternal (this=0xb7d216e0) at /home/t_mattjo/src/firefox/mobilebase/ipc/chromium/src/base/message_loop.cc:219
#27 0x026c470d in MessageLoop::RunHandler (this=0xb7d216e0) at /home/t_mattjo/src/firefox/mobilebase/ipc/chromium/src/base/message_loop.cc:202
#28 0x026c46b1 in MessageLoop::Run (this=0xb7d216e0) at /home/t_mattjo/src/firefox/mobilebase/ipc/chromium/src/base/message_loop.cc:176
#29 0x0239b906 in nsBaseAppShell::Run (this=0xb3917a10) at /home/t_mattjo/src/firefox/mobilebase/widget/src/xpwidgets/nsBaseAppShell.cpp:175
#30 0x0214caad in nsAppStartup::Run (this=0xb395cb20) at /home/t_mattjo/src/firefox/mobilebase/toolkit/components/startup/src/nsAppStartup.cpp:192
#31 0x012acd11 in XRE_main (argc=1, argv=0xbffff234, aAppData=0xb7d0e380) at /home/t_mattjo/src/firefox/mobilebase/toolkit/xre/nsAppRunner.cpp:3627
#32 0x0804969c in main (argc=1, argv=0xbffff234) at /home/t_mattjo/src/firefox/mobilebase/mobile/app/nsBrowserApp.cpp:146
(gdb)

Comment 2

[Josh Matthews [:jdm]]

I'm fairly certain I found the problem.  In GeolocationRequestParent::~GeolocationRequestParent, there's a call to |delete mProxy| because it's a raw pointer.  However, gdb shows me this:


(gdb) p *mProxy
$6 = (nsGeolocationRequestProxy) {
  <nsIGeolocationRequest> = {
    <nsISupports> = {
      _vptr.nsISupports = 0x2ae8988
    }, <No data fields>}, 
  members of nsGeolocationRequestProxy: 
  mRefCnt = {
    mValue = 2
  }, 
  _mOwningThread = {
    mThread = 0x804c548
  }, 
  mParent = 0x8ecebc8
}

Obviously somebody tries to garbage collect this later and it's already deleted by then.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Patch

This fix is nice and small.  The proxy object is refcounted, so it's very wrong and bad to be doing manual memory management with it.  This patch makes it nsCOMPtr, and the object is cleaned up a short time after the rest of the OOP machinery (verified in gdb).  I ran some quick tests of the geolocation API and they all appear to be in working order now.

Comment 4

[Josh Matthews [:jdm]]

Also the |//namespace dom| whitespace change is in there because there were about 300 trailing whitespace characters previously, and it was wrapping several lines in emacs.

Comment 7

[Josh Matthews [:jdm]]

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