601157 - EXSLT regexp:match leaks a the result nodeset

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Philipp Wagner [:imphil]]

Attachment: Testcase

As noted in bug 581305 the EXSLT extension functions leak when a nodeset is returned (possibly the same happens if an object is returned?). This currently only affects regexp:match.

Steps to reproduce:
* Use a trunk debug build
* Run it with XPCOM_MEM_LEAK_LOG=leak.log set
* Load the attached testcase.html (it's adapted from the mochitest added in bug 581305)
* look at leak.log


If the match function in txEXSLTRegExFunctions.js is reduced to

>  match: function(context, str, regex, flags) {
>    var nodeset = Components.classes[NODESET_CONTRACTID]
>                            .createInstance(Ci.txINodeSet);
>
>    return nodeset;
>  }

that's what we leak (the testcase fails of course):


>    0 TOTAL                                          32      104   524163        3 ( 1745,18 +/-  2710,50)   677370        3 ( 1693,30 +/-  3262,21)
>  803 txAExprResult                                  24       24      365        1 (   37,70 +/-    13,03)      713        1 (   41,22 +/-    11,33)
>  808 txNodeSetAdaptor                               48       48        1        1 (    1,00 +/-     0,00)        6        1 (    2,18 +/-     0,98)
>  819 txXPathObjectAdaptor                           32       32        1        1 (    1,00 +/-     0,00)        6        1 (    2,18 +/-     0,98)

Comment 1

[Philipp Wagner [:imphil]]

If this bug is fixed, the testcase for regexp:match can be reenabled:
http://mxr.mozilla.org/mozilla-central/source/content/xslt/tests/mochitest/test_exslt_regex.html?force=1#23

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is that all that leaks for you?  I see a 300K leak over several dozen kinds of objects.

Comment 3

[Philipp Wagner [:imphil]]

If you do not modify txEXSLTRegExFunctions.js as shown in comment 0 it leaks way more (whole DOM documents etc.). I modified the match function above to show that the result leaks (and with it everything else).

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Add/Release log for txNodeSetAdaptor

This seems like as good a place to start as any.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There are 3 JS-looking refs in that log, but only two that get GCd on shutdown.  The ref that appears to be the unbalanced one is the addref at:

 	xul.dll!txNodeSetAdaptor::AddRef() 
	xul.dll!NS_TableDrivenQI() 
	xul.dll!txNodeSetAdaptor::QueryInterface() 
	xul.dll!XPCWrappedNative::InitTearOff() 
	xul.dll!XPCWrappedNative::FindTearOff() 
	xul.dll!XPCWrappedNative::GetNewOrUsed() 
	xul.dll!XPCConvert::NativeInterface2JSObject() 
	xul.dll!NativeInterface2JSObject() 
	xul.dll!nsXPConnect::WrapNative() 
	xul.dll!nsJSCID::CreateInstance() 
	xul.dll!NS_InvokeByIndex_P() 
	xul.dll!CallMethodHelper::Invoke() 
	xul.dll!CallMethodHelper::Call() 
	xul.dll!XPCWrappedNative::CallMethod() 
	xul.dll!XPC_WN_CallMethod() 
	mozjs.dll!js::CallJSNative() 
	mozjs.dll!CallCompiler::generateNativeStub() 
	mozjs.dll!js::mjit::ic::NativeCall() 
	mozjs.dll!js::mjit::EnterMethodJIT() 
	mozjs.dll!js::mjit::JaegerShot() 
	mozjs.dll!js::RunScript() 
	mozjs.dll!js::Invoke() 
	mozjs.dll!js::ExternalInvoke() 
	mozjs.dll!JS_CallFunctionValue() 
	xul.dll!nsXPCWrappedJSClass::CallMethod() 
	xul.dll!nsXPCWrappedJS::CallMethod() 
	xul.dll!PrepareAndDispatch() 
	xul.dll!SharedStub() 
	xul.dll!NS_InvokeByIndex_P() 
	xul.dll!txXPCOMExtensionFunctionCall::evaluate() 
	xul.dll!FilterExpr::evaluate() 
	xul.dll!txValueOf::execute() 
	xul.dll!txXSLTProcessor::execute() 
	xul.dll!txMozillaXSLTProcessor::TransformToFragment() 
	xul.dll!NS_InvokeByIndex_P() 
	xul.dll!CallMethodHelper::Invoke() 
	xul.dll!CallMethodHelper::Call() 
	xul.dll!XPCWrappedNative::CallMethod() 
	xul.dll!XPC_WN_CallMethod() 
	mozjs.dll!js::CallJSNative() 
	mozjs.dll!CallCompiler::generateNativeStub() 
	mozjs.dll!js::mjit::ic::NativeCall() 
	mozjs.dll!js::mjit::EnterMethodJIT() 
	mozjs.dll!js::mjit::JaegerShot() 
	mozjs.dll!js::RunScript() 
	mozjs.dll!js::Execute() 
	mozjs.dll!JS_EvaluateUCScriptForPrincipals() 
	mozjs.dll!JS_EvaluateUCScriptForPrincipalsVersion() 
	xul.dll!nsJSContext::EvaluateString() 
	xul.dll!nsScriptLoader::EvaluateScript() 
	xul.dll!nsScriptLoader::ProcessRequest() 
	xul.dll!nsScriptLoader::ProcessScriptElement() 
	xul.dll!nsScriptElement::MaybeProcessScript() 
	xul.dll!nsHTMLScriptElement::MaybeProcessScript() 
	xul.dll!nsHTMLScriptElement::DoneAddingChildren() 
	xul.dll!nsHtml5TreeOpExecutor::RunScript() 
	xul.dll!nsHtml5TreeOpExecutor::RunFlushLoop() 
	xul.dll!nsHtml5ExecutorFlusher::Run() 
	xul.dll!nsThread::ProcessNextEvent() 
	xul.dll!NS_ProcessNextEvent_P() 
	xul.dll!mozilla::ipc::MessagePump::Run() 
	xul.dll!MessageLoop::RunInternal() 
	xul.dll!MessageLoop::RunHandler() 
	xul.dll!MessageLoop::Run() 
	xul.dll!nsBaseAppShell::Run() 
	xul.dll!nsAppShell::Run() 
	xul.dll!nsAppStartup::Run() 
	xul.dll!XRE_main() 
	firefox.exe!NS_internal_main() 
	firefox.exe!wmain() 
	firefox.exe!__tmainCRTStartup() 
	firefox.exe!wmainCRTStartup() 
	kernel32.dll!76513677() 
	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
	ntdll.dll!774e9d42() 
	ntdll.dll!774e9d15() 

The direct NS_InvokeByIndex from the XSLT code here scares me too.  In the NS_InvokeByIndex calls in content/xtf these is magical deleting that goes on after the call.

Comment 6

[Peter Van der Beken [:peterv]]

I'll have to look into it more closely, but I wonder if we need to SetValIsInterface in http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp#546 if eNODESET or eOBJECT.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

That's almost what we want.  We have to guard ourselves a little so that we don't try to release booleans and the like.  I think this is what we want.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Oh, you said if eNODESET or eOBJECT, I just can't read ;-)

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 483780 [details] [diff] [review]
Patch

Please remove the tabs.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Tabs removed and patch pushed.

http://hg.mozilla.org/mozilla-central/rev/2c1a0c8337ec