Closed Bug 1582937 Opened 5 months ago Closed 5 months ago

Remove header dependencies between mozglue/dllservices and browser/app/winlauncher

Categories

(Core :: mozglue, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- fixed

People

(Reporter: jorgk-bmo, Assigned: aklotz)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 obsolete files)

Pretty busted right now, and this
https://hg.mozilla.org/mozilla-central/rev/8e2da9ff5f5aace0988f8d358aafdba04cddf950#l1.12
is just the beginning.

The main issue is:
0:08.66 c:/mozilla-source/comm-central/mozglue/dllservices/LoaderObserver.h(11,10): fatal error: 'mozilla/NtLoaderAPI.h' file not found
0:08.66 #include "mozilla/NtLoaderAPI.h"

And also:
0:09.45 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/glue/WindowsDllServices.h(12,10): fatal error: 'mozilla/LoaderAPIInterfaces.h' file not found
0:09.45 #include "mozilla/LoaderAPIInterfaces.h"

Attached patch 1582937-C-C.patch (obsolete) — Splinter Review
Assignee: nobody → jorgk
Attached patch 1582937-M-C.patch (obsolete) — Splinter Review

If I hard-code some some paths, I can get it to compile (so far). So something has gone wrong with some Makefile or build magic :-(

Flags: needinfo?(rob)
Flags: needinfo?(mhowell)
Flags: needinfo?(aklotz)

I wonder whether this affects Windows only. I'll push the C-C patch and retrigger the Dailies.

Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1eb948124d13
Port bug 1542830: adjustments to mail/app/nsMailApp.cpp. rs=bustage-fix DONTBUILD

Hmm, the M-C patch also doesn't let me compile, after a while I get:

0:13.19 In file included from c:/mozilla-source/comm-central/toolkit/crashreporter/minidump-analyzer/minidump-analyzer.cpp:30:
0:13.19 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\mozilla/glue/WindowsDllServices.h(12,10): fatal error: '../../browser/app/winlauncher/freestanding/LoaderAPIInterfaces.h' file not found
0:13.19 #include "../../browser/app/winlauncher/freestanding/LoaderAPIInterfaces.h"

Strangely the automation run is into 19 minutes and hasn't failed. So is it just me?

Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

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

::: mail/app/nsMailApp.cpp
@@ +24,5 @@
>  #include "nsIFile.h"
>  
>  #ifdef XP_WIN
> +#  include "LauncherProcessWin.h"
> +#  include "mozilla/WindowsDllBlocklist.h"

BTW, # include "LauncherProcessWin.h" was added to M-C here:
https://hg.mozilla.org/mozilla-central/rev/eb5c30217df9#l5.12
Looks like we didn't port it back then, so I've added it now to align with M-C again.
Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

This should be good for review.
Attachment #9094383 - Flags: review?(richard.marti)

OK, a clobber build got me further, but like in automation it fails at the end:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=1eb948124d131b9e064c5ea44750a03a666544c6&selectedJob=267794793
z:/task_1569052759/build/src/obj-thunderbird/dist/include\mozilla/glue/WindowsDllServices.h(12,10): fatal error: 'mozilla/LoaderAPIInterfaces.h' file not found

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267794793&repo=comm-central&lineNumber=14762

Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

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

::: mail/app/nsMailApp.cpp
@@ +23,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsIFile.h"
>  
>  #ifdef XP_WIN
> +#  include "LauncherProcessWin.h"

Doesn't this need also the other files added by bug 1454745 and the change in moz.build?

Hmm, you mean browser/app/LauncherProcessWin.cpp/h and

+    SOURCES += [
+        'LauncherProcessWin.cpp',
+    ]

Good question. If we don't port that, I guess adding the include file also doesn't make sense.

But that's just a secondary issue. Right now, we need to fix the bustage. BTW, to get going again locally, I did:
hg qbackout -s -r da7e775c4051fa5803873a409da0ffaf5cb0a864:6fcb417f7ff4048f0237e4949639d305198e20ad
to backout bug 1542830 so I can work again. Sadly, there is more "weekend bustage" I need to take care of.

Summary: Port bug 1542830 - TB build busted on 2019-09-21 → Port bug 1542830 - TB Windows build busted on 2019-09-21

Looking at this again, the basic issue is that source code in mozglue/dllservices relies on code in browser/app/winlauncher. That won't work for TB. I tried copying the entire winlauncher directory to mail/app, but had no success compiling it so far.

Attached patch patch-to-copy-winlauncher.patch (obsolete) — Splinter Review

OK, some tweaks to moz.build + a copy of browser's winlauncher. It doesn't compile.

Comment on attachment 9094383 [details] [diff] [review]
1582937-C-C.patch

Yes, I'll remove the include again since we don't have app\winlauncher\LauncherProcessWin.h so far.
Attachment #9094383 - Attachment is obsolete: true
Attachment #9094383 - Flags: review?(richard.marti)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2bd686fea9ae
Remove include of app/winlauncher/LauncherProcessWin.h again which Thunderbird doesn't have. r=me
Attached patch llvm-dlltool-commmail.patch (obsolete) — Splinter Review

mach configure completed and the build is progressing.

Flags: needinfo?(rob)
Attached patch patch-to-copy-winlauncher.patch (obsolete) — Splinter Review

OK, this should work together with Rob's llvm-dlltool-commmail.patch for M-C.

As the commit message says: Copy browser/app/winlauncher to mail/app/winlauncher.

Attachment #9094384 - Attachment is obsolete: true
Attachment #9094407 - Attachment is obsolete: true

So, with both patches together, TB compiles and runs. Time to ask for reviews.

Flags: needinfo?(mhowell)
Flags: needinfo?(aklotz)
Comment on attachment 9094410 [details] [diff] [review]
llvm-dlltool-commmail.patch

Whoever comes first. This is pretty urgent.
Attachment #9094410 - Flags: review?(mhowell)
Attachment #9094410 - Flags: review?(aklotz)
Comment on attachment 9094411 [details] [diff] [review]
patch-to-copy-winlauncher.patch

Straight copy with some build stuff also copied. Can we improve the
```
+    LOCAL_INCLUDES += [
+        '/comm/mail/app/winlauncher',
+    ]
```

Oh yes, there are some Firefox strings I should change to Thunderbird.
Attachment #9094411 - Flags: review?(rob)
Attachment #9094411 - Flags: feedback?(mhowell)
Attachment #9094411 - Flags: feedback?(aklotz)
Comment on attachment 9094410 [details] [diff] [review]
llvm-dlltool-commmail.patch

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

You shouldn't need any of this stuff for Thunderbird. Let's sort out the parts that you actually need to build instead of pulling in the world.
Attachment #9094410 - Flags: review?(mhowell)
Attachment #9094410 - Flags: review?(aklotz)
Attachment #9094410 - Flags: review-
Comment on attachment 9094411 [details] [diff] [review]
patch-to-copy-winlauncher.patch

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

You should not need to pull in the world from the launcher process to get Thunderbird to build. Let's isolate the stuff that is actually needed and just take care of that.
Attachment #9094411 - Flags: feedback?(mhowell)
Attachment #9094411 - Flags: feedback?(aklotz)
Attachment #9094411 - Flags: feedback-

Also, in Firefox we build xpcshell.exe and plugin-container.exe just fine without linking them against launcher process libs, so there's no reason why we need to do that for thunderbird.exe. We just need to sort out mozglue's header dependencies.

I whole-heartedly agree with comment #21 to comment #23. I don't want to fork that stuff and maintain it. I much prefer a "small" solution. Can you give us a hand with "We just need to sort out mozglue's header dependencies"?

Can you try my patch in https://phabricator.services.mozilla.com/D46715 and see if I got everything?

Flags: needinfo?(jorgk)

Compiles for me, thanks!

Flags: needinfo?(jorgk)
Attachment #9094410 - Attachment is obsolete: true
Attachment #9094411 - Attachment is obsolete: true
Attachment #9094411 - Flags: review?(rob)

Moving to mozglue.

Assignee: jorgk → aklotz
Component: General → mozglue
Keywords: leave-openregression
Product: MailNews Core → Core
Summary: Port bug 1542830 - TB Windows build busted on 2019-09-21 → Remove header dependencies between mozglue/dllservices and browser/app/winlauncher
Priority: -- → P1
Status: NEW → ASSIGNED
Attachment #9094418 - Attachment is obsolete: true

I'll fold this into the next landing of bug 1542830

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.