1323424 - Port bug 1321593 to SeaMonkey - Refactor nsXREAppData so that -app and builtin share more codepaths, less manual memory management

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Frank-Rainer Grahl (:frg)]

+++ This bug was initially created as a clone of Bug #1323313 +++

+++ This bug was initially created as a clone of Bug #1321593 +++

Spinoff from bug 1306327 comment 7. Right now the normal Firefox startup path and the -app startup path are separate in annoying ways. Unifying those codepaths will make things better.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: xxx-hackywip.patch

Patch works and can be used to compile locally but is essentially a copy of the nsBrowserApp.cpp. kdesktopfolder needs to be removed from it because SeaMonkey doesn't use a browser dir. It also contains the sse2 notification which we should keep (need to look up the bug number). If no one takes the bug I can try to finish it over the weekend.

Comment 2

[Tony Mechelynck [:tonymec]]

2.50 is current trunk and, even though this is way above my fixing abilities, I think I can confirm my original impression (that the problem is multi-platform) by looking at the patch; hence a very slight bit of triageing.

(In reply to Frank-Rainer Grahl from comment #1)
> If no one takes the bug I can try to finish it over the weekend.

"/!\ Unassigned bug with patches attached", says Bugzilla. If you have attached a "working but imperfect" patch, that's at least half the work. To use the kind of language customary among some of my Moroccan-Turkish-Polish-Bulgarian-Congolese neighbours, "the other half will happen soon, in sha' Allah" (i.e. "if God wills it"; and of course, "Heaven helps those who help themselves"). ;-)

Comment 3

[Tony Mechelynck [:tonymec]]

Bug 1321593 comment #0 talks about annoying differences between "normal startup path and -app startup path" so I suppose this bug belongs in SeaMonkey::Startup_&_Profiles. Please correct me if I'm wrong.

Comment 4

[Philip Chee]

Comment on attachment 8818530 [details] [diff] [review]
xxx-hackywip.patch

> +#ifdef MOZ_LINUX_32_SSE2_STARTUP_ERROR
> +#include <cpuid.h>
> +#include "mozilla/Unused.h"
....
I think we need the rest of the changes from:
  https://hg.mozilla.org/mozilla-central/rev/09d9c83ae3a9
  Bug 1300843 - Print an error on 32-bit Linux in the absence of SSE2
You should spin this off into a separate bug.

> -#ifdef XP_WIN
> +#ifndef XP_WIN
Ah no Neil deliberately switched this round when he ported Bug 762310

> +  vfprintf(stderr, fmt, ap);
> +#else
>    char msg[2048];
> -
>    vsnprintf_s(msg, _countof(msg), _TRUNCATE, fmt, ap);
 
>    wchar_t wide_msg[2048];
>    MultiByteToWideChar(CP_UTF8,
>                        0,
>                        msg,
>                        -1,
>                        wide_msg,
> @@ -76,25 +125,23 @@ static void Output(const char *fmt, ... 
>  #else
>    // Linking user32 at load-time interferes with the DLL blocklist (bug 932100).
>    // This is a rare codepath, so we can load user32 at run-time instead.
>    HMODULE user32 = LoadLibraryW(L"user32.dll");
>    if (user32) {
>      decltype(MessageBoxW)* messageBoxW =
>        (decltype(MessageBoxW)*) GetProcAddress(user32, "MessageBoxW");
>      if (messageBoxW) {
> -      messageBoxW(nullptr, wide_msg, L"XULRunner", MB_OK
> -                                                 | MB_ICONERROR
> -                                                 | MB_SETFOREGROUND);
> +      messageBoxW(nullptr, wide_msg, L"SeaMonkey", MB_OK
Remains "XULRunner" according to Neil.

> +                                               | MB_ICONERROR
> +                                               | MB_SETFOREGROUND);
Vertical alignment off?

> +#ifdef LIBFUZZER
> +XRE_LibFuzzerSetMainType XRE_LibFuzzerSetMain;
> +XRE_LibFuzzerGetFuncsType XRE_LibFuzzerGetFuncs;
> +#endif
Bug 1289194 - Experimental LibFuzzer integration.
The libfuzzer changes are relatively self contained.
https://users.own-hero.net/~decoder/LinkerDiagram-v2.png
Please spin this off in another bug. Plus you need to port the other parts of Bug 1289194.

> kdesktopfolder needs to be removed from it because SeaMonkey doesn't use a browser dir.
Right!
> +    nsCOMPtr<nsIFile> appSubdir;
> +    greDir->Clone(getter_AddRefs(appSubdir));
> +    appSubdir->Append(NS_LITERAL_STRING(kDesktopFolder));
> +    appData.directory = appSubdir;

In our case our appSubdir is the same as our greDir, so perhaps:
    nsCOMPtr<nsIFile> appDir;
    greDir->Clone(getter_AddRefs(appDir));
    appData.directory = appDir;

Comment 5

[Hartmut Figge]

The patch gives me a successful build of SM-Trunk Linux x86_64. An unusable one, though, because of no bookmarks and empty windows in MailNews. May be unrelated to this bug.

Comment 6

[Frank-Rainer Grahl (:frg)]

Hartmut, try removing the kdesktopfolder stuff as ratty suggested. Will check it out tomorrow. If you can start the console see if any errors there are reported.

Comment 7

[Hartmut Figge]

Attachment: not_the_real_patch

Didn't know that Philip is ratty. Well, not_the_real_patch works.

Comment 8

[Frank-Rainer Grahl (:frg)]

Attachment: 1323424-nsXREAppData.patch

Needs to go on top of Bug 1324017 and Bug 1324021. Windows ok. Will try Linux next.

Comment 9

[Frank-Rainer Grahl (:frg)]

Attachment: 1323424-nsXREAppData.patch

Tested Linux x64 and Windows x64. This goes on top of Bug 1324017 and Bug 1324021 as previously stated. 

Unrelated changes to the bug here: 1 SprintfLiteral and 1 formatting change for line length to match nsBrowserApp.cpp.

Comment 10

[Stefan [:stefanh]]

Comment on attachment 8819535 [details] [diff] [review]
1323424-nsXREAppData.patch

I tested this together with the other patches and it looks like there are some issues:

1) When launching the browser with a fresh profile I get a browser window with the content area shrunked (vertically minimized, maybe 100px high or something like that).
2) Typing an address in the url bar and hitting enter results in this message in the console:
*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: content is null
Full stack: handleURLBarCommand/<@chrome://navigator/content/navigator.js:1768:7
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:452:5
fetch@resource://gre/modules/PlacesUtils.jsm:2192:12
promiseShortcutOrURI@chrome://navigator/content/navigator.js:1790:10
addToUrlbarHistory@chrome://communicator/content/contentAreaClick.js:217:5
handleURLBarCommand@chrome://navigator/content/navigator.js:1682:5
anonymous@chrome://global/content/autocomplete.xml line 1154 > Function:1:8
_fireEvent@chrome://global/content/autocomplete.xml:1155:28
finishAutoComplete@chrome://global/content/autocomplete.xml:701:13
processKeyPress@chrome://global/content/autocomplete.xml:818:15
onxblkeypress@chrome://global/content/autocomplete.xml:1316:8

*************************
3) Customizing doesn't work. I can open the sheet, but hitting "Done" doesn't close the sheet.

Note: I backed out everything, did 'hg update -r 325711', re-spinned and the issues disappeared.

Comment 11

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #10) 
> Note: I backed out everything, did 'hg update -r 325711', re-spinned and the
> issues disappeared.

Which means the revision before 1321593 (parent of https://hg.mozilla.org/mozilla-central/rev/a0e38abdcdd8).

Comment 12

[Frank-Rainer Grahl (:frg)]

Stefan,

I should have mentioned it. Where the fixes for Bug 1324119 and Bug 1324121 in the tree? If not could you please retry with them in.

Comment 13

[Stefan [:stefanh]]

Comment on attachment 8819535 [details] [diff] [review]
1323424-nsXREAppData.patch

Yeah, looks like it works fine now. To avoid confusion: Make sure you've pulled tip from https://hg.mozilla.org/chatzilla/.

Comment 14

[Philip Chee]

Comment on attachment 8819535 [details] [diff] [review]
1323424-nsXREAppData.patch

Your commit message should also say that your patch also ports parts of:
Bug 1286306 - Add an app info property exposing the state of the Windows dll blocklist
Bug 1302163 - Change code to use SprintfLiteral instead of snprintf
r=me a=me

Comment 15

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/9dff59feff428451f3cbf5d3fe0b7934153558d1

Comment 16

[Tony Mechelynck [:tonymec]]

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0 SeaMonkey/2.50a1" 
ID:20161224033348 en-US 
c-c:9dff59feff428451f3cbf5d3fe0b7934153558d1 
m-c:da22155a2dc30dafc93d70f38f6bb8a49248c80f

I don't use MailNews, but this new tinderbox-build (from ftp.m.o.) works for me AFAICT, with Navigator (i.e. browser) and ChatZilla (not the built-in version but the 0.9.93.2016122212 cZ nightly, current cZ "default" head).

I regard this fix as VERIFIED for en-US Linux x86_64. Please check it also on other systems and languages if possible.