1149127 - Stop exporting LoadInfo unnecessarily

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

[(no longer active)]

Attachment: Stop exporting LoadInfo unnecessarily

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

I don't know if we can, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c104 and also
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c105

In case you can make compiled code tests work I am happy to r+.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Just in case, here is the problem I was facing back when I added the MOZ_EXPORT:
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c71

Comment 4

[(no longer active)]

Hmm did you only hit this issue on Mac?  TestCSPParser can be built successfully with this patch.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

I think I remember I was facing that problem on linux as well as mac. Would be curious to know what has changed in the meantime to make this compile without MOZ_EXPORT. Anyway, maybe push to try - if it works - I green light the patch - didn't like the export in the first place but I think it was the best solution back when we added it :-)

Comment 6

[(no longer active)]

OK.  FWIW I have no idea how this could have ever worked, since we only mark the class MOZ_EXPORT, which means that code outside of libxul will _also_ see this class as to be exported, and not imported, which is probably what you want to happen...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8638e5db5cdb

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8585469 [details] [diff] [review]
Stop exporting LoadInfo unnecessarily

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> OK.  FWIW I have no idea how this could have ever worked, since we only mark
> the class MOZ_EXPORT, which means that code outside of libxul will _also_
> see this class as to be exported, and not imported, which is probably what
> you want to happen...
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8638e5db5cdb

Looks pretty green to me - thanks for updating - how did you come across this anyway?

Comment 8

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #6)
> > OK.  FWIW I have no idea how this could have ever worked, since we only mark
> > the class MOZ_EXPORT, which means that code outside of libxul will _also_
> > see this class as to be exported, and not imported, which is probably what
> > you want to happen...
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=8638e5db5cdb
> 
> Looks pretty green to me - thanks for updating - how did you come across
> this anyway?

Just spotted this by accident.  A MOZ_EXPORT on a class that can't be imported cannot be right.  :-)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec0efae7ec5a

Comment 10

[Kent James (:rkent)]

This patch broke my builds. I have a binary extension that recompiles portions of the comm-central code using external linkage, and apparently loadInfo is needed by portions of that.

The justification for this is just "Stop exporting LoadInfo unnecessarily"  Well it is necessary to me. Can we revert this?

Comment 11

[(no longer active)]

What's the extension in question?  We can definitely back out this patch but I'd like to add a comment saying what requires this.  Also, note that LoadInfo will change in other ways in bug 1149853 very soon now, so you may need to fix that extension regardless.

Comment 12

[Kent James (:rkent)]

The extension in question is ExQuilla, that in turn uses a glue extension called "New Account Types" (SkinkGlue) which is used to add new account types to Thunderbird (and is the place where the compile breaks). ExQuilla adds support for Exchange Web Services to Thunderbird. There is also a project planned this summer, perhaps under Fedora GSOC or perhaps under sponsorship from Thunderbird partner organizations, to use SkinkGlue to do a trial implementation of a new Email protocol called JMAP in Thunderbird.

Unfortunately due to the architecture of Thunderbird, the only practical way to add new account types through an extension is using a binary extension which recompiles core components using external linkage, hence the issue here. So anything that breaks the ability to recompile using external linkage breaks new account types in Thunderbird extensions. Yes this is a fragile situation, and one that we would like to get rid of and may eventually, but it is the current situation.

Comment 13

[(no longer active)]

(In reply to Kent James (:rkent) from comment #12)
> The extension in question is ExQuilla, that in turn uses a glue extension
> called "New Account Types" (SkinkGlue) which is used to add new account
> types to Thunderbird (and is the place where the compile breaks).

Is this SkinkGlue? http://hg.mozilla.org/users/kent_caspia.com/skinkglue/  I can't find any mention of LoadInfo in that code.  And I couldn't find the source code for ExQuilla.

Can you please attach the compiler error that you're getting, and attach the relevant source code?

Comment 14

[Kent James (:rkent)]

Skinkglue error:

137:52.33 nsMsgSendUtils.obj : error LNK2019: unresolved external symbol "public: __thiscall mozilla::LoadInfo::LoadInfo(class nsIPrincipal *,class nsIPrincipal *,class nsINode *,unsigned int,unsigned int,class nsIURI *)" (??0LoadInfo@mozilla@@QAE@PAVnsIPrincipal@@0PAVnsINode@@IIPAVnsIURI@@@Z) referenced in function "enum nsresult __cdecl NS_NewInputStreamChannelInternal(class nsIChannel * *,class nsIURI *,class nsIInputStream *,class nsACString const &,class nsACString const &,class nsINode *,class nsIPrincipal *,class nsIPrincipal *,unsigned int,unsigned int,class nsIURI *)" (?NS_NewInputStreamChannelInternal@@YA?AW4nsresult@@PAPAVnsIChannel@@PAVnsIURI@@PAVnsIInputStream@@ABVnsACString@@3PAVnsINode@@PAVnsIPrincipal@@5II1@Z)
137:52.33 
137:52.33 skinkglue.dll : fatal error LNK1120: 1 unresolved externals
137:52.33 
137:52.33 c:/tb/central/src/mozilla/config/rules.mk:812: recipe for target 'skinkglue.dll' failed

Skinkglue source:

https://bitbucket.org/rkentjames/skinkglue

But the relevant code is pretty straightforward. It is just recompiling mailnews code using external linkage. Here is the core of skinkglue's nsMsgSendUtils:

#include "../../../../mailnews/compose/src/nsMsgCompUtils.cpp"
#include "../../../../mailnews/compose/src/nsMsgCompFields.cpp"
#include "../../../../mailnews/compose/src/nsMsgAttachmentHandler.cpp"
#include "../../../../mailnews/compose/src/nsMsgPrompts.cpp"
#include "../../../../mailnews/compose/src/nsMsgSendReport.cpp"
#include "../../../../mailnews/compose/src/nsMsgSendPart.cpp"
#include "../../../../mailnews/compose/src/nsMsgCopy.cpp"
#ifdef XP_MACOSX
#include "../../../../mailnews/compose/src/nsMsgAppleDoubleEncode.cpp"
#include "../../../../mailnews/compose/src/nsMsgAppleEncode.cpp"
#endif 

#if !defined(MSQ_MOZ_17) && !defined(MSQ_MOZ_24) && !defined(MSQ_MOZ_28)
#include "../../../../mailnews/mime/src/MimeHeaderParser.cpp"
#include "../../../../mailnews/base/util/Services.cpp"
//#include "../../../../mailnews/mime/src/nsMimeConverter.cpp"
#endif

So the reference to loadgroup is in comm-central mailnews compose code.

Comment 15

[(no longer active)]

Wow, I cannot overstate how *insane* this is.  :(

I'll revert this patch since it only eliminates a few exported symbols from libxul but please note that the reason why this works now is by luck and if this prevents more future cleanups such as this one, the right thing to do would be to keep those cleanups on mozilla-central and ask you or the maintainer of this library to fix things properly.  IOW, please expect this to break on any day in ways that we can't hack around like this.

Comment 16

[Kent James (:rkent)]

It's not so insane when you realize that until a few months ago, there was a concerted effort led by Neil to make sure that mailnews code could be linked using external linkage, as that had some advantages in certain configurations. SkinkGlue was designed as a megahack that allowed javascript-based code to interact with the mailnews design where there was a base C++ class that had overrides to represent specific account types. One person's insanity is another person's brilliant insight.

There were plans to incorporate the SkinkGlue concepts into the core mailnews code in the TB 38 time frame that did not get done in time. There is a reasonable chance that this will occur though before the next major release, as I/we realize that there is no real commitment now to maintain external link compatibility of mailnews code.

All that I can ask is that you don't break this without a good reason, so your decision here to revert is appreciated.

Comment 17

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f7ab41e6cc8

Comment 18

[(no longer active)]

(In reply to Kent James (:rkent) from comment #16)
> It's not so insane when you realize that until a few months ago, there was a
> concerted effort led by Neil to make sure that mailnews code could be linked
> using external linkage, as that had some advantages in certain
> configurations. SkinkGlue was designed as a megahack that allowed
> javascript-based code to interact with the mailnews design where there was a
> base C++ class that had overrides to represent specific account types. One
> person's insanity is another person's brilliant insight.

Sorry, but it doesn't matter whether the mailnews code cold be linked externally.  The mozilla headers you #include in that code are for the most part internal only and they will remain so.  nsNetUtil.h (which is where this usage of the LoadInfo constructor comes from) is one of the few headers that is OK to include externally, which is why I'm reverting this patch as opposed to convince you to fix things on your side.  :-)

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8f7ab41e6cc8

Comment 20

[Nathan Froyd [:froydnj]]

I think we should reopen this; we have made the decision to stop exporting symbols from libxul, and these symbols account for nearly a third of the remaining exported symbols.

Comment 21

[(no longer active)]

Kent, is comment 14 still an issue?

Comment 22

[Nathan Froyd [:froydnj]]

[Tracking Requested - why for this release]: Same reason as bug 1336086 comment 5:

We removed some symbols from export in 53:

https://groups.google.com/forum/#!msg/mozilla.dev.platform/qB-9gJl9Jgs/O8LhMg1dBwAJ

this bug would remove more of them.  It seems better to have one release where we remove as many as possible all at once, rather than removing some in one release, some in another, and more in another.

Comment 23

[Kent James (:rkent)]

This is no longer an issue for exports, as Thunderbird in version 52 will not support binary extensions. We have not taken steps to actually disable it, but there will no longer be any attempt to maintain binary compatibility over major versions, and breaking changes (such as this one) are expected to occur, perhaps even in esr52 code.

The insanity of comment 15 continues, but that is now included in the core code as JsAccount so there are no longer issues of exported symbols.

Comment 24

[(no longer active)]

Thanks, so I'll revert the backout patch in comment 19.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec85a3bf653
Stop exporting LoadInfo unnecessarily; r=ckerschb

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ec85a3bf653

Comment 27

[Marcia Knous [:marcia]]

Tracking 53/54+ for the reasons in Comment 22.

Comment 28

[(no longer active)]

Comment on attachment 8585469 [details] [diff] [review]
Stop exporting LoadInfo unnecessarily

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression.
[User impact if declined]: See comment 22.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: It doesn't really require verification, it's just stopping to export some symbols.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This can break software that relies on the symbols that we are exporting, but we have made a conscious decision to break such software.
[String changes made/needed]: None.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8585469 [details] [diff] [review]
Stop exporting LoadInfo unnecessarily

Consolidating some changes to stop exporting symbols, let's uplift to 53.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8bd8cdf859ca