flypig.co.uk

Yesterday we fixed a problem causing the MOC to fail building, as well as refactoring some functions that had been renamed upstream. This morning I was happy to see that the build moved forwards beyond those errors. So today we're on to something new. Here's what we're dealing with:

153:38.02 In file included from Unified_cpp_js_src_jit14.cpp:47:
153:38.02 ${PROJECT}/gecko-dev/js/src/jit/arm64/vixl/MozCpu-vixl.cpp:
          In function ‘int membarrier( ’ was not declared in this scope
153:38.02      return syscall(__NR_membarrier, cmd, flags);
153:38.02                     ^~~~~~~~~~~~~~~
153:38.03 ${PROJECT}/gecko-dev/js/src/jit/arm64/vixl/MozCpu-vixl.cpp:52:20: note:
          suggested alternative: ‘membarrier’
153:38.03      return syscall(__NR_membarrier, cmd, flags);
153:38.03                     ^~~~~~~~~~~~~~~
153:38.03                     membarrier

The __NR_membarrier define comes from unistd.h with a value that's processor-specific. There's no mention of it in the header in the SDK, but a quick search on the Web makes the reason clear: the value was introduced in kernel version 4.3. Sailfish OS runs various different kernel versions; the one on my Xperia 10 III is 4.19:

[defaultuser@kolbe ~]$ uname -a
Linux kolbe 4.19.248 #1 SMP PREEMPT Fri Jul 7 13:17:23 UTC 2023 aarch64 GNU/Linux

However, the latest headers available in the SDK are for Linux kernel 3.18. Curious to know why, I asked mal on IRC:
 
So right now the headers aren't available, but they may be available in the future.

Looking through the header files for newer kernels I can see the value needed seems to be architecture-specific, but we're only interested in the aarch64 variant (because all of this code is gated with an __aarch64 define, so I'm wondering whether I can just get away with defining it.

This might be a really bad idea. On the other hand, looking through the code, there doesn't seem any way around it short of reverting the entire upstream commit. Adding the value in will allow the code to compile, but could cause havoc at run time in case I choose the wrong value and end up calling a random syscall.

However, mal also helpfully pointed out that there are kernel headers for Linux 5.4 available; apparently these are needed for some native Sailfish OS builds. Would these be safe to use?
 
For now I'm just going to define the value, since (assuming I've got the right one) this should also be safe for the same reasons. I'll shift to moving the newer headers in due course.

I shouldn't forget to do this, because I can make the situation clear in the commit/patch that I create. So I'm going to add the value in and maybe, just maybe, the newer linux headers will be introduced in a future SDK release. Thanks go to mal for helping with this. Thankfully the changes get the build to move on again.

Next we have another NS_LITERAL_STRING error similar to that we saw on Day 10.

190:14.47 mobile/sailfishos/components
190:18.48 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp: 
          In member function ‘virtual nsresult nsEmbedClipboard::SetData(
          nsITransferable*, nsIClipboardOwner*, int32_t)’:
190:18.48 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:64:30:
          error: ‘NS_LITERAL_STRING’ was not declared in this scope
190:18.48    root->SetPropertyAsAString(NS_LITERAL_STRING("data"), buffer);
190:18.48                               ^~~~~~~~~~~~~~~~~
190:18.49 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:64:30:
          note: suggested alternative: ‘NS_EXTERNAL_VIS’
190:18.49    root->SetPropertyAsAString(NS_LITERAL_STRING("data"), buffer);
190:18.49                               ^~~~~~~~~~~~~~~~~
190:18.49                               NS_EXTERNAL_VIS

There is a difference this time though: this is NS_LITERAL_STRING whereas then it was NS_LITERAL_CSTRING (note the extra "C"). It's because of that extra letter that my checks for other instances didn't throw anything up previously. But the fix is the same: we can simply change all these to either wrap them with u..._ns in the case of string literals, or using the nsLiteralString constructor otherwise (note again the slight change: last time it was nsLiteralCString with an extra "C").

Having worked through all these cases the build gets another step further. Next we have the following:

184:35.72 mobile/sailfishos/components
184:39.33 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:
          In member function ‘virtual nsresult nsEmbedClipboard::GetData(
          nsITransferable*, int32_t)’:
184:39.34 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:90:16:
          error: invalid use of incomplete type ‘class nsIThread’
184:39.34      rv = thread->ProcessNextEvent(true, &processedEvent);
184:39.34                 ^~
184:39.34 In file included from ${PROJECT}/obj-build-mer-qt-xr/dist/include
          /nsThreadUtils.h:13,
184:39.34                  from ${PROJECT}/gecko-dev/mobile/sailfishos/components
                           /nsClipboard.cpp:10:
184:39.34 ${PROJECT}/obj-build-mer-qt-xr/dist/include/MainThreadUtils.h:12:7:
          note: forward declaration of ‘class nsIThread’
184:39.34  class nsIThread;
184:39.34        ^~~~~~~~~
184:39.43 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos
          /components/nsClipboard.h:11,
184:39.43                  from ${PROJECT}/gecko-dev/mobile/sailfishos
          /components/nsClipboard.cpp: :
184:39.43 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:
          In instantiation of ‘void nsCOMPtr::assert_validity() [with T = nsIThread]’:
184:39.43 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:477:5:
          required from ‘nsCOMPtr::nsCOMPtr() [with T = nsIThread]’
184:39.43 ${PROJECT}/gecko-dev/mobile/sailfishos/components/nsClipboard.cpp:86:23:
          required from here
184:39.43 ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:436:21: error:
          static assertion failed: nsCOMPtr only works for types with IIDs.
          Either use RefPtr; add an IID to your type with 
          NS_DECLARE_STATIC_IID_ACCESSOR/NS_DEFINE_STATIC_IID_ACCESSOR; or make
          the nsCOMPtr point to a base class with an IID.
184:39.43      static_assert(1 < sizeof(TestForIID(nullptr)),
184:39.43                    ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Checking the nsIThread.h file the ProcessNextEvent() method is definitely there, so I'm guessing both these errors are consequences of the header file not being included. Now I've added it in, let's see if that helps.

We're left with a whole slew of errors. I've cut out most of them for brevity, but also because we're not going to be able to fix them all today: most we'll have to come back to tomorrow.

But the errors are actually really important: they're the most significant upstream change we've yet hit, and they deserve inclusion in full. But we'll come to that in due course. Here, at any rate, are the first few errors:

226:53.87 mobile/sailfishos
227:03.66 In file included from ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp:8:
227:03.66 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.h:29:7: error: cannot derive from
          ‘final’ base ‘mozilla::layers::CompositorBridgeParent’ in derived type 
          ‘mozilla::embedlite::EmbedLiteCompositorBridgeParent’
227:03.66  class EmbedLiteCompositorBridgeParent : public mozilla::layers::CompositorBridgeParent
227:03.66        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
227:03.66 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.h:58:16: error: ‘virtual void
           mozilla::embedlite::EmbedLiteCompositorBridgeParent
           ::CompositeToDefaultTarget(mozilla::layers::PCompositorBridgeParent
           ::VsyncId)’ marked ‘override’, but does not override
227:03.66    virtual void CompositeToDefaultTarget(VsyncId aId) override;
227:03.66                 ^~~~~~~~~~~~~~~~~~~~~~~~
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp: In constructor ‘mozilla::embedlite
          ::EmbedLiteCompositorBridgeParent::EmbedLiteCompositorBridgeParent
          (uint32_t, mozilla::layers::CompositorManagerParent*, mozilla
          ::CSSToLayoutDeviceScale, const TimeDuration&, const CompositorOptions&,
          bool, const IntSize&)’:
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp:60:16: error:
          ‘AddBoolVarCache’ is not a member of ‘mozilla::Preferences’
227:04.21    Preferences::AddBoolVarCache(&mUseExternalGLContext,
227:04.21                 ^~~~~~~~~~~~~~~
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp: In member function ‘void 
          mozilla::embedlite::EmbedLiteCompositorBridgeParent::PrepareOffscreen()’:
227:04.21 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread
          /EmbedLiteCompositorBridgeParent.cpp:111:5: error: ‘GLScreenBuffer’
          was not declared in this scope
227:04.21      GLScreenBuffer* screen = context->Screen();
227:04.21      ^~~~~~~~~~~~~~

[...]

That's actually only a quarter of the errors that were actually generated. There were a real lot of errors.

Starting from the top, the first problem is that the CompositorBridgeParent class is marked as final. Final classes are supposed to be classes that can't be inherited from; none of their constituent parts can be overridden. But EmbedLite wants EmbedLiteCompositorBridgeParent to subclass it.

This turns out not to be new, and there's already a patch 0018 with the title "Make it possible to extend CompositorBridgeParent" that removes the final annotation to allow this to happen.

The patch is really simple, just removing the final annotation from the class, and it applies without the need for any manual teasing. Nice! At least some of the other errors in the output above could well be caused by this one small change, although it's not clear that this will cover all of them. But the easiest way to clear this question up is to run the build again.

$ patch -d gecko-dev -p1 < rpm/0018-sailfishos-compositor-Make-it-possible-to-extend-Com.patch
patching file gfx/layers/ipc/CompositorBridgeParent.h
Hunk #1 succeeded at 308 (offset -4 lines).

It'll take another few hours for the build to complete and it's quite later here, so I'll have to wait to find out the result until the morning. For all the other posts, check out my full Gecko Dev Diary.