flypig.co.uk

You may recall that last night I had to leave the build running after fixing a font structure error. I was happy to hear during the Sailfish Community meeting on IRC this morning that there was some anticipation for the result (thanks ExTechOp!). So let's get to it...

When I returned to my laptop this morning, I found the following at the bottom of my console output:

128:15.56 ipc/chromium
128:19.37 make[4]: *** No rule to make target 'moc_message_pump_qt.cc',
          needed by 'moc_message_pump_qt.o'.  Stop.
128:19.37 make[3]: *** [${PROJECT}/gecko-dev/config/recurse.mk:72: ipc/chromium/target-objects] Error 2
128:19.38 make[2]: *** [${PROJECT}/gecko-dev/config/recurse.mk:34: compile] Error 2
128:19.38 make[1]: *** [${PROJECT}/gecko-dev/config/rules.mk:355: default] Error 2
128:19.38 make: *** [client.mk:65: build] Error 2
128:19.38 0 compiler warnings present.

Well, that's not a built library, but it is further than it was before! The build ran for over two hours before failing this time. But as it happens we also already saw this error before on Day 10. Grepping the patches, we can see that moc_message_pump_qt.cc actually appears in two patches: 0002 which we've already discussed at some length, and 0007 which is new:

$ grep -rIn "moc_message_pump_qt.cc" *
0002-sailfishos-qt-Bring-back-Qt-layer.-JB-50505.patch:2392:+        '!moc_message_pump_qt.cc',
0007-sailfishos-gecko-Disable-MOC-code-generation-for-mes.patch:25:- '!moc_message_pump_qt.cc',

The latter has the title "Disable MOC code generation for message_pump_qt". That's pretty encouraging, because it's precisely this MOC code generation that's causing difficulty.

In case you're curious and don't already know, MOC stands for "Meta Object Compiler". This is a Qt-thing, because Qt doesn't exactly use pure C++. More specifically, it uses a bunch of extensions (e.g. new keywords) that the C++ compiler doesn't understand. Before being compiled, Qt code is passed through the Meta Object Compiler in order to convert these extensions into your common-or-garden standard C++.

The files it generates are generally of the form moc_something.cc which is what we have here. Based on this, and looking at the error, it looks very much like the build makefile is asking for the object file moc_message_pump_qt.o to be generated. The make file is specifying that in order to generate the object file, it should build the C++ file moc_message_pump_qt.cc. But that file doesn't exist (or can't be found).

And that's because the file isn't currently being generated. Patch 0007 should help with this, not by generating the code needed, but rather by ensuring the object file isn't asked for at all.

And the patch applies first time without glitches!

$ patch -d gecko-dev -p1 < rpm/0007-sailfishos-gecko-Disable-MOC-code-generation-for-mes.patch 
patching file ipc/chromium/moz.build
Hunk #1 succeeded at 119 (offset 8 lines).
patching file ipc/chromium/src/base/message_pump_qt.cc
patching file ipc/chromium/src/base/message_pump_qt.h

Looking at the patch, there's some other non-MOC related stuff happening in there too, but it all looks pretty sensible and worth keeping.

The question now is, as always, will it build?!.

Sadly, the answer is not exactly. Even though it's an incremental build on top of the previous one it still runs for over two hours. But then bails out with this:

134:28.06 ipc/chromium
134:34.13 In file included from Unified_cpp_ipc_chromium0.cpp:65:
134:34.13 ${PROJECT}/gecko-dev/ipc/chromium/src/base/message_loop.cc:
          In constructor ‘MessageLoop::MessageLoop(MessageLoop::Type, nsIEventTarget*)’:
134:34.13 ${PROJECT}/gecko-dev/ipc/chromium/src/base/message_loop.cc:248:17: error:
          expected type-specifier
134:34.13      pump_ = new base::MessagePumpForUI();
134:34.13                  ^~~~

The full section of code causing the problem looks like this:

233 #if defined(OS_WIN)
234   // TODO(rvargas): Get rid of the OS guards.
235   if (type_ == TYPE_DEFAULT) {
236     pump_ = new base::MessagePumpDefault();
237   } else if (type_ == TYPE_IO) {
238     pump_ = new base::MessagePumpForIO();
239   } else {
240     DCHECK(type_ == TYPE_UI);
241     pump_ = new base::MessagePumpForUI();
242   }
243 #elif defined(OS_POSIX)
244   if (type_ == TYPE_UI) {
245 #  if defined(OS_MACOSX)
246     pump_ = base::MessagePumpMac::Create();
247 #  elif defined(OS_LINUX) || defined(OS_BSD)
248     pump_ = new base::MessagePumpForUI();
249 #  endif  // OS_LINUX
250   } else if (type_ == TYPE_IO) {
252     pump_ = new base::MessagePumpLibevent();
252   } else {
253     pump_ = new base::MessagePumpDefault();
254   }
255 #endif    // OS_POSIX

That's a lot of preprocessor directives.

Line 248 of this is the one that's failing, which we can see is already neatly wrapped in a check for Linux. So this is almost certainly the line we need to address. So why isn't it building? Once again, it looks like the solution is in the existing patches, this one being patch 0009 "Backport Embed MessageLoop constructor back". So the obvious next step is to apply this and restart the build.

Once again the patch applies cleanly.

$ patch -d gecko-dev -p1 < rpm/0009-sailfishos-gecko-Backport-Embed-MessageLoop-contruct.patch 
patching file ipc/chromium/src/base/message_loop.cc
Hunk #1 succeeded at 28 (offset 3 lines).
Hunk #2 succeeded at 178 (offset 9 lines).
patching file ipc/chromium/src/base/message_loop.h
patching file ipc/chromium/src/base/message_pump_qt.h

So off we go again with another build, hopefully not a two hour wait this time.

By this point you might be wondering "why don't you just apply all the patches first, and only then check the build?" As it happens, this is pretty much the strategy that was initially attempted for the jump from ESR 68 to ESR 78. The problem is there are a lot of patches. Getting through them all would take a very long time, and changes from one patch to the next are much harder to test if you don't already have a working build.

Moreover, there are far fewer patches that are necessary to get the code to build, compared to getting it running well on Sailfish OS. The upshot is that picking out only those patches that are absolutely necessary to get the build to pass turns out to be the better strategy.

There's a bunch of assumptions and suppositions in this reasoning, but even if the claim turns out not to be true, it's the path I've started down already. So I'm sticking to it. Sunk costs and all that.

Two hours later and the build reaches it's conclusion again with the following error:

133:43.62 In file included from PEmbedLiteViewChild.cpp:14,
133:43.62                  from UnifiedProtocols14.cpp:20:
133:43.62 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/embedlite
          /EmbedLiteViewChild.h:17:10  fatal error: nsIIdleServiceInternal.h:
          No such file or directory
133:43.62  #include "nsIIdleServiceInternal.h"
133:43.62           ^~~~~~~~~~~~~~~~~~~~~~~~~~
133:43.62 compilation terminated.

Let's see what that error relates to by checking the git log.

$ git log -- widget/nsIdleService.h
commit 8f54209ef6191bf7f25082ec10cf81786c04aeff
Author: Doug Thayer 
Date:   Mon Jul 20 16:06:59 2020 +0000

    Bug 1651165 - Rename idle service r=Gijs,geckoview-reviewers,snorp
    
    Differential Revision: https://phabricator.services.mozilla.com/D83413Id

From the diff we can see that the nsIdleService and nsIdleServiceInternal classes have been renamed to nsUserIdleService and nsUserIdleServiceInternal respectively. To fix this we need to update all references in the EmbedLite code to match these changes. Happily it's a pretty straightforward change.

However, since these changes included renaming of a generated header files, it was necessary to re-run the build from scratch. This time it takes longer — two hours fifty minutes — before it fails. That's good. The next error message is the following.

169:45.72 In file included from ${PROJECT}/obj-build-mer-qt-xr/dist/include
          /mozilla/embedlite/EmbedLiteViewChild.h:18,
169:45.72                  from PEmbedLiteViewChild.cpp:14,
169:45.72                  from UnifiedProtocols14.cpp:20:
169:45.72 ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/embedlite
          /BrowserChildHelper.h:16:10  fatal error: nsIWebBrowserChrome3.h:
          No such file or directory
169:45.72  #include "nsIWebBrowserChrome3.h"
169:45.72           ^~~~~~~~~~~~~~~~~~~~~~~~
169:45.72 compilation terminated.

Looking at the ESR 78 code I can see that there is an nsIWebBrowserChrome3.h file in this version. It's in the toolkit/components/browser directory. A quick check of the git log shows it was removed for Bugzilla Bug 1701668 (with the descriptive title of "Remove nsIWebBrowserChrome3 interface"). The commit message also helpfully points out the relevant upstream diff.

So nsIWebBrowserChrome3 is gone from upstream. The fix has to be one of two things. Either we revert the upstream change to bring nsIWebBrowserChrome3.h back, or we remove all references to nsIWebBrowserChrome3.h in the EmbedLite code as well. I need to figure out which is the right approach.

Grepping the code for instances of its use gives the following.

$ grep -rIn "nsIWebBrowserChrome3" *
embedding/embedlite/utils/BrowserChildHelper.cpp:728:
    BrowserChildHelper::GetWebBrowserChrome(nsIWebBrowserChrome3** aWebBrowserChrome)
embedding/embedlite/utils/BrowserChildHelper.cpp:735:
    BrowserChildHelper::SetWebBrowserChrome(nsIWebBrowserChrome3* aWebBrowserChrome)
embedding/embedlite/utils/BrowserChildHelper.h:16:
    #include "nsIWebBrowserChrome3.h"
embedding/embedlite/utils/BrowserChildHelper.h:148:
    nsCOMPtr mWebBrowserChrome;

This includes both our EmbedLite code and upstream's gecko code, and essentially means that only the Getter and Setter are left. That's a strong indication that the right move is to get rid of it entirely by just ripping out the Getters and Setters and being done with it.

There are still a bunch of calls to GetWebBrowserChrome() and SetWebBrowserChrome(), but they relate to a different type and so I'm expecting those shouldn't interact badly with the removal from BrowserChildHelper. But another build should make that clear.

It'll be another long build, so I won't now be finding out the answer to this until tomorrow.

I can't finish this post without mentioning the awesome work that direc85 has been doing. You may recall that two days ago I mentioned that he'd tracked down a plausible candidate for the gcc bug causing the swgl build to fail. I'm pleased to say that direct85 has continued his work, applying the patch to the gcc source and working through the steps needed to get it to rebuild. As I write this it's still a work-in-progress, but nevertheless progressing well. As part of my work for tomorrow I hope to test it out with the swgl build to see what happens.

Let's hope this validates direc85's impressive detective work. It'd be really great if this fixes things, so something to look forward to!

For all the other posts, check out my full Gecko Dev Diary.