Closed Bug 1318806 Opened 5 years ago Closed 4 years ago

Stop using nsISupportsArray for RDF data sources

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: erahm, Unassigned)

References

Details

Attachments

(1 file)

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
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.
I think this is mostly (if not even exclusively) used in Seamonkey.
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
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)
There are two people you can ask:
standard8@mozilla.com
neil@httl.net
Flags: needinfo?(standard8)
Flags: needinfo?(philip.chee)
Flags: needinfo?(neil)
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?
> 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".
And for /mailnews/ we just assume that Thunderbird test coverage includes that.
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?
(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.
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)
Just a heads up, I'm landing the |nsISupportsArray| removal patch in m-c next week (March 6, 2017).
I have pushed some experiments to try server, as discussed with Ratty.
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.
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)
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.
Depends on: 464710
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 :-(
Yes, so I'll prepare a landable patch now and will pursue the more ambitious plan in other bugs.
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.
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)
>> 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)
Heads up: the removal patch has landed on mozilla-inbound, I'd suggest landing your temporary fix.
Thanks, I'm CC'ed on bug 792209. We've been waiting for you ;-)
Comment on attachment 8843761 [details] [diff] [review]
1318806-nsISupportsArray.patch

Thanks Aceman, let's go with this for now.
Attachment #8843761 - Flags: review+
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+
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
Closed: 4 years ago
Flags: needinfo?(philip.chee)
Flags: needinfo?(neil)
Flags: needinfo?(iann_bugzilla)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
No longer blocks: 2.51BulkMalfunctions
Blocks: 1345919
See you all in bug 1345919 ;-)
You need to log in before you can comment on or make changes to this bug.