1457713 - Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in (mail, mailnew and chat) and convert QueryInterface(aIID)

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

See
https://hg.mozilla.org/mozilla-central/rev/c58f0b4dd849
https://hg.mozilla.org/mozilla-central/rev/8db81ce4d3a2

Also, having Ci.nsISupports will assert, making debug runs real red:
Assertion failure: !ifaces.Contains((nsISupports::COMTypeInfo<nsISupports, void>::kIID), IIDComparator()), at /builds/worker/workspace/build/src/dom/base/MozQueryInterface.cpp:101

One appears to be related to
TEST-UNEXPECTED-FAIL | comm/mailnews/jsaccount/test/unit/test_componentsExist.js | xpcshell return code: 1
but all Mozmill runs also failed on debug.

Comment 1

[Kris Maglione [:kmag]]

Just run this script. It should do everything for you.

https://bitbucket.org/kmaglione/m-c-rewrites/src/8f1732b171db0102c781a6c2668098d2a492aeca/processors/chromeutils-import.jsm

Comment 2

[Kris Maglione [:kmag]]

Er, sorry:

https://bitbucket.org/kmaglione/m-c-rewrites/src/3acc52e49ab40cc8451d780c3721f4b0f1b2f70e/processors/chromeutils-generateQI.jsm

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-remove-nsISupports.patch

Let's get rid of the debug crashes first:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=11d9986f9f03049c2a8147ed9494bc9761f4fbc5

Comment 4

[:aceman]

Comment on attachment 8971814 [details] [diff] [review]
1457713-remove-nsISupports.patch

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

Thanks. But we could do the optimization of QueryInterface implementations (per the m-c patch) too in the next patch. I'll look at it.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-remove-nsISupports.patch

I sadly missed half of it :-(

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-fix-test.patch

And more.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=68a969b5c1e18293c196e16d9d156279659c87d5

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8971827 [details] [diff] [review]
1457713-remove-nsISupports.patch

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

::: chat/content/browserRequest.js
@@ -27,5 @@
>  
>    QueryInterface: function(aIID) {
> -    if (aIID.equals(Ci.nsIWebProgressListener)   ||
> -        aIID.equals(Ci.nsISupportsWeakReference) ||
> -        aIID.equals(Ci.nsISupports))

This is wrong. The reason we removed nsISupports from the generateQI calls is because it's implicitly included, and having an extra copy is bad for code style and performance. In manually-generated implementations, it should still be included (though C++ will never call a JS QueryInterface method for nsISupports).

Comment 8

[:aceman]

Comment on attachment 8971828 [details] [diff] [review]
1457713-fix-test.patch

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

Kris, what about this?

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8971828 [details] [diff] [review]
1457713-fix-test.patch

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

This looks fine, but the test_componentsExist changes are probably unnecessary, and it seems incomplete:

https://dxr.mozilla.org/comm-central/search?q=regexp%3A%5B%5E(%5DCi%5C.nsISupports%5Cb%5B%5E.%3B)%5Cs%5D+-path%3Amozilla%2F&redirect=false

You're probably better off just starting with running the script in comment 2, and then cleaning up the nsISupports references it misses, where they're not passed as immediate arguments to generateQI.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-QI.patch

OK, here I'm only removing Ci.nsISupports from calls to generateQI().
The changes in test_componentsExist.js are necessary or the test will fail.

Kris, how does one run your script?

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-QI.patch (v2)

Actually, test_componentsExist.js only needed one small change.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+1) from comment #10)
> Created attachment 8971833 [details] [diff] [review]
> 1457713-QI.patch
> 
> OK, here I'm only removing Ci.nsISupports from calls to generateQI().
> The changes in test_componentsExist.js are necessary or the test will fail.
> 
> Kris, how does one run your script?

Huh. I thought I had a README...

Basically:

  %cd /path/to/comm-central
  %sh /path/to/m-c-rewrites/run-replacer.sh chromeutils-generateQI

You might want to add "mozilla/" to the list of exclusions in chromeutils-generateQI.jsm first, though.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-QI.patch (v3) [landed comment #14]

OK, let's not mess with the test, just address the problem at hand.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e51241c644f8
Port bug 1456035: Remove nsISupports from calls to XPCOMUtils.generateQI(). rs=bustage-fix

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #9)
> This looks fine, but the test_componentsExist changes are probably
> unnecessary, and it seems incomplete:
> https://dxr.mozilla.org/comm-central/search?q=regexp%3A%5B%5E(%5DCi%5C.
> nsISupports%5Cb%5B%5E.%3B)%5Cs%5D+-path%3Amozilla%2F&redirect=false

Thanks for the feedback.

For now, I've removed Ci.nsISupports from calls to generateQI(). There was no doubt that this would now assert.

That JS Account stuff is just mysterious, again, I discarded the test changes and just fixed the problem at hand.

https://dxr.mozilla.org/comm-central/search?q=regexp%3A%5B%5E(%5DCi%5C.nsISupports%5Cb%5B%5E.%3B)%5Cs%5D+-path%3Amozilla%2F&redirect=false
As for being incomplete: I don't know.

For example, I can't tell what that code does with the interfaces:
https://dxr.mozilla.org/comm-central/rev/8d636ad5810b5586433294c16cb2376d826fcca8/chat/components/src/imContacts.js#245
  getInterfaces: function(countRef) {
    let interfaces = [Ci.nsIClassInfo, Ci.nsISupports, Ci.imITag];
    countRef.value = interfaces.length;
    return interfaces;
  },
  getHelperForLanguage: language => null,
  flags: 0,
  QueryInterface: XPCOMUtils.generateQI([Ci.imITag, Ci.nsIClassInfo])
It doesn't use 'interfaces' to pass into generateQI().

We'll run your script in peace once the immediate bustage is solved, hopefully now.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971837 [details] [diff] [review]
1457713-QI.patch (v3) [landed comment #14]

Philipp, you want to see all Calendar changes, so here you go.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-suite.patch [landed comment #24]

This should cover suite/

Comment 18

[:aceman]

Attachment: 1457713-cal.patch QI rewrite - calendar

Comment 19

[:aceman]

Attachment: 1457713-mail patch QI rewrite - TB [landed comment #25]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=49cf13eb35d767bdbde1194612eb599a16410c12

Comment 20

[Martin Schröder [:mschroeder]]

Comment on attachment 8971859 [details] [diff] [review]
1457713-cal.patch QI rewrite - calendar

># HG changeset patch
># User aceman <[e-mail]>
># Parent  59fcdd0ef581a44c954ff0722f1914b41aa2d058
>Bug 1457713 - rewrite QueryInterface JS implementations the same way as m-c bug 1456035 does - calendar. r?Fallen
>
[...]
>diff --git a/calendar/base/src/calCachedCalendar.js b/calendar/base/src/calCachedCalendar.js
>--- a/calendar/base/src/calCachedCalendar.js
>+++ b/calendar/base/src/calCachedCalendar.js
[...]
>@@ -224,17 +221,17 @@ calCachedCalendar.prototype = {
>             Components.utils.reportError(exc);
>         }
>     },
> 
>     getOfflineAddedItems: function(callbackFunc) {
>         let self = this;
>         self.offlineCachedItems = {};
>         let getListener = {
>-            QueryInterface: XPCOMUtils.generateQI([Components.interfaces.calIOperationListener]),
>+            QueryInterface: ChromeUtilsgenerateQI(["calIOperationListener"]),
>             onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems) {
>                 for (let item of aItems) {
>                     self.offlineCachedItems[item.hashId] = item;
>                     self.offlineCachedItemFlags[item.hashId] = cICL.OFFLINE_FLAG_CREATED_RECORD;
>                 }
>             },
> 
>             onOperationComplete: function(aCalendar, aStatus, aOpType, aId, aDetail) {

It should be "ChromeUtils.generateQI" instead of "ChromeUtilsgenerateQI".

Comment 21

[:aceman]

Attachment: 1457713-cal.patch QI rewrite - calendar v1.1

Thanks, of course:)

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971860 [details] [diff] [review]
1457713-mail patch QI rewrite - TB [landed comment #25]

Thanks.

Comment 23

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8971855 [details] [diff] [review]
1457713-suite.patch [landed comment #24]

Looks good. Thanks. SeaMonkey needs some additional rewrites as in the cal patches. I will put these in a followup bug.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3a02fb80639b
Port bug 1456035 [Remove nsISupports from calls to XPCOMUtils.generateQI()] suite/ part. r=frg DONTBUILD CLOSED TREE

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/46c62a2fcc9d
rewrite QueryInterface JS implementations the same way as M-C bug 1456035 does - chat/ mail/ mailnews/. r=jorgk

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8971861 [details] [diff] [review]
1457713-cal.patch QI rewrite - calendar v1.1

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

Thanks for the patches! calendar uses .generateQI([Ci.calITheInterface] instead of .generateQI(["calITheInterface"]), I'd appreciate if you could change this throughout the patch. r+ with that changed.

Comment 27

[Jorg K (CEST = GMT+2)]

Hmm, the ones I can see are all like:
  QueryInterface: XPCOMUtils.generateQI(calTimezoneInterfaces), ...

Comment 28

[:aceman]

Attachment: 1457713-cal.patch QI rewrite - calendar v1.2 [landed comment #33]

OK, then we can drop most of the patch.
There are still some manual 'QueryInterface: function(aIID) {}' remaining but those are implemented specially (call some more functions internally) so I didn't replace those.

Comment 29

[Jorg K (CEST = GMT+2)]

Aceman, is there going to be a patch to change XPCOMUtils.generateQI to ChromeUtils.generateQI?

I don't understand why you've already changed some of those in the Calendar patch and now you've dropped those hunks again.

Comment 30

[:aceman]

(In reply to Jorg K (GMT+1) from comment #29)
> Aceman, is there going to be a patch to change XPCOMUtils.generateQI to
> ChromeUtils.generateQI?

Nothing more.

> I don't understand why you've already changed some of those in the Calendar
> patch and now you've dropped those hunks again.

Because I made those changes seeing there was a manual 'QueryInterface: function(aIID) {}' function to convert and then I found out I don't know how to convert it;) So I just didn't backtrack those other changes. Now I did.

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #30)
> (In reply to Jorg K (GMT+1) from comment #29)
> > Aceman, is there going to be a patch to change XPCOMUtils.generateQI to
> > ChromeUtils.generateQI?
> Nothing more.
Well, the bug is about changing over to ChromeUtils.generateQI() which is partly done in the rewrites of the manual QIs. So are you saying you're not going to do that patch? We should try to get Kris' script going for that.

Comment 32

[Jorg K (CEST = GMT+2)]

Filed bug 1458370 for the Calendar part.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/25cf4e4c34f5
replace 'QueryInterface: function()' with 'XPCOMUtils.generateQI()' - calendar. r=philipp

Comment 34

[Jorg K (CEST = GMT+2)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12)
> Huh. I thought I had a README...
> Basically:
>   %cd /path/to/comm-central
>   %sh /path/to/m-c-rewrites/run-replacer.sh chromeutils-generateQI
> You might want to add "mozilla/" to the list of exclusions in
> chromeutils-generateQI.jsm first, though.

Thunderbird is moving to a setup where comm-central lives in a subdirectory "comm" of mozilla-central. This suits the script since it expects to run 'mach' from the top directory. Before it was in mozilla/.

I downloaded run-replacer.sh. main.js, Processor.jsm, Replacer.jsm and chromeutils-generateQI.jsm from
https://bitbucket.org/kmaglione/m-c-rewrites/src/3acc52e49ab4?at=default,
I've added all m-c directories to the path to be ignored, like "dom", "layout", "netwerk", etc., only leaving "comm" to be processed. I wasn't sure where the script automatically excludes .hg, .cargo and the object directory, so I added them as well, better safe than very sorry in case it modified .hg.

Running the script I now get:
JavaScript error: ./main.js, line 13: NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIFile.initWithPath]
That line is: const baseDir = nsFile(arguments[0]);

BTW, I'm running this on Windows in Mozilla's development environment which provides a bash shell.

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: replacer-script.7z

Contains modified chromeutils-generateQI.jsm. Aceman, perhaps you can give it a try.

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-generateQI-mail.patch

Here's a patch created using 'sed' with indentation corrected manually.

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-generateQI-chat.patch

Straight 'sed', no manual fix needed.

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-generateQI-mailnews.patch

'sed' and manual indentation fixes where necessary. So we're done here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3748f8572ea8aebbb3351a60431f0e0249e2095e

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: 1457713-generateQI-mail.patch

Grrr, that hit jQuery :-( - Now fixed.

Comment 40

[:aceman]

Comment on attachment 8975981 [details] [diff] [review]
1457713-generateQI-mailnews.patch

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

Thanks.
It seems ChromeUtils.generateQI() also takes string names of the interfaces (Ci.X and "X"). Do we know which version is better to use?

::: mailnews/base/search/src/nsMsgTraitService.js
@@ +34,1 @@
>        Ci.nsIMsgTraitService]),

Maybe this does not need to be wrapped.

::: mailnews/base/src/newMailNotificationService.js
@@ +56,5 @@
>  NewMailNotificationService.prototype = {
>    classDescription: "Maintain counts of new and unread messages",
>    classID:              Components.ID("{740880E6-E299-4165-B82F-DF1DCAB3AE22}"),
>    contractID:           "@mozilla.org/newMailNotificationService;1",
> +  QueryInterface:       ChromeUtils.generateQI([Ci.nsIObserver, Ci.nsIFolderListener, Ci.mozINewMailNotificationService]),

This seems to need some wrapping.

Comment 41

[:aceman]

Comment on attachment 8975983 [details] [diff] [review]
1457713-generateQI-mail.patch

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

Thanks.

::: mail/base/content/msgHdrView.js
@@ +404,1 @@
>        [Ci.nsIMsgHeaderSink]),

unwrap

Comment 42

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #40)
> It seems ChromeUtils.generateQI() also takes string names of the interfaces
> (Ci.X and "X"). Do we know which version is better to use?
Looks like a string causes more processing:
https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/dom/base/MozQueryInterface.cpp#62

But I won't change strings to Ci.X now.

Comment 43

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+2) (bustage-fix only, NI for urgent reviews) from comment #42)
> (In reply to :aceman from comment #40)
> > It seems ChromeUtils.generateQI() also takes string names of the interfaces
> > (Ci.X and "X"). Do we know which version is better to use?
> Looks like a string causes more processing:
> https://searchfox.org/mozilla-central/rev/
> 8affe6e83188787eb61fe0528eeb6eef6081ba06/dom/base/MozQueryInterface.cpp#62
> 
> But I won't change strings to Ci.X now.

Not really. The two are about equally efficient. If it was in JITted code and we had IC support for Ci property lookups, that form might be cheaper, but in the current regime, if anything, it's probably slightly more expensive.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f378ff5bbc89
Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in mail. r=aceman
https://hg.mozilla.org/comm-central/rev/bf2c27b81cae
Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in chat. r=aceman
https://hg.mozilla.org/comm-central/rev/148c2d27abc3
Port bug 1456035: Change XPCOMUtils.generateQI to ChromeUtils.generateQI in mailnews. r=aceman