581305 - Expect nsIClassInfo of XPath extension functions in class, not in factory

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Philipp Wagner [:imphil]]

Attachment: patch v2

Split from bug 578142, it gets a bit too messy over there otherwise.

I wrote a mochitest for all three regexp functions (match, test, replace). While at it I saw that the patch broke those. So here's the updated txEXSLTRegExFunctions as well as the test and the original patch.

Please have a look at bug 578142 for the original discussion.

Comment 1

[Peter Van der Beken [:peterv]]

Comment on attachment 459752 [details] [diff] [review]
patch v2

>+            rv = helper->QueryInterface(aIID, (void**)aHelper);
>+            NS_ENSURE_SUCCESS(rv, rv);
> 
>             return NS_OK;

This can just be

            return helper->QueryInterface(aIID, (void**)aHelper);

>diff --git a/content/xslt/src/xslt/txEXSLTRegExFunctions.js b/content/xslt/src/xslt/txEXSLTRegExFunctions.js

>     QueryInterface: function(iid) {
>         if (iid.equals(Ci.nsISupports) ||
>+            iid.equals(Ci.nsIClassInfo) ||
>             iid.equals(Ci.txIEXSLTRegExFunctions))
>             return this;
>-
>-        if (iid.equals(Ci.nsIClassInfo))
>-            return txEXSLTRegExModule.factory
>+            
>+            return this.classInfo;

This looks wrong.

I don't really understand why you have to remove _xpcom_factory, afaict that should still work as is (see http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/XPCOMUtils.jsm#235).

Comment 2

[Philipp Wagner [:imphil]]

I removed the factory because the classinfo is now expected inside the class itself, not inside the factory. So having a custom factory instead of using the default one seemed unnecessary.

If you want to keep the factory, there are still two more problems:
1) txEXSLTRegExModule is not defined (at least I couldn't find it, I guess it should have read txIEXSLTRegExFunctions)
2) txIEXSLTRegExFunctions is still empty at that point, what would work is 
> if (iid.equals(Ci.nsIClassInfo))
>     return txEXSLTRegExFunctions.prototype._xpcom_factory;
but I don't really see the advantage of that approach.

I just saw that I have an 
> +            return this.classInfo;
in the patch, this was part of my testing that should not have been part of the patch. I'll remove it.

Comment 3

[Philipp Wagner [:imphil]]

Thinking about it again, we might want to keep the factory if we still want the class to be a singleton. But I don't see a reason why it should be one.

Comment 4

[Peter Van der Beken [:peterv]]

Dropping the factory is fine I guess. New patch coming?

Comment 5

[Philipp Wagner [:imphil]]

Attachment: patch v3

This version fixes the duplicate return statement that made it into the previous version. Also I set flags to 0 indicating that we don't actually have a singleton and removes all remains of it.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 471613 [details] [diff] [review]
patch v3

> txEXSLTRegExFunctions.prototype = {
>-    classID: Components.ID("{18a03189-067b-4978-b4f1-bafe35292ed6}"),
>+    classID: EXSLT_REGEXP_CID,
> 
>     QueryInterface: function(iid) {
>         if (iid.equals(Ci.nsISupports) ||
>+            iid.equals(Ci.nsIClassInfo) ||
>             iid.equals(Ci.txIEXSLTRegExFunctions))
>             return this;
> 
>-        if (iid.equals(Ci.nsIClassInfo))
>-            return txEXSLTRegExModule.factory
>-
>         throw Components.results.NS_ERROR_NO_INTERFACE;
>     },

This can now be:

        QueryInterface: XPCOMUtils.generateQI([Ci.txIEXSLTRegExFunctions,
                                               Ci.nsIClassInfo]),

Comment 7

[Philipp Wagner [:imphil]]

Attachment: patch v3.1 r=peterv

changes in v3.1: replaced own QueryInterface implementation with XPCOMUtils.generateQI

approval2.0 requested because we need this for XForms to work. This patch adds a mochitest as well.

Comment 8

[alexander :surkov (:asurkov)]

landed on 2.0 http://hg.mozilla.org/mozilla-central/rev/3f5093da5ab2

Comment 9

[(no longer active)]

This made the Md2 suite on all platforms leak, so I backed it out:

http://hg.mozilla.org/mozilla-central/rev/83b75c9b01b2

Example log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284659608.1284661384.15299.gz&fulltext=1

Comment 10

[Philipp Wagner [:imphil]]

Hm, I'm a bit lost here ... I can confirm that the test is OK and didn't just uncover a previously existing problem, there are no leaks in the test without the other changes.

The leak log looks to me like a whole document is leaked, but how could the patch have caused this? Unfortunately I can't find a point where to start looking for the problem.  Could someone more experienced give me a hint here?

Comment 11

[alexander :surkov (:asurkov)]

Theoretically that might be unknown accident leak, did you try it to run on try server?

Comment 12

[Philipp Wagner [:imphil]]

I didn't put it on try server, but I can reproduce the leaks reliably on my local machine.

Comment 13

[Philipp Wagner [:imphil]]

A quick update on this: what's leaking is actually regex:match, the other regex functions don't leak.

Comment 14

[Philipp Wagner [:imphil]]

Well ... I was wrong in comment #10 when I said the original code without the patch didn't leak - it does. I confirmed this now on enough machines and with tinderbox debug builds without any patch: they leak, the test only uncovered this. I opened bug 601157 for that, as I'm unable to do further debugging on this (I don't have enough XPCOM and nsCOMPtr knowledge, as I found out after many hourse of trying).

Comment 15

[alexander :surkov (:asurkov)]

well, can you change a tests to avoid leaks? or can we land it with disabled test until bug 601157 is fixed?

Comment 16

[Philipp Wagner [:imphil]]

Yes, we could remove/disable the test for regexp:match. The test was only added to test the extension functionality of XPath, not the individual EXSLT XPath functions, for which is would be sufficient to test only one of the EXSLT regex functions and the other ones don't leak.

I'll put together a patch for this and put it on try server to make sure we're safe.

Comment 17

[Philipp Wagner [:imphil]]

Attachment: patch v3.2: same as 3.1 with regex:match test disabled

Results on try look good with the regex:match test disabled:
http://tests.themasta.com/tinderboxpushlog/?tree=MozillaTry&rev=0f8d63098274

I attached the patch I pushed to try. It would be great if we could push this to mozilla-central after the tree opens again in order to not delay work on XForms further. After bug 601157 is fixed the regex:match test can be re-enabled.

Does this need another review, approval or anything?

Comment 18

[alexander :surkov (:asurkov)]

it sounds no, but we can't land it until beta 7 is shipped or you have blocking beta 7 status

Comment 19

[Philipp Wagner [:imphil]]

Attachment: [for checkin] v3.2 (same as 3.1 with regex:match test disabled)

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e6dcd36ed6c7