Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond) Reporter
	Description • 9 years ago

STR:

- Nightly build (tested on windows, I believe it reproduces on OSX as well)
- e10s enabled
- Start Firefox
- In the "Home Page", make sure cursor is blinking in the "Search" box.
- Close the window.

You end up with leaks like this in the parent process:

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
     |                                      | Per-Inst   Leaked|   Total      Rem|
   0 |TOTAL                                 |       22    11276| 5102876       70|
   1 |APZCTreeManager                       |      472      472|       1        1|
   2 |APZEventState                         |       88       88|       1        1|
   6 |ActiveElementManager                  |       48       48|       1        1|
 114 |CompositorChild                       |      800      800|       1        1|
 116 |CompositorParent                      |     1064     2128|       2        2|
 118 |CompositorVsyncDispatcher             |       64       64|       1        1|
 119 |CompositorVsyncScheduler              |      232      232|       1        1|
 121 |CondVar                               |       40      360|     261        9|
 317 |InputQueue                            |       32       32|       1        1|
 358 |Mutex                                 |       32      320|     792       10|
 392 |PCompositorChild                      |      696      696|       1        1|
 393 |PCompositorParent                     |      696      696|       2        1|
 483 |RefCountedMonitor                     |       80       80|      10        1|
 485 |RefCountedTask                        |       16       64|      36        4|
 555 |TSFStaticSink                         |       96       96|       1        1|
 556 |TSFTextStore                          |      248      248|       3        1|
 558 |TaskThrottler                         |      208      416|       2        2|
 606 |WeakReference<MessageListener>        |       16       32|     230        2|
 665 |ZoomConstraints                       |       12       12|       6        1|
 706 |ipc::MessageChannel                   |      384      768|      18        2|
 757 |nsBaseWidget                          |      320      640|       5        2|
 981 |nsIdleService                         |      104      104|       1        1|
 982 |nsIdleServiceDaily                    |      104      104|       1        1|
 983 |nsIdleServiceWin                      |      104      104|       1        1|
1157 |nsStringBuffer                        |        8        8|  127362        1|
1198 |nsTArray_base                         |        8      104|  950884       13|
1206 |nsThread                              |      256      256|      42        1|
1212 |nsTimerImpl                           |      160      320|    3992        2|
1243 |nsWeakReference                       |       32       64|     391        2|
1248 |nsWindow                              |      960     1920|       5        2|


If you click out of the search box (so there's no blinking cursor), and close the window, no leaks.

Something like this might even be causing some random/intermittent leaks on tbpl as well.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 1 • 9 years ago

On Linux (debug) I see a different set of leaks, but still leaking the world.

We seem to be leaking an nsWindow (and a lot of what it owns) via a reference held in IMEStateManager::sFocusedIMEWidget. That would explain a correlation between focus and a leak.

The call stack where the sFocusedIMEWidget is set:
#0  0x00007fc905d2f1e8 in mozilla::StaticRefPtr<nsIWidget>::AssignAssumingAddRef(nsIWidget*) (this=0x7fc90cb642c8 <mozilla::IMEStateManager::sFocusedIMEWidget>, aNewPtr=0x7fc8e1a3e400) at ../../dist/include/mozilla/StaticPtr.h:138
#1  0x00007fc905d2d53d in mozilla::StaticRefPtr<nsIWidget>::AssignWithAddref(nsIWidget*) (this=0x7fc90cb642c8 <mozilla::IMEStateManager::sFocusedIMEWidget>, aNewPtr=0x7fc8e1a3e400) at ../../dist/include/mozilla/StaticPtr.h:131
#2  0x00007fc905d29e19 in mozilla::StaticRefPtr<nsIWidget>::operator=(nsIWidget*) (this=0x7fc90cb642c8 <mozilla::IMEStateManager::sFocusedIMEWidget>, aRhs=0x7fc8e1a3e400) at ../../dist/include/mozilla/StaticPtr.h:104
#3  0x00007fc905d17274 in mozilla::IMEStateManager::NotifyIME(mozilla::widget::IMENotification const&, nsIWidget*, bool) (aNotification=..., aWidget=0x7fc8e1a3e400, aOriginIsRemote=true)
    at /home/roc/mozilla-inbound/dom/events/IMEStateManager.cpp:1347
#4  0x00007fc906553d89 in mozilla::dom::TabParent::RecvNotifyIMEFocus(mozilla::ContentCache const&, mozilla::widget::IMENotification const&, nsIMEUpdatePreference*) (this=0x7fc8ebd2c800, aContentCache=..., aIMENotification=..., aPreference=0x7ffd8e3c5757) at /home/roc/mozilla-inbound/dom/ipc/TabParent.cpp:1956
#5  0x00007fc903ff4148 in mozilla::dom::PBrowserParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (this=0x7fc8ebd2c800, msg__=..., reply__=@0x7ffd8e3c74f0: 0x0)
    at /home/roc/mozilla-inbound/obj-ff-debug/ipc/ipdl/PBrowserParent.cpp:3600
#6  0x00007fc9041042d4 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (this=0x7fc8dd661000, msg__=..., reply__=@0x7ffd8e3c74f0: 0x0)
    at /home/roc/mozilla-inbound/obj-ff-debug/ipc/ipdl/PContentParent.cpp:6312
#7  0x00007fc903b3eaf8 in mozilla::ipc::MessageChannel::DispatchSyncMessage(IPC::Message const&, IPC::Message*&) (this=0x7fc8dd661068, aMsg=..., aReply=@0x7ffd8e3c74f0: 0x0) at /home/roc/mozilla-inbound/ipc/glue/MessageChannel.cpp:1356
#8  0x00007fc903b3e819 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) (this=0x7fc8dd661068, aMsg=...) at /home/roc/mozilla-inbound/ipc/glue/MessageChannel.cpp:1301
#9  0x00007fc903b3e663 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (this=0x7fc8dd661068)
    at /home/roc/mozilla-inbound/ipc/glue/MessageChannel.cpp:1276
#10 0x00007fc903b56141 in details::CallMethod<, mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::IndexSequence<>, mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tuple<>&) (obj=0x7fc8dd661068, method=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7fc903b3e4f8 <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, arg=...)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/task.h:28
#11 0x00007fc903b55dc7 in DispatchTupleToMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tuple<>&) (obj=0x7fc8dd661068, method=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7fc903b3e4f8 <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, arg=...) at /home/roc/mozilla-inbound/ipc/chromium/src/base/task.h:46
#12 0x00007fc903b5591e in RunnableMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)(), mozilla::Tuple<> >::Run() (this=0x7fc8d7b8f700) at /home/roc/mozilla-inbound/ipc/chromium/src/base/task.h:307
#13 0x00007fc903b47e31 in mozilla::ipc::MessageChannel::RefCountedTask::Run() (this=0x7fc8dac2d9d0)
    at ../../dist/include/mozilla/ipc/MessageChannel.h:472
#14 0x00007fc903b4801e in mozilla::ipc::MessageChannel::DequeueTask::Run() (this=0x7fc891109a60)
    at ../../dist/include/mozilla/ipc/MessageChannel.h:489
#15 0x00007fc903a950a7 in MessageLoop::RunTask(Task*) (this=0x7fc90d150540, task=0x7fc891109a60)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:364
#16 0x00007fc903a95121 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (this=0x7fc90d150540, pending_task=...) at /home/roc/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:372
#17 0x00007fc903a95573 in MessageLoop::DoWork() (this=0x7fc90d150540)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:459
#18 0x00007fc903b42654 in mozilla::ipc::DoWorkRunnable::Run() (this=0x7fc90d1deb50)
    at /home/roc/mozilla-inbound/ipc/glue/MessagePump.cpp:220
#19 0x00007fc9035ad2c6 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7fc9191aff00, aMayWait=true, aResult=0x7ffd8e3c797f) at /home/roc/mozilla-inbound/xpcom/threads/nsThread.cpp:964
#20 0x00007fc9036154e6 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fc9191aff00, aMayWait=true)
    at /home/roc/mozilla-inbound/xpcom/glue/nsThreadUtils.cpp:297
#21 0x00007fc903b420d4 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7fc90d1e3dc0, aDelegate=0x7fc90d150540) at /home/roc/mozilla-inbound/ipc/glue/MessagePump.cpp:127
#22 0x00007fc903a94b3f in MessageLoop::RunInternal() (this=0x7fc90d150540)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:234
#23 0x00007fc903a94ad2 in MessageLoop::RunHandler() (this=0x7fc90d150540)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:227
#24 0x00007fc903a94a61 in MessageLoop::Run() (this=0x7fc90d150540)
    at /home/roc/mozilla-inbound/ipc/chromium/src/base/message_loop.cc:201
#25 0x00007fc9068935c8 in nsBaseAppShell::Run() (this=0x7fc8f0617040)
    at /home/roc/mozilla-inbound/widget/nsBaseAppShell.cpp:156
#26 0x00007fc9076fe3eb in nsAppStartup::Run() (this=0x7fc8f0619060)
    at /home/roc/mozilla-inbound/toolkit/components/startup/nsAppStartup.cpp:281
#27 0x00007fc907798ad0 in XREMain::XRE_mainRun() (this=0x7ffd8e3c7f60)
    at /home/roc/mozilla-inbound/toolkit/xre/nsAppRunner.cpp:4298
#28 0x00007fc907798e5f in XREMain::XRE_main(int, char**, nsXREAppData const*) (this=0x7ffd8e3c7f60, argc=3, argv=0x7ffd8e3c94d8, aAppData=0x7ffd8e3c9160) at /home/roc/mozilla-inbound/toolkit/xre/nsAppRunner.cpp:4391
#29 0x00007fc90779912d in XRE_main(int, char**, nsXREAppData const*, uint32_t) (argc=3, argv=0x7ffd8e3c94d8, aAppData=0x7ffd8e3c9160, aFlags=0) at /home/roc/mozilla-inbound/toolkit/xre/nsAppRunner.cpp:4493
#30 0x0000000000405672 in do_main(int, char**, nsIFile*) (argc=3, argv=0x7ffd8e3c94d8, xreDirectory=0x7fc919161a80)
    at /home/roc/mozilla-inbound/browser/app/nsBrowserApp.cpp:212
#31 0x00000000004059e3 in main(int, char**) (argc=3, argv=0x7ffd8e3c94d8)
    at /home/roc/mozilla-inbound/browser/app/nsBrowserApp.cpp:352

I don't see any code that tries to clear sFocusedIMEWidget on window destruction or shutdown. This looks like the problem.

Masayuki, is there anything that is supposed to clear sFocusedIMEWidget when a window is closed or the browser shuts down?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 2 • 9 years ago

sFocusedIMEWidget should be cleared when NOTIFY_IME_OF_BLUR is being sent to the nsWindow.
http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEStateManager.cpp?rev=4c9308a551ba&mark=1381-1381#1349

This is called from TabParent::RecvNotifyIMEFocus() when a remote process's editor has focus.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=67da9ced67f1&mark=1949-1953,1956-1956#1945

But if the widget cached by TabParent is already nullptr, this fails.

http://mxr.mozilla.org/mozilla-central/source/widget/PuppetWidget.cpp?rev=374b5bb34c32&mark=693-694,717-718#687

Or if the tab child for the puppet widget is already nullptr, this fails too.

So, if these points are already cleared by shutdown process *before* the editor in child process loses focus, this leak is possible.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 3 • 9 years ago

Thanks.

PuppetWidget::NotifyIMEOfFocusChange does send a NOTIFY_IME_OF_BLUR event before we shut it down. And it's received by TabParent::RecvNotifyIMEFocus --- but GetWidget returned null so we returned early.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 4 • 9 years ago

TabParent::GetWidget returns null because mFrameElement's document's presshell has already been torn down.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 5 • 9 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> TabParent::GetWidget returns null because mFrameElement's document's
> presshell has already been torn down.

Sounds like when TagParent's widget becomes nullptr, IMEStateManager should be notified of that.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 9 years ago

Bug 1222092. Don't let sFocusedIMEWidget keep an nsIWidget alive during shutdown. r=masayuki

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 7 • 9 years ago

Comment on attachment 8686987 [details]
MozReview Request: Bug 1222092. Don't let sFocusedIMEWidget keep an nsIWidget alive during shutdown. r=masayuki

https://reviewboard.mozilla.org/r/25109/#review22631

I don't like nsBaseWidget referes IMEStateManager directly, tough, this is simple and enough.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 8 • 9 years ago

Oh, wait. Did you test it on Windows? Until NOTIFY_IME_OF_BLUR is received by TSFTextStore, the widget is grabbed by it.
http://mxr.mozilla.org/mozilla-central/source/widget/windows/TSFTextStore.h#312

Maybe, other platforms have similar problem.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 9 • 9 years ago

Okay, other platforms looks like safe since native IME event handlers called like OnDestroyWidget() and release the instance.

But only on Windows, we don't do so:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/WinIMEHandler.cpp#332

TSFTextStore and IMMHandler need to have such method and releasing the reference to the destroyed nsIWidget. Especially, TSFTextStore has strong reference. So, the nsWindow for Windows is never released until NOTIFY_IME_OF_BLUR is sent.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 10 • 9 years ago

Filed bug 1224454, I'll fix it when I can work on it.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 11 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/50657cb9988a3618b7db979b9f00e40a3c790a17
Bug 1222092. Don't let sFocusedIMEWidget keep an nsIWidget alive during shutdown. r=masayuki

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/50657cb9988a

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 13 • 9 years ago

I fixed bug 1224454 too. So, I guess that this bug has gone. But I'm not sure because when I shutdown my debug build, it's crashed by an assertion (bug 1224454 comment 5).

So, I'd like you to verify the fix.