1318806 - Stop using nsISupportsArray for RDF data sources

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Eric Rahm [:erahm]]

The final major usage of nsISupportsArray is in comm-central's nsMsgFolderDataSource [1]. There are a few other RDF data sources that mention nsISuppportsArray in comments, but it isn't actually used. In bug 1312132 we updated the interfaces to take an nsISupports instance instead, but internally we still QI to nsISupportsArray in nsMsgFolderDataSource.

One option is to switch away from using the RDF command inteface, the other would be to figure out where the binding is that calls the command interface and switch it to nsIArray.

[1] https://dxr.mozilla.org/comm-central/rev/70de4f4f7c99060bc25a5fe8e76e5b3f74e9984d/mailnews/base/src/nsMsgFolderDataSource.h#92-99

Comment 1

[Eric Rahm [:erahm]]

Just a note: unfortunately I don't think I have enough background to help start a patch for this one. I also did a quick search for JS implementations of |IsCommandEnabled|, but didn't find any.

Comment 2

[:aceman]

I think this is mostly (if not even exclusively) used in Seamonkey.

Comment 3

[:aceman]

I tried to see if these functions are ever called by TB and it does not seem to be the case.

Can somebody try if even Seamonkey uses it? Just put a debug output into the functions and run the test suite to see if they are ever hit.
I'd think if this were called by some RDF or other ancient code, we would see the caller too in DXR. BUt I can't spot it.

This is the last place in all of c-c and m-c that uses nsISupportsArray (https://dxr.mozilla.org/comm-central/search?q=nsISupportsArray) so there is a risk the implementation in m-c can do away any time and break the build.

Comment 4

[Philip Chee]

There are two people you can ask:
[e-mail]
[e-mail]

Comment 5

[:aceman]

And neither of them is probable to respond now.

Can you just put a printf() into those functions and run the Seamonkey test suite on the build?

Comment 6

[Philip Chee]

> And neither of them is probable to respond now.
> 
> Can you just put a printf() into those functions and run the Seamonkey test
> suite on the build?

Due to Bug 1268970 we cannot run any SeaMonkey "test suite".

Comment 7

[Philip Chee]

And for /mailnews/ we just assume that Thunderbird test coverage includes that.

Comment 8

[:aceman]

I would not assume that in this case. RDF stuff may have remained in SM that is no longer used in TB.

Should I just make a patch that removes all this code and see what happens in SM?

Comment 9

[Philip Chee]

(In reply to :aceman from comment #8)

> Should I just make a patch that removes all this code and see what happens
> in SM?

I guess that's one way of testing it.

Comment 10

[:aceman]

OK, but then you will have to test it locally as pushing to try-comm-central may have no effect.

Comment 11

[Mark Banner (:standard8)]

I could believe those functions aren't used in Thunderbird any more - bug 420506 and friends killed a lot of RDF stuff from there, though not quite everything.

For SM, I'm not sure what exactly the state is, removing nsISupportsArray is definitely worth it, so it might be any idea just to land some empty functions for those two as well.

Note at some stage FF is likely to get rid of RDF as well, though I haven't currently heard any short term plans for that.

Comment 12

[Eric Rahm [:erahm]]

Just a heads up, I'm landing the |nsISupportsArray| removal patch in m-c next week (March 6, 2017).

Comment 13

[:aceman]

I have pushed some experiments to try server, as discussed with Ratty.

Comment 14

[Jorg K (CEST = GMT+2)]

They all failed to build (as you may have noticed, C-C was closed at the time waiting for bustage to arrive). Try again now.

Comment 15

[Jorg K (CEST = GMT+2)]

Silly question: How does the RDF here relate to, for example:
https://dxr.mozilla.org/comm-central/search?q=rv+%3D+rdf-%3EGetResource(uri%2C+gette&redirect=false

Comment 16

[:aceman]

Ratty, Ewong, can you try running with this patch locally?
https://hg.mozilla.org/try-comm-central/rev/47d9db9f9f20acd5201b34f25a02938085f266d3

Does it break SM/tests?

Comment 17

[:aceman]

It seems we can't just disable the whole nsMsgFolderDataSource.* for TB. Some RDF functions are still used in TB:
mailnews/base/content/virtualFolderListDialog.xul:          datasources="rdf:msgaccountmanager rdf:mailnewsfolders"
mailnews/base/src/nsSubscribableServer.cpp:        rv = mRDFService->GetDataSource("rdf:subscribe", getter_AddRefs(ds));
mail/base/content/subscribe.js:var subscribeDS = RDF.GetDataSource("rdf:subscribe");
mail/base/content/msgSelectOffline.xul:        datasources="rdf:msgaccountmanager rdf:mailnewsfolders"

My current patch breaks those dialogs, e.g. the virtual folder list is totally empty.

I'll see if bug 464710 helps to remove these uses.

If that doesn't work (or not in time), I'll see if we can disable just the RDF commands in nsMsgFolderDataSource.* or only the few functions with nsISupportsArray.

Comment 18

[Jorg K (CEST = GMT+2)]

I don't think it will work in time, so a landable patch that fixes bustage an maintains existing (known) functionality is what we need. I have a patch that fixes bustage only :-(

Comment 19

[:aceman]

Yes, so I'll prepare a landable patch now and will pursue the more ambitious plan in other bugs.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1318806-nsISupportsArray.patch

Minimal removal. Patch stolen from Aceman ;-)

Let's see whether
  File > New > Saved Search, Choose button
and
  Right-click unified folder, Properties, Choose button still work.

If not, we're in trouble.

Comment 21

[Jorg K (CEST = GMT+2)]

This works for me, so I'd be happy to r+ this and land this as a temporary measure when the bustage arrives.

Frank-Rainer, I assume you will want to keep compiling even if these removals will break some corners of SM, right?
The other SM crowd is just soooooo slow in answering.

Comment 22

[Frank-Rainer Grahl (:frg)]

>> Frank-Rainer, I assume you will want to keep compiling even if these removals will break some corners of SM, right?

Yes. Thanks.

>> The other SM crowd is just soooooo slow in answering.

ewong is fighting with the builders and Ratty is currently absent.

Comment 23

[Edmund Wong (:ewong)]

(In reply to Jorg K (GMT+1) from comment #21)
> This works for me, so I'd be happy to r+ this and land this as a temporary
> measure when the bustage arrives.
> 
> Frank-Rainer, I assume you will want to keep compiling even if these
> removals will break some corners of SM, right?
> The other SM crowd is just soooooo slow in answering.

Yes, I do apologize for being slow.  I'll improve my response type.

Comment 24

[Eric Rahm [:erahm]]

Heads up: the removal patch has landed on mozilla-inbound, I'd suggest landing your temporary fix.

Comment 25

[Jorg K (CEST = GMT+2)]

Thanks, I'm CC'ed on bug 792209. We've been waiting for you ;-)

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8843761 [details] [diff] [review]
1318806-nsISupportsArray.patch

Thanks Aceman, let's go with this for now.

Comment 27

[:aceman]

Comment on attachment 8843761 [details] [diff] [review]
1318806-nsISupportsArray.patch

Yes, we assume these commands are not used in TB.

SM may have a bigger problem.

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/16ef5e8993a0963c2d46930f0f534bd930357868

I'm closing this bug. The SM people will notice whatever doesn't work for them. TB will remove further dependency on nsMsgFolderDataSource.cpp in bug 464710.

Let's file a follow-up for
- removing the code we don't need any more after bug 464710,
- the additional work SM might require and for
- removing code that is now "#if 0"-ed away.

Comment 29

[Jorg K (CEST = GMT+2)]

See you all in bug 1345919 ;-)