UnregisterWindow holds window list lock during callbacks, can make addons cause deadlocks

RESOLVED FIXED in mozilla36

Status

()

Core
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Garrett Holmstrom, Assigned: Brian Marshall, Mentored)

Tracking

34 Branch
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Full disclosure:  I am in way over my head here; most of the info in this bug is more or less quoted/paraphrased from an IRC discussion with jdm.

Steps to reproduce:

On occasion, closing a window causes the main thread to deadlock and I have to kill the program.  I can't seem to reproduce it with a new profile and no addons, but the ~20 addons I am using with my existing profile seem to make it fairly easy to reproduce.  I can list them or spend a bunch more time working on a reproducer with a new profile if you would like, but judging by the discussion in IRC, the cause of the issue is clear.  I am happy to test patches.

Actual results:

(Again, from IRC)
When nsWindowMediator::UnregisterWindow [1] runs after I close a window it grabs a window enumerator lock and then runs notifyCloseWindow callbacks.  Some code uses document.getElementById there and tries to load XBL bindings, triggering a content policy thing that uses the window mediator, which ends up digging through the window enumerator again and deadlocking everything.

[1] http://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsWindowMediator.cpp#108

Expected results:

UnregisterWindow should collect the windows in a vector, release the lock, and iterate over that vector when invoking listeners.

Here's the backtrace:

#0  0x000000311ac0e51d in __lll_lock_wait () from /lib64/libpthread.so.0
#1  0x000000311ac0a151 in _L_lock_1022 () from /lib64/libpthread.so.0
#2  0x000000311ac0a0f2 in pthread_mutex_lock () from /lib64/libpthread.so.0
#3  0x00007f3d6baddf59 in PR_Lock () from /home/gholms/firefox/libnspr4.so
#4  0x00007f3d6821cafb in nsWindowMediator::GetEnumerator(char16_t const*, nsISimpleEnumerator**) () from /home/gholms/firefox/libxul.so
#5  0x00007f3d67071830 in NS_InvokeByIndex () from /home/gholms/firefox/libxul.so
#6  0x00007f3d66dc129b in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) () from /home/gholms/firefox/libxul.so
#7  0x00007f3d66dc5f63 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) () from /home/gholms/firefox/libxul.so
#8  0x00007f3d66fbb542 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#9  0x00007f3d66fbc502 in Interpret(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#10 0x00007f3d66fc84f3 in js::RunScript(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#11 0x00007f3d66fbb619 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#12 0x00007f3d66fc8afa in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#13 0x00007f3d66f9e080 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#14 0x00007f3d66fb3e86 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#15 0x00007f3d66f9e620 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) () from /home/gholms/firefox/libxul.so
#16 0x00007f3d66f9e76a in js::proxy_Call(JSContext*, unsigned int, JS::Value*) () from /home/gholms/firefox/libxul.so
#17 0x00007f3d66fbb6a1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#18 0x00007f3d66fbc502 in Interpret(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#19 0x00007f3d66fc84f3 in js::RunScript(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#20 0x00007f3d66fbb619 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#21 0x00007f3d66fc8afa in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#22 0x00007f3d66f9e080 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#23 0x00007f3d66fb3e86 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#24 0x00007f3d66f9e620 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) () from /home/gholms/firefox/libxul.so
#25 0x00007f3d66f9e76a in js::proxy_Call(JSContext*, unsigned int, JS::Value*) () from /home/gholms/firefox/libxul.so
#26 0x00007f3d66fbb6a1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#27 0x00007f3d66fbc502 in Interpret(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#28 0x00007f3d66fc84f3 in js::RunScript(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#29 0x00007f3d66fbb619 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#30 0x00007f3d66fc8a3b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#31 0x00007f3d66f9e080 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#32 0x00007f3d66fb3e86 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#33 0x00007f3d66f9e620 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) () from /home/gholms/firefox/libxul.so
#34 0x00007f3d66f9e76a in js::proxy_Call(JSContext*, unsigned int, JS::Value*) () from /home/gholms/firefox/libxul.so
#35 0x00007f3d66fbb6a1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#36 0x00007f3d66fbc502 in Interpret(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#37 0x00007f3d66fc84f3 in js::RunScript(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#38 0x00007f3d66fbb619 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#39 0x00007f3d66fc8a3b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#40 0x00007f3d66f9e080 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#41 0x00007f3d66fb3e86 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#42 0x00007f3d66f9e620 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) () from /home/gholms/firefox/libxul.so
#43 0x00007f3d66f9e76a in js::proxy_Call(JSContext*, unsigned int, JS::Value*) () from /home/gholms/firefox/libxul.so
#44 0x00007f3d66fbb6a1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#45 0x00007f3d66fbc502 in Interpret(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#46 0x00007f3d66fc84f3 in js::RunScript(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#47 0x00007f3d66fbb619 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#48 0x00007f3d66fc8a3b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#49 0x00007f3d66f64d90 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#50 0x00007f3d66dbb58f in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) () from /home/gholms/firefox/libxul.so
#51 0x00007f3d67072489 in PrepareAndDispatch () from /home/gholms/firefox/libxul.so
#52 0x00007f3d670719b3 in SharedStub () from /home/gholms/firefox/libxul.so
#53 0x00007f3d67cc99af in nsContentPolicy::CheckPolicy(tag_nsresult (nsIContentPolicy::*)(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) () from /home/gholms/firefox/libxul.so
#54 0x00007f3d67cc9b7f in nsContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) () from /home/gholms/firefox/libxul.so
#55 0x00007f3d674c3b76 in NS_CheckContentLoadPolicy(unsigned int, nsIURI*, nsIPrincipal*, nsISupports*, nsACString_internal const&, nsISupports*, short*, nsIContentPolicy*, nsIScriptSecurityManager*) ()
   from /home/gholms/firefox/libxul.so
#56 0x00007f3d67c8d191 in nsContentUtils::CheckSecurityBeforeLoad(nsIURI*, nsIPrincipal*, unsigned int, bool, unsigned int, nsISupports*, nsCString const&, nsISupports*) () from /home/gholms/firefox/libxul.so
#57 0x00007f3d67bee3b9 in nsXBLService::LoadBindingDocumentInfo(nsIContent*, nsIDocument*, nsIURI*, nsIPrincipal*, bool, nsXBLDocumentInfo**) () from /home/gholms/firefox/libxul.so
#58 0x00007f3d67bee9e2 in nsXBLService::GetBinding(nsIContent*, nsIURI*, bool, nsIPrincipal*, bool*, nsXBLBinding**, nsTArray<nsIURI*>&) () from /home/gholms/firefox/libxul.so
#59 0x00007f3d67bef089 in nsXBLService::GetBinding(nsIContent*, nsIURI*, bool, nsIPrincipal*, bool*, nsXBLBinding**) () from /home/gholms/firefox/libxul.so
#60 0x00007f3d67bef234 in nsXBLService::LoadBindings(nsIContent*, nsIURI*, nsIPrincipal*, nsXBLBinding**, bool*) () from /home/gholms/firefox/libxul.so
#61 0x00007f3d66e13c36 in mozilla::dom::Element::WrapObject(JSContext*) () from /home/gholms/firefox/libxul.so
---Type <return> to continue, or q <return> to quit---
#62 0x00007f3d66dfad44 in mozilla::dom::DocumentBinding::getElementById(JSContext*, JS::Handle<JSObject*>, nsIDocument*, JSJitMethodCallArgs const&) () from /home/gholms/firefox/libxul.so
#63 0x00007f3d66dfdb8e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) () from /home/gholms/firefox/libxul.so
#64 0x00007f3d66fbb542 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#65 0x00007f3d66fc8a3b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#66 0x00007f3d66f9e080 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#67 0x00007f3d66fb3e86 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const () from /home/gholms/firefox/libxul.so
#68 0x00007f3d66f9e620 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) () from /home/gholms/firefox/libxul.so
#69 0x00007f3d66f9e76a in js::proxy_Call(JSContext*, unsigned int, JS::Value*) () from /home/gholms/firefox/libxul.so
#70 0x00007f3d66fbb6a1 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#71 0x00007f3d66fbc502 in Interpret(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#72 0x00007f3d66fc84f3 in js::RunScript(JSContext*, js::RunState&) () from /home/gholms/firefox/libxul.so
#73 0x00007f3d66fbb619 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () from /home/gholms/firefox/libxul.so
#74 0x00007f3d66fc8a3b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#75 0x00007f3d66f64d90 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) () from /home/gholms/firefox/libxul.so
#76 0x00007f3d66dbb58f in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) () from /home/gholms/firefox/libxul.so
#77 0x00007f3d67072489 in PrepareAndDispatch () from /home/gholms/firefox/libxul.so
#78 0x00007f3d670719b3 in SharedStub () from /home/gholms/firefox/libxul.so
#79 0x00007f3d6821bb51 in notifyCloseWindow(nsIWindowMediatorListener*, void*) () from /home/gholms/firefox/libxul.so
#80 0x00007f3d66d9686b in nsCOMArray_base::EnumerateForwards(bool (*)(void*, void*), void*) const () from /home/gholms/firefox/libxul.so
#81 0x00007f3d6821be16 in nsWindowMediator::UnregisterWindow(nsWindowInfo*) () from /home/gholms/firefox/libxul.so
#82 0x00007f3d6821cbb2 in nsWindowMediator::UnregisterWindow(nsIXULWindow*) () from /home/gholms/firefox/libxul.so
#83 0x00007f3d682156f4 in nsAppShellService::UnregisterTopLevelWindow(nsIXULWindow*) () from /home/gholms/firefox/libxul.so
#84 0x00007f3d682220a3 in nsXULWindow::Destroy() () from /home/gholms/firefox/libxul.so
#85 0x00007f3d6821b603 in nsWebShellWindow::Destroy() () from /home/gholms/firefox/libxul.so
#86 0x00007f3d6821b4b3 in nsWebShellWindow::RequestWindowClose(nsIWidget*) () from /home/gholms/firefox/libxul.so
#87 0x00007f3d67c49616 in delete_event_cb(_GtkWidget*, _GdkEventAny*) () from /home/gholms/firefox/libxul.so
#88 0x000000312e9484ec in _gtk_marshal_BOOLEAN__BOXED () from /lib64/libgtk-x11-2.0.so.0
#89 0x000000311d810298 in g_closure_invoke () from /lib64/libgobject-2.0.so.0
#90 0x000000311d82235d in signal_emit_unlocked_R () from /lib64/libgobject-2.0.so.0
#91 0x000000311d829ddd in g_signal_emit_valist () from /lib64/libgobject-2.0.so.0
#92 0x000000311d82a3af in g_signal_emit () from /lib64/libgobject-2.0.so.0
#93 0x000000312ea777a4 in gtk_widget_event_internal () from /lib64/libgtk-x11-2.0.so.0
#94 0x000000312e946d93 in gtk_main_do_event () from /lib64/libgtk-x11-2.0.so.0
#95 0x000000312e46040c in gdk_event_dispatch () from /lib64/libgdk-x11-2.0.so.0
#96 0x000000311d0492a6 in g_main_context_dispatch () from /lib64/libglib-2.0.so.0
#97 0x000000311d049628 in g_main_context_iterate.isra () from /lib64/libglib-2.0.so.0
#98 0x000000311d0496dc in g_main_context_iteration () from /lib64/libglib-2.0.so.0
#99 0x00007f3d66e0b8ef in nsAppShell::ProcessNextNativeEvent(bool) () from /home/gholms/firefox/libxul.so
#100 0x00007f3d66e0acdd in nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool, unsigned int) () from /home/gholms/firefox/libxul.so
#101 0x00007f3d66d935fe in nsThread::ProcessNextEvent(bool, bool*) () from /home/gholms/firefox/libxul.so
#102 0x00007f3d66d99b19 in NS_ProcessNextEvent(nsIThread*, bool) () from /home/gholms/firefox/libxul.so
#103 0x00007f3d66dac903 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) () from /home/gholms/firefox/libxul.so
#104 0x00007f3d67243a43 in MessageLoop::Run() () from /home/gholms/firefox/libxul.so
#105 0x00007f3d67c33655 in nsBaseAppShell::Run() () from /home/gholms/firefox/libxul.so
#106 0x00007f3d68332851 in nsAppStartup::Run() () from /home/gholms/firefox/libxul.so
#107 0x00007f3d683616bf in XREMain::XRE_mainRun() () from /home/gholms/firefox/libxul.so
#108 0x00007f3d68363d85 in XREMain::XRE_main(int, char**, nsXREAppData const*) () from /home/gholms/firefox/libxul.so
#109 0x00007f3d68364009 in XRE_main () from /home/gholms/firefox/libxul.so
#110 0x0000000000406ffb in do_main(int, char**, nsIFile*) ()
#111 0x00000000004046c4 in main ()

Updated

3 years ago
Component: Untriaged → General
Product: Firefox → Core

Comment 1

3 years ago
This shouldn't be a difficult bug to solve. We just need to ensure that we don't hold on to the lock from nsWindowMediator::UnregisterWindow while performing callbacks.
Mentor: josh@joshmatthews.net
Whiteboard: [good first bug][lang=c++]
(Assignee)

Comment 2

3 years ago
Hi, I'd like to try to fix this bug.

It looks like the lock should be released before notifyCloseWindow is called, currently on line 130:
  mListeners.EnumerateForwards(notifyCloseWindow, &winData);

(In reply to Garrett Holmstrom from comment #0)
> UnregisterWindow should collect the windows in a vector, release the lock,
> and iterate over that vector when invoking listeners.

Does collecting the windows mean collecting nsIWindowMediatorListener objects? Should mListeners be copied to a local std::vector<nsIWindowMediatorListener*> variable?

Also, should MutexAutoUnlock be used in this case? Thanks for your help.
(Assignee)

Comment 3

3 years ago
Created attachment 8499951 [details] [diff] [review]
Release window mediator lock to avoid deadlock

An nsTArray<nsIWindowMediatorListener*> holds the listeners. Is a raw pointer okay, or should something like an nsCOMPtr be used?

Two for-loops are used to iterate through the arrays since that seems more readable, but a while-loop was used above them in the same function. Should they all use the same kind of loop?

I don't know how to reproduce the bug, so I can't verify the fix.
Attachment #8499951 - Flags: review?(josh)

Comment 4

3 years ago
Comment on attachment 8499951 [details] [diff] [review]
Release window mediator lock to avoid deadlock

Review of attachment 8499951 [details] [diff] [review]:
-----------------------------------------------------------------

Everything about this mutex confuses me; I've posted https://groups.google.com/forum/#!topic/mozilla.dev.platform/pDfl3arc3Dw asking if it's actually necessary. Until then, this patch looks good (except that you're right, nsTArray<nsCOMPtr<nsIWindowMediatorListener>> would be safer).

Tossing this review to Neil, according to https://wiki.mozilla.org/Modules/Core#XPApps .
Attachment #8499951 - Flags: review?(neil)
Attachment #8499951 - Flags: review?(josh)
Attachment #8499951 - Flags: feedback+

Comment 5

3 years ago
(In reply to Josh Matthews from comment #4)
> nsTArray<nsCOMPtr<nsIWindowMediatorListener>> would be safer

On the other hand you can copy-construct an nsCOMArray<nsIWindowMediatorListener> listeners(mListeners).
(Assignee)

Comment 6

3 years ago
Created attachment 8500865 [details] [diff] [review]
Release window mediator lock to avoid deadlock

Uses an nsCOMArray<nsIWindowMediatorListener> instead of an nsTArray<nsIWindowMediatorListener*>.
Attachment #8499951 - Attachment is obsolete: true
Attachment #8499951 - Flags: review?(neil)
Attachment #8500865 - Flags: review?(neil)

Comment 7

3 years ago
Comment on attachment 8500865 [details] [diff] [review]
Release window mediator lock to avoid deadlock

Tentative r=me depending on the outcome of comment #4.
Attachment #8500865 - Flags: review?(neil) → review+

Comment 8

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=2fdb6d3cf97e

Comment 9

3 years ago
Garrett, want to give one of the debug builds in https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-7c7bb0c0f6d1/ a shot once they're finished?
Flags: needinfo?(gholms)
Neil, I'm inclined to land this and file a follow up about removing the lock altogether, given that nobody else replied on the mailing list.

Updated

3 years ago
Assignee: nobody → bmarsd
(Assignee)

Comment 11

3 years ago
(In reply to Josh Matthews [:jdm] from comment #10)
> Neil, I'm inclined to land this and file a follow up about removing the lock
> altogether, given that nobody else replied on the mailing list.

It looks like someone did reply to the thread, saying to remove the lock and use MOZ_RELEASE_ASSERT(NS_IsMainThread()).
Feel like giving that a try?
Flags: needinfo?(bmarsd)
(Assignee)

Comment 13

3 years ago
Sure, I'll start working on a patch.
Flags: needinfo?(bmarsd)
(Assignee)

Comment 14

3 years ago
Created attachment 8506498 [details] [diff] [review]
Remove the lock in nsWindowMediator
Attachment #8506498 - Flags: feedback?(josh)
Comment on attachment 8506498 [details] [diff] [review]
Remove the lock in nsWindowMediator

Looking good!
Attachment #8506498 - Flags: feedback?(josh) → feedback+
(Assignee)

Comment 16

3 years ago
Comment on attachment 8506498 [details] [diff] [review]
Remove the lock in nsWindowMediator

Thanks!

I wonder if nsWindowMediator::AddListener and RemoveListener should have asserts. They didn't acquire the lock before, but they modify mListeners.
Attachment #8506498 - Flags: review?(neil)
Actually we assert if we addref or release off-main-thread these days, so it's unlikely that we're actually using this off the main thread at all.

Updated

3 years ago
Attachment #8506498 - Flags: review?(neil) → review+
https://tbpl.mozilla.org/?tree=Try&rev=2765aae703d0
(Assignee)

Comment 19

3 years ago
(In reply to Josh Matthews [:jdm] from comment #18)
> https://tbpl.mozilla.org/?tree=Try&rev=2765aae703d0

Looking at the failures, they don't seem to be related to the patch. Is there anything I should do?
Nope. Garrett, could you give a build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-014d714fe3ee/ a shot?
(Reporter)

Comment 21

3 years ago
I can't seem to reproduce the problem with that build, so either that patch fixed it or an addon update obviated it.  Nice work!  I can't speak to whether or not replacing that lock will have other side effects, but if I encounter any strangeness with window closing in the next few days I will chime in again.
Flags: needinfo?(gholms)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51b51f049d9
https://hg.mozilla.org/mozilla-central/rev/e51b51f049d9
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.