Stop using nsISupportsArray for RDF data sources

RESOLVED FIXED in Thunderbird 55.0

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Unassigned)

Tracking

Trunk
Thunderbird 55.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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

2 years ago
I think this is mostly (if not even exclusively) used in Seamonkey.
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk

Comment 3

2 years ago
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.
Flags: needinfo?(philip.chee)
Flags: needinfo?(iann_bugzilla)

Comment 4

2 years ago
There are two people you can ask:
standard8@mozilla.com
neil@httl.net
Flags: needinfo?(standard8)
Flags: needinfo?(philip.chee)
Flags: needinfo?(neil)

Comment 5

2 years ago
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

2 years ago
> 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

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

Comment 8

2 years ago
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

2 years ago
(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

2 years ago
OK, but then you will have to test it locally as pushing to try-comm-central may have no effect.
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.
Flags: needinfo?(standard8)
(Reporter)

Comment 12

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

Comment 13

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

Comment 14

2 years ago
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 16

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

Does it break SM/tests?
Flags: needinfo?(philip.chee)
Flags: needinfo?(ewong)

Comment 17

2 years ago
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.

Updated

2 years ago
Depends on: 464710

Comment 18

2 years ago
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

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

Comment 20

2 years ago
Created attachment 8843761 [details] [diff] [review]
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

2 years ago
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.
Flags: needinfo?(frgrahl)
Blocks: 1334779
>> 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.
Flags: needinfo?(frgrahl)
(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.
Flags: needinfo?(ewong)
(Reporter)

Comment 24

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

Comment 25

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

Comment 26

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

Thanks Aceman, let's go with this for now.
Attachment #8843761 - Flags: review+

Comment 27

2 years ago
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.
Attachment #8843761 - Flags: review+

Comment 28

2 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(philip.chee)
Flags: needinfo?(neil)
Flags: needinfo?(iann_bugzilla)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Blocks: 1345770
No longer blocks: 1334779

Updated

2 years ago
Blocks: 1345919

Comment 29

2 years ago
See you all in bug 1345919 ;-)
You need to log in before you can comment on or make changes to this bug.