This should finally fix the ref leaks that keep all gecko nsDocuments alive even on the most basic docs.
-- v4: 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: Store minimum compat mode required for events in the ctor table. 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.
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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);
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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; }
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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);
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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;
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Since such event types don't exist as separate event types in older modes, this prevents confusing leaks without bloating the constructors with more boilerplate.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/mshtml/htmlevent.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/dlls/mshtml/htmlevent.c b/dlls/mshtml/htmlevent.c index dd0bfe8db02..ccea4e16567 100644 --- a/dlls/mshtml/htmlevent.c +++ b/dlls/mshtml/htmlevent.c @@ -3122,9 +3122,6 @@ static DOMEvent *progress_event_ctor(void *iface, nsIDOMEvent *nsevent, eventid_ { DOMProgressEvent *progress_event;
- if(compat_mode < COMPAT_MODE_IE10) - 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))) return NULL; @@ -3154,6 +3151,7 @@ static DOMEvent *storage_event_ctor(void *iface, nsIDOMEvent *nsevent, eventid_t static const struct { REFIID iid; DOMEvent *(*ctor)(void *iface, nsIDOMEvent *nsevent, eventid_t, compat_mode_t); + compat_mode_t min_compat_mode; } event_types_ctor_table[] = { [EVENT_TYPE_EVENT] = { NULL, generic_event_ctor }, [EVENT_TYPE_UIEVENT] = { &IID_nsIDOMUIEvent, ui_event_ctor }, @@ -3164,7 +3162,7 @@ static const struct { [EVENT_TYPE_DRAG] = { NULL, generic_event_ctor }, [EVENT_TYPE_PAGETRANSITION] = { NULL, page_transition_event_ctor }, [EVENT_TYPE_CUSTOM] = { &IID_nsIDOMCustomEvent, custom_event_ctor }, - [EVENT_TYPE_PROGRESS] = { &IID_nsIDOMProgressEvent, progress_event_ctor }, + [EVENT_TYPE_PROGRESS] = { &IID_nsIDOMProgressEvent, progress_event_ctor, COMPAT_MODE_IE10 }, [EVENT_TYPE_MESSAGE] = { NULL, message_event_ctor }, [EVENT_TYPE_STORAGE] = { NULL, storage_event_ctor }, }; @@ -3175,6 +3173,9 @@ static DOMEvent *alloc_event(nsIDOMEvent *nsevent, compat_mode_t compat_mode, ev void *iface = NULL; DOMEvent *event;
+ if(compat_mode < event_types_ctor_table[event_type].min_compat_mode) + event_type = EVENT_TYPE_EVENT; + if(event_types_ctor_table[event_type].iid) nsIDOMEvent_QueryInterface(nsevent, event_types_ctor_table[event_type].iid, &iface);
From: Gabriel Ivăncescu gabrielopcode@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@gmail.com --- dlls/mshtml/binding.h | 2 +- dlls/mshtml/nsio.c | 125 +++++++++++++++++++++++++++++++++--------- 2 files changed, 101 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..5081c1fea3c 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,86 @@ 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 *owner = This->owner; + This->owner = NULL; + nsISupports_Release(owner); + } + if(This->post_data_stream) { + nsIInputStream *post_data_stream = This->post_data_stream; + This->post_data_stream = NULL; + nsIInputStream_Release(post_data_stream); + } + if(This->load_group) { + nsILoadGroup *load_group = This->load_group; + This->load_group = NULL; + nsILoadGroup_Release(load_group); + } + if(This->notif_callback) { + nsIInterfaceRequestor *notif_callback = This->notif_callback; + This->notif_callback = NULL; + nsIInterfaceRequestor_Release(notif_callback); + } + if(This->original_uri) { + nsIURI *original_uri = This->original_uri; + This->original_uri = NULL; + nsIURI_Release(original_uri); + } + if(This->referrer) { + nsIURI *referrer = This->referrer; + This->referrer = NULL; + nsIURI_Release(referrer); + } + + 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 +3585,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 +4059,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 +4083,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)
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/mshtml/nsio.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/dlls/mshtml/nsio.c b/dlls/mshtml/nsio.c index 5081c1fea3c..78da446dd3d 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) @@ -2291,6 +2293,11 @@ static nsresult NSAPI nsChannel_unlink(void *p) This->post_data_stream = NULL; nsIInputStream_Release(post_data_stream); } + if(This->load_info) { + nsISupports *load_info = This->load_info; + This->load_info = NULL; + nsISupports_Release(load_info); + } if(This->load_group) { nsILoadGroup *load_group = This->load_group; This->load_group = NULL;
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Avoids having to look it up again later during collection.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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);
From: Gabriel Ivăncescu gabrielopcode@gmail.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@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);
Updated again, this time I NULL first before releasing during unlink, to prevent corruption in case recursion can happen. I see other places do similar so better be safe.
Sorry for the noise.
Jacek Caban (@jacek) commented about dlls/mshtml/nsio.c:
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);
You could just move querying for nsWineURI below origin charset handling instead.
On Tue Apr 25 16:45:42 2023 +0000, Gabriel Ivăncescu wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/2691/diffs?diff_id=43913&start_sha=83dc0c808fa3ba6e40495e10b1d49ee40bdf76fb#d85ccdf4f69c07c4732da8a94e52860b56cd9c94_1125_1106)
Making our forced collection more aggressive seems reasonable to me, but I'd hope for a better interface for it. Depending on some side effect of otherwise unrelated logging does not look solid. If Gecko doesn't expose needed interface, it shouldn't be too hard to extend Gecko.
Message pumping in destructor would still be a problem. I guess it's a problem only in some specific tests. We could perhaps just pump window messages before destroying the object in problematic tests themselves.
On Tue Apr 25 17:11:34 2023 +0000, Jacek Caban wrote:
Making our forced collection more aggressive seems reasonable to me, but I'd hope for a better interface for it. Depending on some side effect of otherwise unrelated logging does not look solid. If Gecko doesn't expose needed interface, it shouldn't be too hard to extend Gecko. Message pumping in destructor would still be a problem. I guess it's a problem only in some specific tests. We could perhaps just pump window messages before destroying the object in problematic tests themselves.
I'm not sure if message pumping in the tests is reliable for this, unfortunately. It doesn't happen immediately, since it has some complex heuristics to do the CC. In tests it's not uncommon for the last 3 or 4 tests to leak. It even seems different on each run…
-
Gabriel Ivăncescu -
Gabriel Ivăncescu (＠insn)
-
Jacek Caban (＠jacek)