flypig.co.uk

Having solved the mystery of the crashing browser, the question today is what to do about it. What I can't immediately understand is how in ESR 78 the value read off for the embedlite.compositor.external_gl_context preference is true when the browser is run, but false when the WebView app is run.

We can see the difference quite clearly from stepping through the AllocPLayerTransactionParent() method on ESR 78. First, this is what we see when running the browser. We can clearly observe that mUseExternalGLContext is set to true:

Thread 39 "Compositor" hit Breakpoint 2, mozilla::embedlite::
    EmbedLiteCompositorBridgeParent::AllocPLayerTransactionParent (
    this=0x7f809d5670, aBackendHints=..., aId=...)
    at mobile/sailfishos/embedthread/EmbedLiteCompositorBridgeParent.cpp:77
77        PLayerTransactionParent* p =
(gdb) n
80        EmbedLiteWindowParent *parentWindow = EmbedLiteWindowParent::From(
    mWindowId);
(gdb) p mUseExternalGLContext
$2 = true
(gdb) 

When stepping through the same bit of code when running harbour-webview, mUseExternalGLContext is set to false:

Thread 34 "Compositor" hit Breakpoint 1, mozilla::embedlite::
    EmbedLiteCompositorBridgeParent::AllocPLayerTransactionParent (
    this=0x7f8cb70d50, aBackendHints=..., aId=...)
    at mobile/sailfishos/embedthread/EmbedLiteCompositorBridgeParent.cpp:77
77        PLayerTransactionParent* p =
(gdb) n
80        EmbedLiteWindowParent *parentWindow = EmbedLiteWindowParent::From(
    mWindowId);
(gdb) n
81        if (parentWindow) {
(gdb) p mUseExternalGLContext
$1 = false
(gdb) 

How can this be? There must be somewhere it's being changed. I notice that the value is set in the embedding.js file. Could that be it?

// Make gecko compositor use GL context/surface provided by the application.
pref("embedlite.compositor.external_gl_context", false);
// Request the application to create GLContext for the compositor as
// soon as the top level PuppetWidget is creted for the view. Setting
// this pref only makes sense when using external compositor gl context.
pref("embedlite.compositor.request_external_gl_context_early", false);

The other thing I notice is that the value is set to true in declarativewebutils.cpp, which is part of the sailfish-browser code:

void DeclarativeWebUtils::setRenderingPreferences()
{
    SailfishOS::WebEngineSettings *webEngineSettings = SailfishOS::
    WebEngineSettings::instance();

    // Use external Qt window for rendering content
    webEngineSettings->setPreference(QString(
        "gfx.compositor.external-window"), QVariant(true));
    webEngineSettings->setPreference(QString(
        "gfx.compositor.clear-context"), QVariant(false));
    webEngineSettings->setPreference(QString(
        "gfx.webrender.force-disabled"), QVariant(true));
    webEngineSettings->setPreference(QString(
        "embedlite.compositor.external_gl_context"), QVariant(true));
}

And this wasn't a change made by me:

$ git blame apps/core/declarativewebutils.cpp -L239,239
d8932efa1 src/browser/declarativewebutils.cpp (Raine Makelainen 2016-09-19 20:
    16:59 +0300 239)     webEngineSettings->setPreference(QString(
    "embedlite.compositor.external_gl_context"), QVariant(true));

So the key methods that are of interest to us are WebEngineSettings::initialize() in the sailfish-components-webview project, since this is where I've set the value to false and DeclarativeWebUtils::setRenderingPreferences() in the sailfish-browser project, since this is where, historically, the value was always forced to true on browser start-up.

It turns out the browser startup sequence is a bit messy. The sequence it goes through (along with a whole bunch of other stuff) is to first call initialize(). This method has a guard inside it to prevent the content of the method being called multiple times and which looks like this:

    static bool isInitialized = false;
    if (isInitialized) {
        return;
    }

The first time the method is called the isInitialized static variable is set to false and the contents of the method executes. As per our updated code this sets the embedlite.compositor.external_gl_context static preference to false. This is all fine for the WebView. In the case of the browser the setRenderingPreferences() method is called shortly after, setting the value to true. This is all present and correct.

But now comes the catch. A little later on in the execution sequence initialize() is called again. At this point, were the isInitialized value set to true the method would return early and the contents of the method would be skipped. All would be good. But the problem is that it's not set to true, it's still set to false.

That's because the code that sets it to true is at the end of the method and on first execution the method is returning early due to a separate check; this one:

    // Guard preferences that should be written only once. If a preference 
    // needs to be
    // forcefully written upon each start that should happen before this.
    QString appConfig = QStandardPaths::writableLocation(QStandardPaths::
    AppDataLocation);
    QFile markerFile(QString("%1/__PREFS_WRITTEN__").arg(appConfig));
    if (markerFile.exists()) {
        return;
    }

This call causes the method to exit early so that the isInitialized variable never gets set.

The fact it's exiting early here makes sense. But I don't think it makes sense for the isInitialized variable to be left as it is. I think it should be set to true even if the method exits early at this point.

In case you want to see the full gory details for yourself (trust me: you don't!), here's the step through that shows this sequence of events:

$ gdb sailfish-browser
[...]
(gdb) b DeclarativeWebUtils::setRenderingPreferences
Breakpoint 1 at 0x42c48: file ../core/declarativewebutils.cpp, line 233.
(gdb) b WebEngineSettings::initialize
Breakpoint 2 at 0x22ce4
(gdb) r
Starting program: /usr/bin/sailfish-browser 
[...]
Thread 1 "sailfish-browse" hit Breakpoint 2, SailfishOS::
    WebEngineSettings::initialize () at webenginesettings.cpp:96
96          if (isInitialized) {
(gdb) p isInitialized
$4 = false
(gdb) n
[...]
135         if (markerFile.exists()) {
(gdb) 
136             return;
(gdb) c
Continuing.

Thread 1 "sailfish-browse" hit Breakpoint 1, DeclarativeWebUtils::
    setRenderingPreferences (this=0x55556785f0) at ../core/
    declarativewebutils.cpp:233
233         SailfishOS::WebEngineSettings *webEngineSettings = SailfishOS::
    WebEngineSettings::instance();
(gdb) c
Continuing.

Thread 1 "sailfish-browse" hit Breakpoint 2, SailfishOS::
    WebEngineSettings::initialize () at webenginesettings.cpp:96
96          if (isInitialized) {
(gdb) p isInitialized
$5 = false
(gdb) n
[...]
135         if (markerFile.exists()) {
(gdb) 
136             return;
(gdb) c
Continuing.
[...]

At any rate, it looks like I have some kind of solution: set the preference in both places, but make sure the isInitialized value gets set correctly by moving the place it's set to above the condition that returns early. Tomorrow I'll install the packages with this change and give it a go.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.