1289149 - Update app/nsMailApp.cpp

Status: RESOLVED FIXED

(Thunderbird :: Build Config, defect)

Description

[aleth [:aleth]]

Port Bug 1282980, Bug 1277155, Bug 1271574, Bug 1278528, Bug 1114647 (last part).

Comment 1

[aleth [:aleth]]

Try https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3d3139ba5a87d147ecd8e16a8f911225a6259157

Comment 2

[aleth [:aleth]]

Attachment: Update mail/app/nsMailApp.cpp, im/app/nsMain.cpp

Comment 3

[aleth [:aleth]]

Attachment: Update suite/app/nsSuiteApp.cpp

Looks like suite/ was a bit further behind, so there are a few more changes in here.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8774363 [details] [diff] [review]
Update mail/app/nsMailApp.cpp, im/app/nsMain.cpp

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

Drive-by comment, since you CC'ed me on the bug ;-)

::: mail/app/nsMailApp.cpp
@@ +44,5 @@
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/WindowsDllBlocklist.h"
>  
> +#if !defined(MOZ_WIDGET_COCOA) && !defined(MOZ_WIDGET_ANDROID) \
> +  && !(defined(XP_LINUX) && defined(MOZ_SANDBOX))

We want that sandbox stuff, do we?

@@ +175,5 @@
>      }
>  
>      char appEnv[MAXPATHLEN];
>      snprintf(appEnv, MAXPATHLEN, "XUL_APP_FILE=%s", argv[2]);
> +    if (putenv(strdup(appEnv))) {

Why is the strdup() required? IMHO it just leaks memory here or what am I missing?

@@ +339,5 @@
> +#ifdef MOZ_BROWSER_CAN_BE_CONTENTPROC
> +  // We are launching as a content process, delegate to the appropriate
> +  // main
> +  if (argc > 1 && IsArg(argv[1], "contentproc")) {
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)

Same question as above.

Comment 5

[aleth [:aleth]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #4)
> ::: mail/app/nsMailApp.cpp
> @@ +44,5 @@
> >  #include "mozilla/Telemetry.h"
> >  #include "mozilla/WindowsDllBlocklist.h"
> >  
> > +#if !defined(MOZ_WIDGET_COCOA) && !defined(MOZ_WIDGET_ANDROID) \
> > +  && !(defined(XP_LINUX) && defined(MOZ_SANDBOX))
> 
> We want that sandbox stuff, do we?

Keeping these files as close to the browser version as possible (for ease of future updating and debugging) beats not porting stuff which might be disabled by default in TB. Also, note we ship the sandbox because plugins ;)

> @@ +175,5 @@
> >      }
> >  
> >      char appEnv[MAXPATHLEN];
> >      snprintf(appEnv, MAXPATHLEN, "XUL_APP_FILE=%s", argv[2]);
> > +    if (putenv(strdup(appEnv))) {
> 
> Why is the strdup() required? IMHO it just leaks memory here or what am I
> missing?

See the description in https://hg.mozilla.org/mozilla-central/rev/fb084575df12

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #5)
> See the description in
> https://hg.mozilla.org/mozilla-central/rev/fb084575df12
Thanks, I read it. Maybe we should add a comment, since, as I said, the string is leaked, but apparently intentionally so.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8774363 [details] [diff] [review]
Update mail/app/nsMailApp.cpp, im/app/nsMain.cpp

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

I'm stealing Magnus' reviews big time ;-)
r=jorgk if you add a comment explaining this:
if (putenv(strdup(appEnv))) {

Comment 8

[Jorg K (CEST = GMT+2)]

In fact, I'd put:
// Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv
as a comment.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to aleth [:aleth] from comment #5)
> Keeping these files as close to the browser version as possible (for ease of
> future updating and debugging) beats not porting stuff which might be
> disabled by default in TB.

I very much agree here, we should rather aim to keep it the same as for browser, even if that means not adding comments to explain things. This is code that no one will likely change aside from bugs like these and I don't believe there is enough added value in adding the comment. If someone is really interested, he or she can check hg blame and read all about it here.

Comment 10

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/0b327ea0d79940cc149aa749a0bc40a6194c9850
Bug 1289149 - Update mail/app/nsMailApp.cpp, im/app/nsMain.cpp. r=clokep,jorgk

Comment 11

[aleth [:aleth]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #8)
> In fact, I'd put:
> // Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv
> as a comment.

I like this comment, and it would be good to have it, but unfortunately the right place to put it would be in m-c, for the reasons given in comment 9. That these files have to be forked at all is unfortunate.

Comment 12

[Jorg K (CEST = GMT+2)]

Can you ping Ratty a little harder here. Is this really resolved while SM hasn't landed its patch?

Comment 13

[aleth [:aleth]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #12)
> Can you ping Ratty a little harder here. Is this really resolved while SM
> hasn't landed its patch?

Good point.

Comment 14

[Philip Chee]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #12)
> Can you ping Ratty a little harder here. Is this really resolved while SM
> hasn't landed its patch?
I'm back. Sorry for the delay.

Comment 15

[Philip Chee]

Comment on attachment 8774365 [details] [diff] [review]
Update suite/app/nsSuiteApp.cpp

> +#include <io.h>
> +#include <fcntl.h>
(Looking at DXR/blame) No need for these. They were added for Windows Metro. Metro code was subsequently removed from Firefox. These are just remenants.

> +#include "../../ipc/contentproc/plugin-container.cpp"
Shouldn't this be:
+#include "../../mozilla/ipc/contentproc/plugin-container.cpp" ??

> -#ifdef XP_WIN
> +#ifndef XP_WIN
> +  vfprintf(stderr, fmt, ap);
> +#else
> ...
> -#else
> -  vfprintf(stderr, fmt, ap);
Um no please don't switch these around. We did it this way deliberately in Bug 791238.

> +#define kDesktopFolder ""
> ...
> -    SetStrongPtr(appData.directory, static_cast<nsIFile*>(greDir.get()));
> -    // xreDirectory already has a refcount from NS_NewLocalFile
> -    appData.xreDirectory = xreDirectory;
> +  nsCOMPtr<nsIFile> appSubdir;
> +  greDir->Clone(getter_AddRefs(appSubdir));
> +  appSubdir->Append(NS_LITERAL_STRING(kDesktopFolder));
> +
> +  SetStrongPtr(appData.directory, static_cast<nsIFile*>(appSubdir.get()));
> +  // xreDirectory already has a refcount from NS_NewLocalFile
> +  appData.xreDirectory = xreDirectory;
I think these changes are NOOP, Please revert.
(Looking at DXR/blame. I believe that this was added for Metro)

> -  // Allow seamonkey.exe to launch XULRunner apps via -app <application.ini>
> +  // Allow firefox.exe to launch XULRunner apps via -app <application.ini>
We are not Firefox the original is correct.

> -  if (!lastSlash || (size_t(lastSlash - exePath) > MAXPATHLEN - sizeof(XPCOM_DLL) - 1))
> +  if (!lastSlash || (size_t(lastSlash - exePath) > MAXPATHLEN -
> +sizeof(XPCOM_DLL) - 1))
Bad wrapping. Wrap lines properly or don't wrap.

Comment 16

[Philip Chee]

https://hg.mozilla.org/comm-central/rev/b3fffbba744f

Comment 17

[Philip Chee]

>> +#include <io.h>
>> +#include <fcntl.h>
> (Looking at DXR/blame) No need for these. They were added for Windows Metro.
> Metro code was subsequently removed from Firefox. These are just remenants.
Please file a bug to remove these from Firefox.

https://bugzilla.mozilla.org/buglist.cgi?list_id=13169256&short_desc=remove%20unnecessary%20includes&short_desc_type=allwordssubstr&query_format=advanced

>> +#include "../../ipc/contentproc/plugin-container.cpp"
> Shouldn't this be:
> +#include "../../mozilla/ipc/contentproc/plugin-container.cpp" ??
Please correct this Thunderbird and Instantbird too.

>> -  if (!lastSlash || (size_t(lastSlash - exePath) > MAXPATHLEN - sizeof(XPCOM_DLL) - 1))
>> +  if (!lastSlash || (size_t(lastSlash - exePath) > MAXPATHLEN -
>> +sizeof(XPCOM_DLL) - 1))
> Bad wrapping. Wrap lines properly or don't wrap.
Please correct this Thunderbird and Instantbird too.

Comment 18

[Jorg K (CEST = GMT+2)]

Aleth, can you please address Philip's comments in comment #17.

Comment 19

[aleth [:aleth]]

(In reply to Philip Chee from comment #17)
> >> +#include <io.h>
> >> +#include <fcntl.h>
> > (Looking at DXR/blame) No need for these. They were added for Windows Metro.
> > Metro code was subsequently removed from Firefox. These are just remenants.
> Please file a bug to remove these from Firefox.
> 
> https://bugzilla.mozilla.org/buglist.
> cgi?list_id=13169256&short_desc=remove%20unnecessary%20includes&short_desc_ty
> pe=allwordssubstr&query_format=advanced

Seems like a good catch, but can't you file your own bugs? ;)
 
> >> +#include "../../ipc/contentproc/plugin-container.cpp"
> > Shouldn't this be:
> > +#include "../../mozilla/ipc/contentproc/plugin-container.cpp" ??
> Please correct this Thunderbird and Instantbird too.

I'm not sure this is correct. If the #include is used (given the #ifdefs) and the path is incorrect, we'll get a build failure. We don't get one at the moment, so I'm tempted to leave it as is for now.
 
> >> -  if (!lastSlash || (size_t(lastSlash - exePath) > MAXPATHLEN - sizeof(XPCOM_DLL) - 1))
> >> +  if (!lastSlash || (size_t(lastSlash - exePath) > MAXPATHLEN -
> >> +sizeof(XPCOM_DLL) - 1))
> > Bad wrapping. Wrap lines properly or don't wrap.
> Please correct this Thunderbird and Instantbird too.

See earlier comments - I agree the wrapping is bad, but prefer to keep the file as close to m-c as possible as it's a forked file. The right way to fix the wrapping would be a m-c bug.

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #19)
> I'm not sure this is correct. If the #include is used (given the #ifdefs)
> and the path is incorrect, we'll get a build failure. We don't get one at
> the moment, so I'm tempted to leave it as is for now.

#if !defined(MOZ_WIDGET_COCOA) && !defined(MOZ_WIDGET_ANDROID) \
  && !(defined(XP_LINUX) && defined(MOZ_SANDBOX))
#define MOZ_BROWSER_CAN_BE_CONTENTPROC
#include "../../ipc/contentproc/plugin-container.cpp"
#endif

As far as I know, we don't define MOZ_SANDBOX we we'll never know whether Philip is right. Does S/M define it?

Anyway, I can think for myself ;-)
For the include to be right we need to go from
mail/app to mail/app/../../mozilla/ipc/contentproc to find plugin-container.cpp.

So please correct this, r=jorgk.

Also, I really see no point to maintain horrible wrapping:
  if (!lastSlash || (size_t(lastSlash - exePath) > MAXPATHLEN -
sizeof(XPCOM_DLL) - 1))

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Follow up: Update mail/app/nsMailApp.cpp, im/app/nsMain.cpp

Follow up:
1) removed unused includes and fixed indentation (see bug 1301987)
2) Corrected path to plugin container.

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to aleth [:aleth] from comment #19)
> See earlier comments - I agree the wrapping is bad, but prefer to keep the
> file as close to m-c as possible as it's a forked file. The right way to fix
> the wrapping would be a m-c bug.

The repeated review comments assert that there has not been an agreement on this. I don't believe it is constructive to discuss this with patches and code comments. How do we come to an agreement on this quickly?

I support the opinion that we should stay as close to m-c as possible, since this is a file that we almost never update on our own. Updating this file in the future is easier if there are little differences to m-c, even if this means keeping horrible wrapping and possibly useless code.

Jorg, how important is this to you? Could you possibly live with a few lines of badly formatted code? If not, would you be open to accepting this patch as is for now and filing a bug with a patch to fix the style in m-c?

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Sorry about misreading this bug. I missed that it had already been resolved and my question rather applies to Philip since he made the review comments. Anyway, the jist of it still stands: I'd prefer keeping the files close to m-c and would like to know if we can keep it that way with the alternative of fixing it in m-c if it really bugs someone.

Comment 24

[Jorg K (CEST = GMT+2)]

Philipp, looks like you haven't read the bug completely.
Bug 1301987 was filed for M-C and a patch already provided there (attachment 8790119 [details] [diff] [review]).
The path to the plugin container is wrong and should be fixed although we don't use sandboxing.

Comment 25

[Jorg K (CEST = GMT+2)]

I'll reopen this so Philipp can see that there is another patch awaiting review here.

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

Yes, you are totally right. Sorry for the noise!

Comment 27

[aleth [:aleth]]

Comment on attachment 8790121 [details] [diff] [review]
Follow up: Update mail/app/nsMailApp.cpp, im/app/nsMain.cpp

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

Thanks for adding the m-c bug!

Comment 28

[aleth [:aleth]]

Hmm, looks like we should port bug 1286306 and bug 1285356 as well. But that's for another bug

Comment 29

[Jorg K (CEST = GMT+2)]

Let's see how M-C reacts to bug 1301987. If it goes ahead, I'll land this, otherwise just the fix to the path. I'll keep an eye on it.

Comment 30

[Jorg K (CEST = GMT+2)]

M-C accepted the changes in bug 1301987, so this is good to go:
https://hg.mozilla.org/comm-central/rev/b9cd88a0878f