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

RESOLVED FIXED in seamonkey2.50

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ewong, Assigned: ewong)

Tracking

Trunk
seamonkey2.50
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.50 fixed)

Details

User Story

Rollup patch. Align nsSuiteApp.cpp with lates changes in nsBrowserApp.cpp.
Port Bug 1306327 [Build an XPCOM startup API specifically for the Firefox stub]
Port Bug 1330533 [Simplify LibFuzzer setup]
Port Bug 1332523 [Cleanup bootstrap API]

Attachments

(1 attachment, 6 obsolete attachments)

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
Summary: suite/app/nsSuiteApp.cpp: error: control reaches end of non-void function [-Werror=return-type] → Port |bug 1306327 - Build an XPCOM startup API specifically for the Firefox stub which doesn't need the XPCOM glue| to SeaMonkey
Posted patch proposed patch (obsolete) — Splinter Review
Attachment #8828218 - Flags: review?(iann_bugzilla)
For easier review, may I suggest the method I used in bug 1332017 comment #2.
Posted patch proposed patch (v2) (obsolete) — Splinter Review
not sure about kDesktopFolder..   was thinking of setting it to suite, but
I'm not sure.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8828256 - Flags: review?(iann_bugzilla)
Posted patch proposed patch (v3) (obsolete) — Splinter Review
was told by :glandium that we don't need to set it.
Attachment #8828218 - Attachment is obsolete: true
Attachment #8828256 - Attachment is obsolete: true
Attachment #8828218 - Flags: review?(iann_bugzilla)
Attachment #8828256 - Flags: review?(iann_bugzilla)
Attachment #8828264 - Flags: review?(iann_bugzilla)
Well, (v3) gives me a successful SM-Trunk Linux x86_64. Sadly, it is not usable. ;) No content in any window.
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 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.
Attachment #8828264 - Flags: feedback+
> 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
(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.
Posted patch proposed patch (v4) (obsolete) — Splinter Review
Attachment #8828264 - Attachment is obsolete: true
Attachment #8828264 - Flags: review?(iann_bugzilla)
Attachment #8829041 - Flags: review?(iann_bugzilla)
Bug 1332523 just broke this again. I am on it. We can either merge the patches or open a separate bug.
Part 2 on top of ewongs patch. Please let me know if you prefer a separate patch/bug.
Attachment #8829203 - Flags: feedback?(iann_bugzilla)
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.
One leading blank too much.
Attachment #8829203 - Attachment is obsolete: true
Attachment #8829203 - Flags: feedback?(iann_bugzilla)
Attachment #8829204 - Flags: feedback?(iann_bugzilla)
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.
Attachment #8829041 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #8829204 - Flags: review+
Attachment #8829204 - Flags: feedback?(iann_bugzilla)
Attachment #8829204 - Flags: feedback+
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Unified patch with all issues addressed. Further aligned with nsBrowserApp.cpp as per irc.

r+ from IanN carried forward.
Attachment #8829041 - Attachment is obsolete: true
Attachment #8829204 - Attachment is obsolete: true
Attachment #8829247 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
User Story: (updated)
Resolution: --- → FIXED
Summary: Port |bug 1306327 - Build an XPCOM startup API specifically for the Firefox stub which doesn't need the XPCOM glue| to SeaMonkey → Rollup patch. Align nsSuiteApp.cpp with latest changes in nsBrowserApp.cpp
You need to log in before you can comment on or make changes to this bug.