1114686 - nsMsgCompose.cpp does not work using external linkage.

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Kent James (:rkent)]

Bug 1112413 created a dependency in nsMsgCompose on code in nsJSEnvironment.h, which is not legal for code compiled using external linkage. Overriding nsMsgCompose in a binary addon (such as ExQuilla) requires recompiling using external linkage.

Comment 1

[Kent James (:rkent)]

Attachment: 1114686.patch

Looking at the history of this, the forced garbage collection was added at the time that recycled windows were added in 2001 (13 years ago!). JS garbage collection has improved significantly in that time.

Doing some experiments, with the explicit GC call the release of the nsMsgCompose object occurs a few seconds after the compose window is hidden (that is, too late to actually rely on for critical resetting operations). With the attached patch, which just eliminates this GC call, the nsMsgCompose object is destroyed later (perhaps not for some tens of seconds or a few minutes) but it is ultimately destroyed by normal GC, which is contrary to the comment "But if we don't call it, the release will happen only when we physically close the window which will happen only on quit." So the original justification for this is no longer valid.

So I think we should just remove this problematic code, as GC has improved in 13 years so that it is no longer needed.

I can live without this in ExQuilla, as my infrastructure allows me to apply patches such as this one automatically prior to recompiling with external linkage. But leaving this in will complicate the Summit goal of allowing Compose to be implemented in an extension for rewrite testing. Plus it is a pity to break external linkage with this non-essential line of code.

Comment 2

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #1)
> I can live without this in ExQuilla, as my infrastructure allows me to apply
> patches such as this one automatically prior to recompiling with external
> linkage. But leaving this in will complicate the Summit goal of allowing
> Compose to be implemented in an extension for rewrite testing. Plus it is a
> pity to break external linkage with this non-essential line of code.

I'm not clear as to why nsMsgCompose needs to be reimplemented--I know nsMsgSend does. If you're talking about JSMime changes, those are only affecting the nsIMsgSend boundary; if you're talking about compose-in-a-tab or use-a-better-HTML-editor changes, then you'll likely need to reimplement a fair amount of nsMsgCompose anyways due to the implementation's strong ties to window caching.

Comment 3

[Kent James (:rkent)]

I think we can simplify this discussion to this: this line of code is completely unnecessary, and breaks a feature (external linkage compatibility) that a lot of work went into, and is useful in some contexts. That's why I think we should just remove it.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to Kent James (:rkent) from comment #3)
> I think we can simplify this discussion to this: this line of code is
> completely unnecessary, and breaks a feature (external linkage
> compatibility) that a lot of work went into, and is useful in some contexts.
> That's why I think we should just remove it.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1114686.patch

One hunk from Kent's patch. The other code he wanted to remove is no longer there.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/078dbe4f8871
remove unneeded include of nsJSEnvironment.h from nsMsgCompose.cpp. r=jorgk