Closed Bug 1636036 Opened 4 years ago Closed 4 years ago

GeckoView compilation is broken with "fatal error: 'mozilla/dom/ClientIPCTypes.h' file not found"

Categories

(GeckoView :: General, defect)

Unspecified
All
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: gk, Unassigned)

References

Details

After rebasing our patch set to 77.0b1 GeckoView compilation suddenly breaks with:

50:00.21 In file included from /var/tmp/build/geckoview-4ac6652a6a29/mobile/android/components/geckoview/GeckoViewExternalAppService.cpp:10:
50:00.21 /var/tmp/build/geckoview-4ac6652a6a29/obj-arm-linux-androideabi/dist/include/mozilla/dom/WindowGlobalParent.h:14:10: fatal error: 'mozilla/dom/ClientIPCTypes.h' file not found
50:00.21 #include "mozilla/dom/ClientIPCTypes.h"
50:00.21          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Looking at the patches in bug 1620657 nothing immediately jumps to mind, though. Here is the mozconfig file that worked so far:

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-arm-linux-androideabi
mk_add_options MOZ_APP_DISPLAYNAME="Tor Browser"
export MOZILLA_OFFICIAL=1
CC="clang"
CXX="clang++"

ac_add_options --with-android-min-sdk=16

ac_add_options --enable-optimize
ac_add_options --enable-official-branding

ac_add_options --enable-application=mobile/android
ac_add_options --target=arm-linux-androideabi
ac_add_options --with-android-ndk=/var/tmp/dist/android-toolchain/android-ndk
ac_add_options --with-android-sdk=/var/tmp/dist/android-toolchain/android-sdk-linux
ac_add_options --with-gradle=/var/tmp/dist/android-toolchain/gradle/gradle-5.1.1/bin/gradle

# We do not use Tor Launcher on Android:
ac_add_options --disable-tor-launcher

if [ -z "${TB_BUILD_WITH_UPDATER}" ]; then
# Because Google Play will likely be the primary distribution medium,
# we disable updating and rely on Google Play by default. The
# Developer Policy explicitly prohibits in-app updating:
#    An app distributed via Google Play may not modify, replace, or
#    update itself using any method other than Google Plays update
#    mechanism.
# https://play.google.com/about/privacy-security-deception/malicious-behavior/

    ac_add_options --disable-tor-browser-update
    ac_add_options --disable-verify-mar
fi

ac_add_options --enable-strip
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --disable-rust-debug
ac_add_options --disable-crashreporter
ac_add_options --disable-webrtc

ac_add_options --enable-proxy-bypass-protection

# Disable telemetry
ac_add_options MOZ_TELEMETRY_REPORTING=

Actually, bug 1620657 might be the wrong blocker bug as rev 2a080386c304 passes compilation of the problematic part. Let me bisect a bit more and then reset the dependency.

Have you tried a clobber? That header is generated from ClientIPCTypes.ipdlh so it sees like that may be the culprit.

Flags: needinfo?(gk)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #2)

Have you tried a clobber? That header is generated from ClientIPCTypes.ipdlh so it sees like that may be the culprit.

Yes, I've been building clobber builds. So, I dug a bit more into this issue and here is the problem. Until the patches for bug 1620657 landed the IPDL machinery got only conditionally activated as it was only conditionally needed, that is once MOZ_ANDROID_HISTORY was defined. With the patches for bug 1620657 part of that changed: the IPDL part is mandatory now while it is still only getting conditionally activated, which seems to be a bug. If someone like we unsets MOZ_ANDROID_HISTORY the error I encountered occurs. Something like

diff --git a/mobile/android/components/geckoview/moz.build b/mobile/android/components/geckoview/moz.build
index f4266be401df..27dcaa25ad37 100644
--- a/mobile/android/components/geckoview/moz.build
+++ b/mobile/android/components/geckoview/moz.build
@@ -17,7 +17,8 @@ if CONFIG['MOZ_ANDROID_HISTORY']:
     XPCOM_MANIFESTS += [
         'components.conf',
     ]
-    include('/ipc/chromium/chromium-config.mozbuild')
+
+include('/ipc/chromium/chromium-config.mozbuild')
 
 EXTRA_COMPONENTS += [
     'GeckoView.manifest',
-- 

fixes the issue.

Could we get that fixed and then backported to beta so that we have to carry one patch less around? Thanks!

Flags: needinfo?(gk) → needinfo?(snorp)

Why are you unsetting MOZ_ANDROID_HISTORY? I don't think Tor should need that. You can just ignore history events in the app.

Flags: needinfo?(snorp) → needinfo?(gk)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #4)

Why are you unsetting MOZ_ANDROID_HISTORY? I don't think Tor should need that. You can just ignore history events in the app.

Thanks for the tip. I am not sure what you mean by "just ignore history events in the app" but I looked a bit and asked around why we have that set. We disabled both MOZ_ANDROID_HISTORY and MOZ_PLACES to disable all history/bookmark/icon caching both in a local sqlite db and with Android integration. I guess we'll revisit that during our switch to Fenix.

But regardless of that I still think the current behavior is a bug worth fixing (just moving the include out of the if block is doing it for me and seems like the right thing to do anyway now). Or am I missing something here?

Flags: needinfo?(gk) → needinfo?(snorp)

(In reply to Georg Koppen from comment #5)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #4)

Why are you unsetting MOZ_ANDROID_HISTORY? I don't think Tor should need that. You can just ignore history events in the app.

Thanks for the tip. I am not sure what you mean by "just ignore history events in the app" but I looked a bit and asked around why we have that set. We disabled both MOZ_ANDROID_HISTORY and MOZ_PLACES to disable all history/bookmark/icon caching both in a local sqlite db and with Android integration. I guess we'll revisit that during our switch to Fenix.

GeckoView doesn't store any history on its own, it leaves that up to the app. So you shouldn't need any changes to GeckoView to avoid keeping history.

But regardless of that I still think the current behavior is a bug worth fixing (just moving the include out of the if block is doing it for me and seems like the right thing to do anyway now). Or am I missing something here?

Yeah, that looks reasonable to me.

Flags: needinfo?(snorp)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #6)

(In reply to Georg Koppen from comment #5)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #4)

Why are you unsetting MOZ_ANDROID_HISTORY? I don't think Tor should need that. You can just ignore history events in the app.

Thanks for the tip. I am not sure what you mean by "just ignore history events in the app" but I looked a bit and asked around why we have that set. We disabled both MOZ_ANDROID_HISTORY and MOZ_PLACES to disable all history/bookmark/icon caching both in a local sqlite db and with Android integration. I guess we'll revisit that during our switch to Fenix.

GeckoView doesn't store any history on its own, it leaves that up to the app. So you shouldn't need any changes to GeckoView to avoid keeping history.

Great. Is there any reason then why things like MOZ_ANDROID_HISTORY and MOZ_PLACES are left in the codebase given that Fennec is gone? If not is there a bug we could track to have an overview which of those Fennec related things are still in mozilla-central but should go/get cleaned up at some point?

Flags: needinfo?(snorp)

I think MOZ_PLACES is stil needed on desktop, but I filed Bug 1636954 to remove MOZ_ANDROID_HISTORY.

Flags: needinfo?(snorp)
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.