1332173 - Rollup patch. Align nsSuiteApp.cpp with latest changes in nsBrowserApp.cpp

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Edmund Wong (:ewong)]

Bustage:

/usr/bin/ccache /builds/slave/c-cen-t-lnx/build/gcc/bin/gcc -m32 -march=pentium-m -std=gnu99 -o ldvector.o -c  -DNDEBUG -DTRIMMED=1 '-DSHLIB_SUFFIX="so"' '-DSHLIB_PREFIX="lib"' '-DSHLIB_VERSION="3"' '-DSOFTOKEN_SHLIB_VERSION="3"' -DRIJNDAEL_INCLUDE_TABLES -DMP_API_COMPATIBLE -DNSS_X86_OR_X64 -DNSS_X86 -DFREEBL_LOWHASH -DFREEBL_NO_DEPEND -DMP_IS_LITTLE_ENDIAN -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE -DMP_ASSEMBLY_DIV_2DX1D -DMP_USE_UINT_DIGIT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_LIBPKIX -I/builds/slave/c-cen-t-lnx/build/mozilla/security/nss/lib/freebl -I/builds/slave/c-cen-t-lnx/build/objdir/security/nss/lib/freebl/freebl_freeblpriv3 -I/builds/slave/c-cen-t-lnx/build/mozilla/security/nss/lib/freebl/mpi -I/builds/slave/c-cen-t-lnx/build/mozilla/security/nss/lib/freebl/ecl -I/builds/slave/c-cen-t-lnx/build/objdir/dist/include/nspr -I/builds/slave/c-cen-t-lnx/build/objdir/dist/include/private/nss -I/builds/slave/c-cen-t-lnx/build/objdir/dist/include/nss -I/builds/slave/c-cen-t-lnx/build/objdir/dist/include    -fPIC  -include /builds/slave/c-cen-t-lnx/build/objdir/mozilla-config.h -DMOZILLA_CLIENT -MD -MP -MF .deps/ldvector.o.pp  -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -msse -msse2 -mfpmath=sse -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe  -g -freorder-blocks -Os -fno-omit-frame-pointer    /builds/slave/c-cen-t-lnx/build/mozilla/security/nss/lib/freebl/ldvector.c
libnssckbi.so
rm -f libnssckbi.so
/builds/slave/c-cen-t-lnx/build/objdir/_virtualenv/bin/python /builds/slave/c-cen-t-lnx/build/mozilla/config/expandlibs_exec.py --uselist --  /usr/bin/ccache /builds/slave/c-cen-t-lnx/build/gcc/bin/g++ -m32 -march=pentium-m -std=gnu++11  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -msse -msse2 -mfpmath=sse -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -D_GLIBCXX_USE_CXX11_ABI=0 -pipe  -g -freorder-blocks -Os -fno-omit-frame-pointer  -fPIC -shared -Wl,-z,defs -Wl,--gc-sections -Wl,-h,libnssckbi.so -o libnssckbi.so  empty.o anchor.o bfind.o binst.o bobject.o bsession.o bslot.o btoken.o ckbiver.o constants.o certdata.o   -lpthread -L/builds/slave/c-cen-t-lnx/build/gtk3/usr/local/lib  -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id    -Wl,-rpath-link,/builds/slave/c-cen-t-lnx/build/objdir/dist/bin -Wl,-rpath-link,/usr/local/lib   ../../../../../../security/nss/lib/base/base_nssb/libnssb.a ../../../../../../security/nss/lib/ckfw/ckfw_nssckfw/libnssckfw.a  ../../../../../../config/external/nspr/pr/libnspr4.so ../../../../../../config/external/nspr/libc/libplc4.so ../../../../../../config/external/nspr/ds/libplds4.so -Wl,--version-script,out.nssckbi.def   -ldl  -lpthread -ldl -lc 
make[4]: Leaving directory `/builds/slave/c-cen-t-lnx/build/objdir/suite/app'
/builds/slave/c-cen-t-lnx/build/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../x86_64-unknown-linux-gnu/bin/as: /lib64/libz.so.1: no version information available (required by /builds/slave/c-cen-t-lnx/build/gcc/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../x86_64-unknown-linux-gnu/bin/as)
../../../suite/app/nsSuiteApp.cpp:181:14: error: 'nsDynamicFunctionLoad' does not name a type
 static const nsDynamicFunctionLoad kXULFuncs[] = {
              ^
../../../suite/app/nsSuiteApp.cpp: In function 'int do_main(int, char**, char**, nsIFile*)':
../../../suite/app/nsSuiteApp.cpp:321:49: error: invalid initialization of reference of type 'const mozilla::BootstrapConfig&' from expression of type 'mozilla::XREAppData'
   return XRE_main(argc, argv, appData, mainFlags);
                                                 ^
../../../suite/app/nsSuiteApp.cpp: In function 'nsresult InitXPCOMGlue(const char*, nsIFile**)':
../../../suite/app/nsSuiteApp.cpp:361:26: error: 'XPCOMGlueEnablePreload' was not declared in this scope
   XPCOMGlueEnablePreload();
                          ^
../../../suite/app/nsSuiteApp.cpp:363:32: error: 'XPCOMGlueStartup' was not declared in this scope
   rv = XPCOMGlueStartup(exePath);
                                ^
../../../suite/app/nsSuiteApp.cpp:369:34: error: 'kXULFuncs' was not declared in this scope
   rv = XPCOMGlueLoadXULFunctions(kXULFuncs);
                                  ^
../../../suite/app/nsSuiteApp.cpp:369:43: error: 'XPCOMGlueLoadXULFunctions' was not declared in this scope
   rv = XPCOMGlueLoadXULFunctions(kXULFuncs);
                                           ^
../../../suite/app/nsSuiteApp.cpp: In function 'int do_main(int, char**, char**, nsIFile*)':
../../../suite/app/nsSuiteApp.cpp:322:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^
cc1plus: all warnings being treated as errors
make[4]: *** [nsSuiteApp.o] Error 1
make[3]: *** [suite/app/target] Error 2

Comment 1

[Edmund Wong (:ewong)]

Attachment: proposed patch

Comment 2

[Jorg K (CEST = GMT+2)]

For easier review, may I suggest the method I used in bug 1332017 comment #2.

Comment 3

[Edmund Wong (:ewong)]

Attachment: proposed patch (v2)

not sure about kDesktopFolder..   was thinking of setting it to suite, but
I'm not sure.

Comment 4

[Edmund Wong (:ewong)]

Attachment: proposed patch (v3)

was told by :glandium that we don't need to set it.

Comment 5

[Hartmut Figge]

Well, (v3) gives me a successful SM-Trunk Linux x86_64. Sadly, it is not usable. ;) No content in any window.

Comment 6

[Hartmut Figge]

A new build many checkins later doesn't show the issue. Well, there is a problem with package, but unrelated. So, regarding (v3), I have to say WFM.

Comment 7

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8828264 [details] [diff] [review]
proposed patch (v3)

> -    appData.directory = appDir;
> +    config.appData = &sAppData;
>    }
  
appDataPath is missing in the else 

This works:
>      config.appDataPath = "";

This should also be supported and I think that's currently also the case without an assignment but I didn't try:
>      config.appDataPath = nullptr;


> +    return NS_ERROR_FAILURE;
> +  } 
>   // This will set this thread as the main thread.

NIT: blank at end of line

Otherwise great and good to go.

Comment 8

[Frank-Rainer Grahl (:frg)]

> was told by :glandium that we don't need to set it.

Was it supposed to left unitialized? Worked but I always fee uneasy doing this. IanN should decide but I would at least assign a nullptr.

FRG

Comment 9

[Edmund Wong (:ewong)]

(In reply to Frank-Rainer Grahl from comment #7)
> Comment on attachment 8828264 [details] [diff] [review]
> proposed patch (v3)
> 
> > -    appData.directory = appDir;
> > +    config.appData = &sAppData;
> >    }
>   
> appDataPath is missing in the else 

the appDataPath in the else is set to kDesktopFolder which is not
applicable to us.  That said, it probably would make sense to just
set kDesktopFolder to "" and have config.appDataPath = kDesktopFolder
if only to make it look like browser's version.

Comment 10

[Edmund Wong (:ewong)]

Attachment: proposed patch (v4)

Comment 11

[Frank-Rainer Grahl (:frg)]

Bug 1332523 just broke this again. I am on it. We can either merge the patches or open a separate bug.

Comment 12

[Frank-Rainer Grahl (:frg)]

Attachment: 1332173-BootstrapLibfuzzer.patch

Part 2 on top of ewongs patch. Please let me know if you prefer a separate patch/bug.

Comment 13

[Frank-Rainer Grahl (:frg)]

Looking at the patch I just noticed that I still have a config.appDataPath = ""; in it which is missing from ewongs patch. As stated by ewong I know its not needed/used for SeaMonkey.

mozilla/toolkit/xre/nsAndroidStartup.cpp has a config.appDataPath = nullptr; in the code. This could also be used here to show that we covered it and know about it.

Comment 14

[Frank-Rainer Grahl (:frg)]

Attachment: 1332173-BootstrapLibfuzzer.patch

One leading blank too much.

Comment 15

[Ian Neal]

Comment on attachment 8829041 [details] [diff] [review]
proposed patch (v4)

It would be best to try and sync with this with nsBrowserApp.cpp as much as possible so...

https://dxr.mozilla.org/comm-central/source/suite/app/nsSuiteApp.cpp#102
After:
 #ifdef XP_MACOSX
 #define kOSXResourcesFolder "Resources"
 #endif

can we add:
 #define kDesktopFolder ""

https://dxr.mozilla.org/comm-central/source/suite/app/nsSuiteApp.cpp#109
Can the #ifdef XP_WIN be reversed so that the following can be moved to the top of the #ifndef XP_WIN
vfprintf(stderr, fmt, ap);

The extra lines removed from:
https://dxr.mozilla.org/comm-central/source/suite/app/nsSuiteApp.cpp#111
https://dxr.mozilla.org/comm-central/source/suite/app/nsSuiteApp.cpp#255


>+++ b/suite/app/nsSuiteApp.cpp	Sat Jan 21 09:31:58 2017 +0800
>@@ -335,7 +257,7 @@
> }
> 
> static nsresult
>-InitXPCOMGlue(const char *argv0, nsIFile **xreDirectory)
>+InitXPCOMGlue(const char *argv0)
> {
>   char exePath[MAXPATHLEN];
> 
>@@ -357,41 +279,15 @@
>     return NS_ERROR_FAILURE;
>   }
> 
>-  // We do this because of data in bug 771745
>-  XPCOMGlueEnablePreload();
>-
>-  rv = XPCOMGlueStartup(exePath);
>-  if (NS_FAILED(rv)) {
>+  gBootstrap = mozilla::GetBootstrap(exePath);
>+  if (!gBootstrap) {
>     Output("Couldn't load XPCOM.\n");
>-    return rv;
>+    return NS_ERROR_FAILURE;
>   }
>-
>-  rv = XPCOMGlueLoadXULFunctions(kXULFuncs);
>-  if (NS_FAILED(rv)) {
>-    Output("Couldn't load XRE functions.\n");
>-    return rv;
>-  }
>-
Removed one too many empty lines here.

>@@ -423,38 +319,36 @@
>     }
> #endif
> 
>-    nsresult rv = InitXPCOMGlue(argv[0], nullptr);
>+    nsresult rv = InitXPCOMGlue(argv[0]);
Add an empty line here.
>     if (NS_FAILED(rv)) {
>       return 255;
>     }
> 

r/a=me with those addressed.

Comment 16

[Ian Neal]

Comment on attachment 8829204 [details] [diff] [review]
1332173-BootstrapLibfuzzer.patch

>+++ b/suite/app/moz.build

>+if CONFIG['LIBFUZZER']:
>     USE_LIBS += [ 'fuzzer' ]
>+    LOCAL_INCLUDES += [
>+        '/tools/fuzzing/libfuzzer',
>+    ]
Shouldn't this be /mozilla/tools/fuzzing/libfuxxer ?

>+++ b/suite/app/nsSuiteApp.cpp
>@@ -216,99 +208,65 @@ static int do_main(int argc, char* argv[
>   BootstrapConfig config;
> 
>   if (appDataFile && *appDataFile) {
>     config.appData = nullptr;
>     config.appDataPath = appDataFile;
>   } else {
>     // no -app flag so we use the compiled-in app data
>     config.appData = &sAppData;
>+    config.appDataPath = "";
See comments on other patch, make this:
+    config.appDataPath = kDesktopFolder;

r/a=me with both sets of comments addressed.

Probably better as one single patch.

Comment 17

[Frank-Rainer Grahl (:frg)]

Attachment: 1332173-nsSuiteApp.patch

Unified patch with all issues addressed. Further aligned with nsBrowserApp.cpp as per irc.

r+ from IanN carried forward.

Comment 18

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/0ac5cccaad09773d8c5be2ddd345643e9c450cac