[PATCH v2 0/12] MR2691: mshtml: More ref leak fixes.
This should finally fix the ref leaks that keep all gecko nsDocuments alive even on the most basic docs. -- v2: mshtml/tests: Fix element leak in elem_fire_event. mshtml: Pass actual node_ccp to ccref_decr for nodes. mshtml: Fix nsChannel's load_info leak. mshtml: Support cycle collection for nsChannel. mshtml: Ensure SnowWhite objects are cleared when forcing cycle collection. mshtml: Supply dummy logger when forcing cycle collection. mshtml: Flush all gecko thread events after detaching Gecko browser. mshtml: Fix nsProgressEvent leak for pre IE10 modes. mshtml: Fix URI leak in NewURI on failure. mshtml: Fix nsIFile dir leak in init_xpcom. mshtml: Fix factory leak in init_nsio. mshtml: Do not release the principal returned by GetPrincipal. https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsembed.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mshtml/nsembed.c b/dlls/mshtml/nsembed.c index 3e315bc7211..16659055295 100644 --- a/dlls/mshtml/nsembed.c +++ b/dlls/mshtml/nsembed.c @@ -2440,6 +2440,7 @@ nsIXMLHttpRequest *create_nsxhr(nsIDOMWindow *nswindow) nsres = nsIGlobalObject_QueryInterface(nsglo, &IID_nsIScriptObjectPrincipal, (void**)&sop); assert(nsres == NS_OK); + /* The returned principal is *not* AddRef'd */ nspri = nsIScriptObjectPrincipal_GetPrincipal(sop); nsIScriptObjectPrincipal_Release(sop); @@ -2451,7 +2452,6 @@ nsIXMLHttpRequest *create_nsxhr(nsIDOMWindow *nswindow) if(NS_FAILED(nsres)) nsIXMLHttpRequest_Release(nsxhr); } - nsISupports_Release(nspri); nsIGlobalObject_Release(nsglo); if(NS_FAILED(nsres)) { ERR("nsIXMLHttpRequest_Init failed: %08lx\n", nsres); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mshtml/nsio.c b/dlls/mshtml/nsio.c index d29bb43544e..d60b2f0fa81 100644 --- a/dlls/mshtml/nsio.c +++ b/dlls/mshtml/nsio.c @@ -4000,9 +4000,9 @@ void init_nsio(nsIComponentManager *component_manager) } nsres = nsIFactory_CreateInstance(old_factory, NULL, &IID_nsIIOService, (void**)&nsio); + nsIFactory_Release(old_factory); if(NS_FAILED(nsres)) { ERR("Couldn not create nsIOService instance %08lx\n", nsres); - nsIFactory_Release(old_factory); return; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsembed.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/mshtml/nsembed.c b/dlls/mshtml/nsembed.c index 16659055295..9952a553afd 100644 --- a/dlls/mshtml/nsembed.c +++ b/dlls/mshtml/nsembed.c @@ -561,6 +561,7 @@ static BOOL init_xpcom(const PRUnichar *gre_path) } nsres = NS_InitXPCOM2(&pServMgr, gre_dir, (nsIDirectoryServiceProvider*)&nsDirectoryServiceProvider2); + nsIFile_Release(gre_dir); if(NS_FAILED(nsres)) { ERR("NS_InitXPCOM2 failed: %08lx\n", nsres); FreeLibrary(xul_handle); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsio.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/mshtml/nsio.c b/dlls/mshtml/nsio.c index d60b2f0fa81..5bd07334566 100644 --- a/dlls/mshtml/nsio.c +++ b/dlls/mshtml/nsio.c @@ -3892,8 +3892,11 @@ static nsresult NSAPI nsIOServiceHook_NewURI(nsIIOServiceHook *iface, const nsAC len = MultiByteToWideChar(CP_UTF8, 0, aOriginCharset, -1, NULL, 0); charset = SysAllocStringLen(NULL, len-1); - if(!charset) + if(!charset) { + if(base_wine_uri) + nsIFileURL_Release(&base_wine_uri->nsIFileURL_iface); return NS_ERROR_OUT_OF_MEMORY; + } MultiByteToWideChar(CP_UTF8, 0, aOriginCharset, -1, charset, len); cp = cp_from_charset_string(charset); @@ -3905,6 +3908,7 @@ static nsresult NSAPI nsIOServiceHook_NewURI(nsIIOServiceHook *iface, const nsAC if(base_wine_uri) { hres = combine_url(base_wine_uri->uri, new_spec, &urlmon_uri); + nsIFileURL_Release(&base_wine_uri->nsIFileURL_iface); }else { hres = create_uri(new_spec, 0, &urlmon_uri); if(FAILED(hres)) @@ -3916,8 +3920,6 @@ static nsresult NSAPI nsIOServiceHook_NewURI(nsIIOServiceHook *iface, const nsAC nsres = create_nsuri(urlmon_uri, &wine_uri); IUri_Release(urlmon_uri); - if(base_wine_uri) - nsIFileURL_Release(&base_wine_uri->nsIFileURL_iface); if(NS_FAILED(nsres)) return nsres; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Since we don't make use of the iface and it's transferred to the constructor, we have to discard it. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlevent.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/htmlevent.c b/dlls/mshtml/htmlevent.c index dd0bfe8db02..a38997b284a 100644 --- a/dlls/mshtml/htmlevent.c +++ b/dlls/mshtml/htmlevent.c @@ -3122,8 +3122,10 @@ static DOMEvent *progress_event_ctor(void *iface, nsIDOMEvent *nsevent, eventid_ { DOMProgressEvent *progress_event; - if(compat_mode < COMPAT_MODE_IE10) + if(compat_mode < COMPAT_MODE_IE10) { + nsIDOMProgressEvent_Release(iface); return event_ctor(sizeof(DOMEvent), &DOMEvent_dispex, NULL, NULL, nsevent, event_id, compat_mode); + } if(!(progress_event = event_ctor(sizeof(DOMProgressEvent), &DOMProgressEvent_dispex, DOMProgressEvent_query_interface, DOMProgressEvent_destroy, nsevent, event_id, compat_mode))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> This fixes leaks when some Gecko thread events hold refs to the document or other objects. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsembed.c | 27 +++++++++++++++++++++++++++ dlls/mshtml/nsiface.idl | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/dlls/mshtml/nsembed.c b/dlls/mshtml/nsembed.c index 9952a553afd..30c0ef513b8 100644 --- a/dlls/mshtml/nsembed.c +++ b/dlls/mshtml/nsembed.c @@ -46,6 +46,7 @@ WINE_DECLARE_DEBUG_CHANNEL(gecko); #define NS_PREFERENCES_CONTRACTID "@mozilla.org/preferences;1" #define NS_VARIANT_CONTRACTID "@mozilla.org/variant;1" #define NS_CATEGORYMANAGER_CONTRACTID "@mozilla.org/categorymanager;1" +#define NS_THREADMANAGER_CONTRACTID "@mozilla.org/thread-manager;1" #define NS_XMLHTTPREQUEST_CONTRACTID "@mozilla.org/xmlextras/xmlhttprequest;1" #define PR_UINT32_MAX 0xffffffff @@ -1103,6 +1104,29 @@ nsresult get_nsinterface(nsISupports *iface, REFIID riid, void **ppv) return nsres; } +static void flush_thread_nsevents(void) +{ + nsIThreadManager *threadmgr; + nsIThread *thread; + nsresult nsres; + cpp_bool b; + + nsres = nsIServiceManager_GetServiceByContractID(pServMgr, NS_THREADMANAGER_CONTRACTID, &IID_nsIThreadManager, (void**)&threadmgr); + if(NS_FAILED(nsres)) + return; + + nsres = nsIThreadManager_GetCurrentThread(threadmgr, &thread); + nsIThreadManager_Release(threadmgr); + if(NS_FAILED(nsres)) + return; + + do { + nsres = nsIThread_ProcessNextEvent(thread, FALSE, &b); + } while(NS_SUCCEEDED(nsres) && b); + + nsIThread_Release(thread); +} + static HRESULT nsnode_to_nsstring_rec(nsIContentSerializer *serializer, nsIDOMNode *nsnode, nsAString *str) { nsIDOMNodeList *node_list = NULL; @@ -2398,6 +2422,9 @@ void detach_gecko_browser(GeckoBrowser *This) nsIWebBrowserChrome_Release(&This->nsIWebBrowserChrome_iface); + /* Flush gecko thread events that might be holding refs */ + flush_thread_nsevents(); + /* Force cycle collection */ if(window_utils) { nsIDOMWindowUtils_CycleCollect(window_utils, NULL, 0); diff --git a/dlls/mshtml/nsiface.idl b/dlls/mshtml/nsiface.idl index 5991e0de45b..2189e65a5d5 100644 --- a/dlls/mshtml/nsiface.idl +++ b/dlls/mshtml/nsiface.idl @@ -270,6 +270,46 @@ interface nsIRunnable : nsISupports nsresult Run(); } +[ + object, + uuid(88145945-3278-424e-9f37-d874cbdd9f6f), + local +] +interface nsIEventTarget : nsISupports +{ + nsresult IsOnCurrentThread(bool *aResult); + nsresult Dispatch(nsIRunnable *aEvent, uint32_t aFlags); + nsresult DispatchFromScript(nsIRunnable *aEvent, uint32_t aFlags); +} + +[ + object, + uuid(5801d193-29d1-4964-a6b7-70eb697ddf2b), + local +] +interface nsIThread : nsIEventTarget +{ + nsresult GetPRThread(void /*PRThread*/ **aResult); + nsresult Shutdown(); + nsresult HasPendingEvents(bool *aResult); + nsresult ProcessNextEvent(bool aMayWait, bool *aResult); + nsresult AsyncShutdown(); +} + +[ + object, + uuid(1be89eca-e2f7-453b-8d38-c11ba247f6f3), + local +] +interface nsIThreadManager : nsISupports +{ + nsresult NewThread(uint32_t aCreationFlags, uint32_t aStackSize, nsIThread **aResult); + nsresult GetThreadFromPRThread(void /*PRThread*/ *aThread, nsIThread **aResult); + nsresult GetMainThread(nsIThread **aResult); + nsresult GetCurrentThread(nsIThread **aResult); + nsresult GetIsMainThread(bool *aResult); +} + [ object, uuid(d1899240-f9d2-11d2-bdd6-000064657374), -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> When forcing cycle collection manually, Gecko treats it as a specific type of collection. This kind of collection, unfortunately, will not always clean everything, unlike when shutting down the CC, which results in leaks when closing a doc obj because when we force it we expect it to behave like a shutdown for the given doc obj. Enabling graph logging *will* however clean everything (with env vars), to be able to track all the nodes. To get the same behavior, we can supply a dummy logger that fails early when opening the files (after which wine-gecko replaces the logger with NULL, except now it always visits everything, which is what we want), without actually shutting down the CC, as we aren't actually shutting down, so another doc obj might be created later. And there's no XPCOM interface for shutting it down, anyway. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsembed.c | 116 +++++++++++++++++++++++++++++++++++++++- dlls/mshtml/nsiface.idl | 38 ++++++++++++- 2 files changed, 152 insertions(+), 2 deletions(-) diff --git a/dlls/mshtml/nsembed.c b/dlls/mshtml/nsembed.c index 30c0ef513b8..8d437278d0b 100644 --- a/dlls/mshtml/nsembed.c +++ b/dlls/mshtml/nsembed.c @@ -41,6 +41,7 @@ WINE_DECLARE_DEBUG_CHANNEL(gecko); #define NS_APPSTARTUPNOTIFIER_CONTRACTID "@mozilla.org/embedcomp/appstartup-notifier;1" #define NS_WEBBROWSER_CONTRACTID "@mozilla.org/embedding/browser/nsWebBrowser;1" #define NS_COMMANDPARAMS_CONTRACTID "@mozilla.org/embedcomp/command-params;1" +#define NS_CYCLECOLLECTORLOGGER_CONTRACTID "@mozilla.org/cycle-collector-logger;1" #define NS_HTMLSERIALIZER_CONTRACTID "@mozilla.org/layout/contentserializer;1?mimetype=text/html" #define NS_EDITORCONTROLLER_CONTRACTID "@mozilla.org/editor/editorcontroller;1" #define NS_PREFERENCES_CONTRACTID "@mozilla.org/preferences;1" @@ -2201,6 +2202,119 @@ static const nsISupportsWeakReferenceVtbl nsSupportsWeakReferenceVtbl = { nsSupportsWeakReference_GetWeakReference }; +static nsICycleCollectorLogSink dummy_log_sink; + +static nsresult NSAPI dummy_log_sink_QueryInterface(nsICycleCollectorLogSink *iface, nsIIDRef riid, void **result) +{ + if(IsEqualGUID(&IID_nsISupports, riid) || IsEqualGUID(&IID_nsICycleCollectorLogSink, riid)) { + *result = &dummy_log_sink; + return NS_OK; + } + return NS_NOINTERFACE; +} + +static nsrefcnt NSAPI dummy_log_sink_AddRef(nsICycleCollectorLogSink *iface) +{ + return 2; +} + +static nsrefcnt NSAPI dummy_log_sink_Release(nsICycleCollectorLogSink *iface) +{ + return 1; +} + +static nsresult NSAPI dummy_log_sink_Open(nsICycleCollectorLogSink *iface, void **aGCLog, void **aCCLog) +{ + /* Always fail in Open, which disables the log entirely, while still forcing the CC */ + return NS_ERROR_NOT_IMPLEMENTED; +} + +static nsresult NSAPI dummy_log_sink_CloseGCLog(nsICycleCollectorLogSink *iface) +{ + return NS_OK; +} + +static nsresult NSAPI dummy_log_sink_CloseCCLog(nsICycleCollectorLogSink *iface) +{ + return NS_OK; +} + +static nsresult NSAPI dummy_log_sink_GetFilenameIdentifier(nsICycleCollectorLogSink *iface, nsAString *aIdentifier) +{ + return NS_ERROR_NOT_IMPLEMENTED; +} + +static nsresult NSAPI dummy_log_sink_SetFilenameIdentifier(nsICycleCollectorLogSink *iface, const nsAString *aIdentifier) +{ + return NS_OK; +} + +static nsresult NSAPI dummy_log_sink_GetProcessIdentifier(nsICycleCollectorLogSink *iface, LONG *aIdentifier) +{ + return NS_ERROR_NOT_IMPLEMENTED; +} + +static nsresult NSAPI dummy_log_sink_SetProcessIdentifier(nsICycleCollectorLogSink *iface, LONG aIdentifier) +{ + return NS_OK; +} + +static nsresult NSAPI dummy_log_sink_GetGcLog(nsICycleCollectorLogSink *iface, nsIFile **aPath) +{ + return NS_ERROR_NOT_IMPLEMENTED; +} + +static nsresult NSAPI dummy_log_sink_GetCcLog(nsICycleCollectorLogSink *iface, nsIFile **aPath) +{ + return NS_ERROR_NOT_IMPLEMENTED; +} + +static const nsICycleCollectorLogSinkVtbl dummy_log_sink_vtbl = { + dummy_log_sink_QueryInterface, + dummy_log_sink_AddRef, + dummy_log_sink_Release, + dummy_log_sink_Open, + dummy_log_sink_CloseGCLog, + dummy_log_sink_CloseCCLog, + dummy_log_sink_GetFilenameIdentifier, + dummy_log_sink_SetFilenameIdentifier, + dummy_log_sink_GetProcessIdentifier, + dummy_log_sink_SetProcessIdentifier, + dummy_log_sink_GetGcLog, + dummy_log_sink_GetCcLog +}; + +static nsICycleCollectorLogSink dummy_log_sink = { &dummy_log_sink_vtbl }; + +static void force_cycle_collection(nsIDOMWindowUtils *window_utils) +{ + nsICycleCollectorListener *logger, *all_logger; + nsresult nsres; + + /* Use a dummy logger to force the CC to clean up the entire graph, as in shutdown */ + nsres = nsIServiceManager_GetServiceByContractID(pServMgr, NS_CYCLECOLLECTORLOGGER_CONTRACTID, &IID_nsICycleCollectorListener, (void**)&logger); + if(NS_FAILED(nsres)) { + logger = NULL; + goto collect; + } + + nsres = nsICycleCollectorListener_AllTraces(logger, &all_logger); + nsICycleCollectorListener_Release(logger); + logger = NULL; + if(NS_FAILED(nsres)) + goto collect; + logger = all_logger; + + /* The dummy log sink will fail when opening the log, which will ignore it */ + nsres = nsICycleCollectorListener_SetLogSink(logger, &dummy_log_sink); + assert(nsres == NS_OK); + +collect: + nsIDOMWindowUtils_CycleCollect(window_utils, logger, 0); + if(logger) + nsICycleCollectorListener_Release(logger); +} + static HRESULT init_browser(GeckoBrowser *browser) { mozIDOMWindowProxy *mozwindow; @@ -2427,7 +2541,7 @@ void detach_gecko_browser(GeckoBrowser *This) /* Force cycle collection */ if(window_utils) { - nsIDOMWindowUtils_CycleCollect(window_utils, NULL, 0); + force_cycle_collection(window_utils); nsIDOMWindowUtils_Release(window_utils); } } diff --git a/dlls/mshtml/nsiface.idl b/dlls/mshtml/nsiface.idl index 2189e65a5d5..ccd1f5b82b0 100644 --- a/dlls/mshtml/nsiface.idl +++ b/dlls/mshtml/nsiface.idl @@ -181,7 +181,6 @@ typedef nsISupports nsILocalFile; typedef nsISupports nsIDOMHTMLMenuElement; typedef nsISupports nsIDOMCaretPosition; typedef nsISupports nsIFrameRequestCallback; -typedef nsISupports nsICycleCollectorListener; typedef nsISupports nsIDOMHTMLCanvasElement; typedef nsISupports nsIQueryContentEventResult; typedef nsISupports nsIDOMBlob; @@ -310,6 +309,43 @@ interface nsIThreadManager : nsISupports nsresult GetIsMainThread(bool *aResult); } +[ + object, + uuid(3ad9875f-d0e4-4ac2-87e3-f127f6c02ce1), + local +] +interface nsICycleCollectorLogSink : nsISupports +{ + nsresult Open(void /*FILE*/ **aGCLog, void /*FILE*/ **aCCLog); + nsresult CloseGCLog(); + nsresult CloseCCLog(); + nsresult GetFilenameIdentifier(nsAString *aIdentifier); + nsresult SetFilenameIdentifier(const nsAString *aIdentifier); + nsresult GetProcessIdentifier(int32_t *aIdentifier); + nsresult SetProcessIdentifier(int32_t aIdentifier); + nsresult GetGcLog(nsIFile **aPath); + nsresult GetCcLog(nsIFile **aPath); +} + +[ + object, + uuid(703b53b6-24f6-40c6-9ea9-aeb2dc53d170), + local +] +interface nsICycleCollectorListener : nsISupports +{ + nsresult AllTraces(nsICycleCollectorListener **aListener); + nsresult GetWantAllTraces(bool *aAllTraces); + nsresult GetDisableLog(bool *aDisableLog); + nsresult SetDisableLog(bool aDisableLog); + nsresult GetLogSink(nsICycleCollectorLogSink **aLogSink); + nsresult SetLogSink(nsICycleCollectorLogSink *aLogSink); + nsresult GetWantAfterProcessing(bool *aWantAfterProcessing); + nsresult SetWantAfterProcessing(bool aWantAfterProcessing); + nsresult ProcessNext(void /*nsICycleCollectorHandler*/ *aHandler, bool *aCanContinue); + nsresult AsLogger(void /*nsCycleCollectorLogger*/ **aRetVal); +} + [ object, uuid(d1899240-f9d2-11d2-bdd6-000064657374), -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsembed.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/dlls/mshtml/nsembed.c b/dlls/mshtml/nsembed.c index 8d437278d0b..59a77039de3 100644 --- a/dlls/mshtml/nsembed.c +++ b/dlls/mshtml/nsembed.c @@ -2298,6 +2298,7 @@ static void force_cycle_collection(nsIDOMWindowUtils *window_utils) goto collect; } + /* Note that AllTraces logger merely sets a flag on the original, so it can't be re-used */ nsres = nsICycleCollectorListener_AllTraces(logger, &all_logger); nsICycleCollectorListener_Release(logger); logger = NULL; @@ -2313,6 +2314,19 @@ collect: nsIDOMWindowUtils_CycleCollect(window_utils, logger, 0); if(logger) nsICycleCollectorListener_Release(logger); + + /* Collection might have marked objects as SnowWhite, ensure they're cleaned up now, + but use a normal logger here that does *not* force the CC, to avoid wasting time. */ + nsres = nsIServiceManager_GetServiceByContractID(pServMgr, NS_CYCLECOLLECTORLOGGER_CONTRACTID, &IID_nsICycleCollectorListener, (void**)&logger); + if(NS_FAILED(nsres)) + logger = NULL; + else { + nsres = nsICycleCollectorListener_SetLogSink(logger, &dummy_log_sink); + assert(nsres == NS_OK); + } + nsIDOMWindowUtils_CycleCollect(window_utils, logger, 0); + if(logger) + nsICycleCollectorListener_Release(logger); } static HRESULT init_browser(GeckoBrowser *browser) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> This is traversed from the Gecko document, and should be part of the graph (it refers to gecko objects that participate in it, and which are part of a cycle). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/binding.h | 2 +- dlls/mshtml/nsio.c | 119 +++++++++++++++++++++++++++++++++--------- 2 files changed, 95 insertions(+), 26 deletions(-) diff --git a/dlls/mshtml/binding.h b/dlls/mshtml/binding.h index 948bfb1c02b..07a8a999ec3 100644 --- a/dlls/mshtml/binding.h +++ b/dlls/mshtml/binding.h @@ -37,7 +37,7 @@ typedef struct { nsIHttpChannelInternal nsIHttpChannelInternal_iface; nsICacheInfoChannel nsICacheInfoChannel_iface; - LONG ref; + nsCycleCollectingAutoRefCnt ccref; nsWineURI *uri; nsIInputStream *post_data_stream; diff --git a/dlls/mshtml/nsio.c b/dlls/mshtml/nsio.c index 5bd07334566..e54bb3bc91e 100644 --- a/dlls/mshtml/nsio.c +++ b/dlls/mshtml/nsio.c @@ -45,6 +45,7 @@ static const IID NS_IOSERVICE_CID = static const IID IID_nsWineURI = {0x5088272e, 0x900b, 0x11da, {0xc6,0x87, 0x00,0x0f,0xea,0x57,0xf2,0x1a}}; +static ExternalCycleCollectionParticipant nschannel_ccp; static nsIIOService *nsio = NULL; static const char *request_method_strings[] = {"GET", "PUT", "POST"}; @@ -540,6 +541,14 @@ static nsresult NSAPI nsChannel_QueryInterface(nsIHttpChannel *iface, nsIIDRef r }else if(IsEqualGUID(&IID_nsICacheInfoChannel, riid)) { TRACE("(%p)->(IID_nsICacheInfoChannel %p)\n", This, result); *result = is_http_channel(This) ? &This->nsICacheInfoChannel_iface : NULL; + }else if(IsEqualGUID(&IID_nsXPCOMCycleCollectionParticipant, riid)) { + TRACE("(%p)->(IID_nsXPCOMCycleCollectionParticipant %p)\n", This, result); + *result = &nschannel_ccp; + return S_OK; + }else if(IsEqualGUID(&IID_nsCycleCollectionISupports, riid)) { + TRACE("(%p)->(IID_nsCycleCollectionISupports %p)\n", This, result); + *result = &This->nsIHttpChannel_iface; + return S_OK; }else { TRACE("(%p)->(%s %p)\n", This, debugstr_guid(riid), result); *result = NULL; @@ -556,7 +565,7 @@ static nsresult NSAPI nsChannel_QueryInterface(nsIHttpChannel *iface, nsIIDRef r static nsrefcnt NSAPI nsChannel_AddRef(nsIHttpChannel *iface) { nsChannel *This = impl_from_nsIHttpChannel(iface); - nsrefcnt ref = InterlockedIncrement(&This->ref); + nsrefcnt ref = ccref_incr(&This->ccref, (nsISupports*)&This->nsIHttpChannel_iface); TRACE("(%p) ref=%ld\n", This, ref); @@ -566,30 +575,9 @@ static nsrefcnt NSAPI nsChannel_AddRef(nsIHttpChannel *iface) static nsrefcnt NSAPI nsChannel_Release(nsIHttpChannel *iface) { nsChannel *This = impl_from_nsIHttpChannel(iface); - LONG ref = InterlockedDecrement(&This->ref); + nsrefcnt ref = ccref_decr(&This->ccref, (nsISupports*)&This->nsIHttpChannel_iface, &nschannel_ccp); - if(!ref) { - nsIFileURL_Release(&This->uri->nsIFileURL_iface); - if(This->owner) - nsISupports_Release(This->owner); - if(This->post_data_stream) - nsIInputStream_Release(This->post_data_stream); - if(This->load_group) - nsILoadGroup_Release(This->load_group); - if(This->notif_callback) - nsIInterfaceRequestor_Release(This->notif_callback); - if(This->original_uri) - nsIURI_Release(This->original_uri); - if(This->referrer) - nsIURI_Release(This->referrer); - - free_http_headers(&This->response_headers); - free_http_headers(&This->request_headers); - - free(This->content_type); - free(This->charset); - free(This); - } + TRACE("(%p) ref=%ld\n", This, ref); return ref; } @@ -2263,6 +2251,80 @@ static const nsICacheInfoChannelVtbl nsCacheInfoChannelVtbl = { nsCacheInfoChannel_SetAllowStaleCacheContent }; +static nsresult NSAPI nsChannel_traverse(void *ccp, void *p, nsCycleCollectionTraversalCallback *cb) +{ + nsChannel *This = impl_from_nsIHttpChannel(p); + + TRACE("%p\n", This); + + describe_cc_node(&This->ccref, "nsChannel", cb); + + if(This->owner) + note_cc_edge(This->owner, "owner", cb); + if(This->post_data_stream) + note_cc_edge((nsISupports*)This->post_data_stream, "post_data_stream", cb); + if(This->load_group) + note_cc_edge((nsISupports*)This->load_group, "load_group", cb); + if(This->notif_callback) + note_cc_edge((nsISupports*)This->notif_callback, "notif_callback", cb); + if(This->original_uri) + note_cc_edge((nsISupports*)This->original_uri, "original_uri", cb); + if(This->referrer) + note_cc_edge((nsISupports*)This->referrer, "referrer", cb); + + return NS_OK; +} + +static nsresult NSAPI nsChannel_unlink(void *p) +{ + nsChannel *This = impl_from_nsIHttpChannel(p); + + TRACE("%p\n", This); + + if(This->owner) { + nsISupports_Release(This->owner); + This->owner = NULL; + } + if(This->post_data_stream) { + nsIInputStream_Release(This->post_data_stream); + This->post_data_stream = NULL; + } + if(This->load_group) { + nsILoadGroup_Release(This->load_group); + This->load_group = NULL; + } + if(This->notif_callback) { + nsIInterfaceRequestor_Release(This->notif_callback); + This->notif_callback = NULL; + } + if(This->original_uri) { + nsIURI_Release(This->original_uri); + This->original_uri = NULL; + } + if(This->referrer) { + nsIURI_Release(This->referrer); + This->referrer = NULL; + } + + return NS_OK; +} + +static void NSAPI nsChannel_delete_cycle_collectable(void *p) +{ + nsChannel *This = impl_from_nsIHttpChannel(p); + nsChannel_unlink(p); + + TRACE("(%p)\n", This); + + nsIFileURL_Release(&This->uri->nsIFileURL_iface); + free_http_headers(&This->response_headers); + free_http_headers(&This->request_headers); + + free(This->content_type); + free(This->charset); + free(This); +} + static void invalidate_uri(nsWineURI *This) { if(This->uri) { @@ -3517,10 +3579,10 @@ static nsresult create_nschannel(nsWineURI *uri, nsChannel **ret) channel->nsIUploadChannel_iface.lpVtbl = &nsUploadChannelVtbl; channel->nsIHttpChannelInternal_iface.lpVtbl = &nsHttpChannelInternalVtbl; channel->nsICacheInfoChannel_iface.lpVtbl = &nsCacheInfoChannelVtbl; - channel->ref = 1; channel->request_method = METHOD_GET; list_init(&channel->response_headers); list_init(&channel->request_headers); + ccref_init(&channel->ccref, 1); nsIFileURL_AddRef(&uri->nsIFileURL_iface); channel->uri = uri; @@ -3991,6 +4053,11 @@ static nsIIOServiceHook nsIOServiceHook = { &nsIOServiceHookVtbl }; void init_nsio(nsIComponentManager *component_manager) { + static const CCObjCallback nschannel_ccp_callback = { + nsChannel_traverse, + nsChannel_unlink, + nsChannel_delete_cycle_collectable + }; nsIFactory *old_factory = NULL; nsresult nsres; @@ -4010,6 +4077,8 @@ void init_nsio(nsIComponentManager *component_manager) nsres = nsIIOService_SetHook(nsio, &nsIOServiceHook); assert(nsres == NS_OK); + + ccp_init(&nschannel_ccp, &nschannel_ccp_callback); } void release_nsio(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/nsio.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/mshtml/nsio.c b/dlls/mshtml/nsio.c index e54bb3bc91e..8178b87b4ca 100644 --- a/dlls/mshtml/nsio.c +++ b/dlls/mshtml/nsio.c @@ -2263,6 +2263,8 @@ static nsresult NSAPI nsChannel_traverse(void *ccp, void *p, nsCycleCollectionTr note_cc_edge(This->owner, "owner", cb); if(This->post_data_stream) note_cc_edge((nsISupports*)This->post_data_stream, "post_data_stream", cb); + if(This->load_info) + note_cc_edge((nsISupports*)This->load_info, "load_info", cb); if(This->load_group) note_cc_edge((nsISupports*)This->load_group, "load_group", cb); if(This->notif_callback) @@ -2289,6 +2291,10 @@ static nsresult NSAPI nsChannel_unlink(void *p) nsIInputStream_Release(This->post_data_stream); This->post_data_stream = NULL; } + if(This->load_info) { + nsISupports_Release(This->load_info); + This->load_info = NULL; + } if(This->load_group) { nsILoadGroup_Release(This->load_group); This->load_group = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Avoids having to look it up again later during collection. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Not sure why this was commented out, perhaps old wine-gecko version that didn't support it properly? --- dlls/mshtml/htmlnode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mshtml/htmlnode.c b/dlls/mshtml/htmlnode.c index 5d70b09e234..02f3e2929b2 100644 --- a/dlls/mshtml/htmlnode.c +++ b/dlls/mshtml/htmlnode.c @@ -500,7 +500,7 @@ static ULONG WINAPI HTMLDOMNode_AddRef(IHTMLDOMNode *iface) static ULONG WINAPI HTMLDOMNode_Release(IHTMLDOMNode *iface) { HTMLDOMNode *This = impl_from_IHTMLDOMNode(iface); - LONG ref = ccref_decr(&This->ccref, (nsISupports*)&This->IHTMLDOMNode_iface, /*&node_ccp*/ NULL); + LONG ref = ccref_decr(&This->ccref, (nsISupports*)&This->IHTMLDOMNode_iface, &node_ccp); TRACE("(%p) ref=%ld\n", This, ref); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/tests/events.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index ee15ada69ef..7f7515bbcef 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -383,6 +383,7 @@ static void _elem_fire_event(unsigned line, IUnknown *unk, const WCHAR *event, V str = SysAllocString(event); in_fire_event++; hres = IHTMLElement3_fireEvent(elem3, str, evobj, &b); + IHTMLElement3_Release(elem3); in_fire_event--; SysFreeString(str); ok_(__FILE__,line)(hres == S_OK, "fireEvent failed: %08lx\n", hres); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2691
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=132116 Your paranoid android. === debian11 (32 bit he:IL report) === mshtml: events.c:2457: Test failed: complete = ffffffff === debian11 (32 bit zh:CN report) === mshtml: events.c:2457: Test failed: complete = ffffffff
Jacek Caban (@jacek) commented about dlls/mshtml/htmlevent.c:
{ DOMProgressEvent *progress_event;
- if(compat_mode < COMPAT_MODE_IE10) + if(compat_mode < COMPAT_MODE_IE10) { + nsIDOMProgressEvent_Release(iface); Those constructors are just error-prone, I think it could be better to just move AddRefs to constructors.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2691#note_30987
Jacek Caban (@jacek) commented about dlls/mshtml/nsembed.c:
+ nsIThreadManager *threadmgr; + nsIThread *thread; + nsresult nsres; + cpp_bool b; + + nsres = nsIServiceManager_GetServiceByContractID(pServMgr, NS_THREADMANAGER_CONTRACTID, &IID_nsIThreadManager, (void**)&threadmgr); + if(NS_FAILED(nsres)) + return; + + nsres = nsIThreadManager_GetCurrentThread(threadmgr, &thread); + nsIThreadManager_Release(threadmgr); + if(NS_FAILED(nsres)) + return; + + do { + nsres = nsIThread_ProcessNextEvent(thread, FALSE, &b); This looks hackish. For example, releasing HTML document may happen while processing other document's script. If that's the case, you'd flush other document's events, which does not seem desired.
More generally, I'm not sure what you're trying to fix. We try to force CC run when releasing a document is released because it's nice if it's able to collect objects at that point. But if CC fails to clean up things right away, it's also fine as long as it will clean up things on later run. The next two commits seem similarly controversial. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2691#note_30988
On Mon Apr 24 09:57:42 2023 +0000, Jacek Caban wrote:
Those constructors are just error-prone, I think it could be better to just move AddRefs to constructors. I wanted to avoid it because there's too much boilerplate as-is for each constructor, now will be even more…
I agree it's error prone. Are you still against the idea of storing as much as possible in a data table instead? Including compatibility mode like this, so it wouldn't be part of the constructor at all. The reason I liked it was that it would reduce the amount of boilerplate copy pasted code in each constructor. Or are you still against it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2691#note_31002
On Mon Apr 24 09:58:38 2023 +0000, Jacek Caban wrote:
This looks hackish. For example, releasing HTML document may happen while processing other document's script. If that's the case, you'd flush other document's events, which does not seem desired. More generally, I'm not sure what you're trying to fix. We try to force CC run when releasing a document is released because it's nice if it's able to collect objects at that point. But if CC fails to clean up things right away, it's also fine as long as it will clean up things on later run. The next two commits seem similarly controversial. Somehow it doesn't end up cleaning them later. This is obvious in tests, which is where I tried to remove leaks because it's much easier to reproduce with a clean run.
Without this patch it will be hard to analyze test leaks and see if stuff is truly leaking or not, because it ends up leaking in the tests of course, but the process exits so it doesn't actually matter there. On "real" apps it will be hard as well. Note that the CC never gets shutdown. So in theory if an app stops processing messages, it will never clean the leaks. I'm not sure if this is possible by deregistering gecko's wndprocs somehow, but it's a possible issue. We can't trigger a shutdown in the CC either (although it requires wine-gecko API changes), because we don't know when it's truly shutting down. A new document obj can be created at any time, etc. The current behavior isn't an issue if Firefox is the app running but it is for us in embedded situation. Do you have some ideas here? Maybe I'm missing something. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2691#note_31003
Somehow it doesn't end up cleaning them later.
It would be interesting to understand why. Note that a long-living application may never release any document and we still want CC to do its job properly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2691#note_31010
On Mon Apr 24 16:11:02 2023 +0000, Jacek Caban wrote:
Somehow it doesn't end up cleaning them later. It would be interesting to understand why. Note that a long-living application may never release any document and we still want CC to do its job properly. Well it's pretty obvious why, in hindsight of course (wasn't when I started).
The Cycle Collector runs periodically, using some internal heuristics. But it runs only on the main thread, and only sometimes during processing of events (it can be starved as well, if it takes too long). It can also be forced to run, of course, which is what we're doing when we're closing the document object. That won't work when some gecko event keeps ref to it, since it will be a hard ref "unknown edge". Additionally, it runs during "shutdown"—with a deep scan to clean everything. The CC is global, so this shutdown is for the entire gecko engine. But in our use case, shutdown *never* happens. We never tell it to shutdown, and I don't see a way to tell it either (no XPCOM API to shutdown XPCOM itself). But that's besides the point because even if we had such an API we couldn't make use of it during document object release. Since we aren't really shutting down the engine, another doc obj can be created later, and it would be bad if the CC was shutdown. So for tests it's pretty clear to see what happens: Objects leak. Some events hold ref to the objects. The CC is forced to run when the doc obj is released, but since there's those refs it doesn't know of, they're kept alive and skipped. There's no further message pump at the end of the test (unless we run another test which creates another document obj). So the objects simply leak. Of course in this particular case it's benign for memory usage, since we're shutting down the test process anyway. But it does mean they show up as leaks when we try to look for them, since they were never freed. This isn't just for the events holding refs, the SnowWhiteKiller for instance doesn't run immediately, it runs on the next CC (SnowWhite are objects marked for deletion but which aren't deleted immediately, for whatever reason I can't understand). If the last forced CC has some SnowWhite objects, they will leak when the test process finishes. If an app somehow does this and doesn't pump messages to gecko anymore (not sure if that's possible?) those objects will leak. Since both the CC and the events holding refs are only "executed" during the gecko message pump / event loop. That's why I wanted to clean them up during the forced CC since it's the only way I could find to ensure it… But you're right that it can potentially have issues with multiple doc objects. I can probably drop these patches for now, but the leaks will exist when analyzed, sadly (even the CC graph will show "unknown edge"). Probably not a problem in practice as much, since it's only the "last" few document objects that will be leaking, but still annoying. If you have any ideas how to do this please let me know as I'd love to have these integrated upstream, if possible. Otherwise I guess I will keep these patches for local use when I analyze leaks since otherwise it becomes impossible to analyze them. Sigh. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2691#note_31020
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin