Closed Bug 1289149 Opened 3 years ago Closed 3 years ago

Update app/nsMailApp.cpp

Categories

(Thunderbird :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 51.0

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(3 files)

Attachment #8774363 - Flags: review?(mkmelin+mozilla)
Attachment #8774363 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Looks like suite/ was a bit further behind, so there are a few more changes in here.
Attachment #8774365 - Flags: review?(philip.chee)
Attachment #8774363 - Flags: review?(clokep) → review+
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.
(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
(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 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))) {
Attachment #8774363 - Flags: review?(mkmelin+mozilla) → review+
In fact, I'd put:
// Bug 1271574 - Purposefully leak the XUL_APP_FILE string passed to putenv
as a comment.
(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.
https://hg.mozilla.org/comm-central/rev/0b327ea0d79940cc149aa749a0bc40a6194c9850
Bug 1289149 - Update mail/app/nsMailApp.cpp, im/app/nsMain.cpp. r=clokep,jorgk
(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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
Can you ping Ratty a little harder here. Is this really resolved while SM hasn't landed its patch?
Flags: needinfo?(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?

Good point.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Flags: needinfo?(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.
Attachment #8774365 - Flags: review?(philip.chee) → review+
https://hg.mozilla.org/comm-central/rev/b3fffbba744f
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
>> +#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.
Aleth, can you please address Philip's comments in comment #17.
Flags: needinfo?(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.
Flags: needinfo?(aleth)
(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))
Summary: Update app/nsMainApp.cpp → Update app/nsMailApp.cpp
Follow up:
1) removed unused includes and fixed indentation (see bug 1301987)
2) Corrected path to plugin container.
Attachment #8790121 - Flags: review?(aleth)
(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?
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.
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.
I'll reopen this so Philipp can see that there is another patch awaiting review here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yes, you are totally right. Sorry for the noise!
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!
Attachment #8790121 - Flags: review?(aleth) → review+
Hmm, looks like we should port bug 1286306 and bug 1285356 as well. But that's for another bug
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.
M-C accepted the changes in bug 1301987, so this is good to go:
https://hg.mozilla.org/comm-central/rev/b9cd88a0878f
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.