flypig.co.uk

It's my last dev diary before taking a 14 day break from gecko development today. I'm not convinced that I've made the right decision: there's a part of me that thinks I should forge on ahead and just make all of the things fit into the time I have available. But there's also the realist in me that says something has to give.

So there will be no dev diary tomorrow or until the 8th February, at which point I'll start up again. Ostensibly the reason is so that I can get my presentations together for FOSDEM. I want to do a decent job with the presentations. But I also have a lot going on at work right now. So it's necessary.

But there's still development to do today. Yesterday I set the build running after having added /browser/components/sessionstore to the embedlite/moz.build file. I was hoping this would result in SessionStore.jsm and its dependencies being added to omni.ja.

The package built fine. But after installing it on my device, the new files weren't to be found: they didn't make it into the archive. Worse than that, they've not even made it into the obj-build-mer-qt-x folder on my laptop. That means they're still not getting included in the build.

It rather makes sense as well. The LOCAL_INCLUDES value should, if I'm understanding correctly, list places where C++ headers might be found. This should affect JavaScript files at all.

So I've spent the day digging around in the build system trying to figure out what needs to be changed to get them where they need to be. I'm certain there's an easy answer, but I just can't seem to figure it out.

I thought about trying to override SessionStore.jsm as a component, but since it doesn't actually seem to be a component itself, this didn't work either.

So after much flapping around, I've decided just to patch out the functionality from the SessionStoreFunctions.jsm file. That doesn't feel like the right way to do this, but until someone suggests a better way (which I'm all open to!) this should at least be a pretty simple fix.

Let's see.

I've built a new version of gecko-dev with the changes applied, installed them on my phone and it's now time to run them.

$ sailfish-browser 
[D] unknown:0 - Using Wayland-EGL
library "libGLESv2_adreno.so" not found
library "eglSubDriverAndroid.so" not found
greHome from GRE_HOME:/usr/bin
libxul.so is not found, in /usr/bin/libxul.so
Created LOG for EmbedLiteTrace
[D] onCompleted:105 - ViewPlaceholder requires a SilicaFlickable parent
Created LOG for EmbedLite
Created LOG for EmbedPrefs
Created LOG for EmbedLiteLayerManager
JavaScript error: chrome://embedlite/content/embedhelper.js, line 259:
    TypeError: sessionHistory is null
Call EmbedLiteApp::StopChildThread()
Redirecting call to abort() to mozalloc_abort

The log output is encouragingly quiet. There is one error stating that sessionHistory is null. I think this is unrelated to the SessionStore changes I've made, but it's still worth looking into this to fix it. Maybe this will be what fixes the Back and Forwards buttons?

What's clear is that the SessionStore errors have now gone, which is great. But fixing those errors sadly hasn't fixed the Back and Forwards buttons.

Let's look at this other error then. It's caused by the last line in this code block:

      case "embedui:addhistory": {
        // aMessage.data contains: 1) list of 'links' loaded from DB, 2) current
        // 'index'.

        let docShell = content.docShell;
        let sessionHistory = docShell.QueryInterface(Ci.nsIWebNavigation).
            sessionHistory;
        let legacyHistory = sessionHistory.legacySHistory;

The penultimate line is essentially saying "View the docShell object as a WebNavigation object and access the sessionHistory value stored inside it".

So there are three potential reasons why this might be failing. First it could be that docShell no longer supports the WebNavigation interface. Second it could be that WebNavigation has changed so it no longer contains a sessionHitory value. Third it could be that the value is still there, but it's set to null.

From the nsDocShell class definition in nsDocShell.h it's clear that the interface is still supported:

class nsDocShell final : public nsDocLoader,
                         public nsIDocShell,
                         public nsIWebNavigation,
                         public nsIBaseWindow,
                         public nsIRefreshURI,
                         public nsIWebProgressListener,
                         public nsIWebPageDescriptor,
                         public nsIAuthPromptProvider,
                         public nsILoadContext,
                         public nsINetworkInterceptController,
                         public nsIDeprecationWarner,
                         public mozilla::SupportsWeakPtr {

So let's check that WebNavigation interface, defined in the nsIWebNavigation.idl file. The field is still there in the interface definition:

  /**
   * The session history object used by this web navigation instance. This
   * object will be a mozilla::dom::ChildSHistory object, but is returned as
   * nsISupports so it can be called from JS code.
   */
  [binaryname(SessionHistoryXPCOM)]
  readonly attribute nsISupports sessionHistory;

Although the interface is being accessed from nsDocShell, when we look at the code we can see that the history itself is coming from elsewhere:

  mozilla::dom::ChildSHistory* GetSessionHistory() {
    return mBrowsingContext->GetChildSessionHistory();
  }
[...]
  RefPtr<mozilla::dom::BrowsingContext> mBrowsingContext;

This provides us with an opportunity, because it means we can place a breakpoint here to see what it's doing.

$ gdb sailfish-browser
[...]
(gdb) b nsDocShell::GetSessionHistory
Breakpoint 1 at 0x7fbc7b37c4: nsDocShell::GetSessionHistory. (10 locations)
(gdb) b BrowsingContext::GetChildSessionHistory
Breakpoint 2 at 0x7fbc7b376c: file docshell/base/BrowsingContext.cpp, line 3314.
(gdb) c
Thread 10 "GeckoWorkerThre" hit Breakpoint 1, nsDocShell::GetSessionHistory
    (this=0x7f80aa9280)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/RefPtr.h:313
313     ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/RefPtr.h:
    No such file or directory.
(gdb) c
Continuing.

Thread 10 "GeckoWorkerThre" hit Breakpoint 2, mozilla::dom::BrowsingContext::
    GetChildSessionHistory (this=0x7f80c58e90)
    at docshell/base/BrowsingContext.cpp:3314
3314    ChildSHistory* BrowsingContext::GetChildSessionHistory() {
(gdb) b mChildSessionHistory
Function "mChildSessionHistory" not defined.
Make breakpoint pending on future shared library load? (y or [n]) n
(gdb) p mChildSessionHistory
$1 = {mRawPtr = 0x0}
(gdb) 

So this value is unset, but we'd expect it to be set as a consequence of a call to CreateChildSHistory():

void BrowsingContext::CreateChildSHistory() {
  MOZ_ASSERT(IsTop());
  MOZ_ASSERT(GetHasSessionHistory());
  MOZ_DIAGNOSTIC_ASSERT(!mChildSessionHistory);

  // Because session history is global in a browsing context tree, every process
  // that has access to a browsing context tree needs access to its session
  // history. That is why we create the ChildSHistory object in every process
  // where we have access to this browsing context (which is the top one).
  mChildSessionHistory = new ChildSHistory(this);

  // If the top browsing context (this one) is loaded in this process then we
  // also create the session history implementation for the child process.
  // This can be removed once session history is stored exclusively in the
  // parent process.
  mChildSessionHistory->SetIsInProcess(IsInProcess());
}

As I'm looking through this code in ESR 91 and ESR 78 I notice that the above method has changed: the call to SetIsInProcess() is new. I wonder if that will ultimately be related to why this isn't being set? I'm thinking that the location where the creation happens may be different.

There are indeed some differences. In ESR 91 it's called in BrowsingContext::CreateFromIPC() and BrowsingContext::Attach() whereas in ESR 78 it's called in BrowsingContext::Attach(). Both versions also have it being called from BrowsingContext::DidSet().

I should put some breakpoints on those methods to see which, if any, is being called. And I should do it for both versions.

See here's the result for ESR 91:

$ gdb sailfish-browser 
[...]
(gdb) b BrowsingContext::CreateChildSHistory
Function "BrowsingContext::CreateChildSHistory" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (BrowsingContext::CreateChildSHistory) pending.
(gdb) r
[...]

The breakpoint is never hit and the creation never occurs. In contrast, on ESR 78 we get a hit before the first page has loaded:

$ gdb sailfish-browser
[...]
(gdb) b BrowsingContext::CreateChildSHistory
Function "BrowsingContext::CreateChildSHistory" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (BrowsingContext::CreateChildSHistory) pending.
(gdb) r
[...]

Thread 8 "GeckoWorkerThre" hit Breakpoint 1, mozilla::dom::BrowsingContext::
    CreateChildSHistory (this=this@entry=0x7f889dc120)
    at obj-build-mer-qt-xr/dist/include/mozilla/cxxalloc.h:33
33      obj-build-mer-qt-xr/dist/include/mozilla/cxxalloc.h:
    No such file or directory.
(gdb) bt
#0  mozilla::dom::BrowsingContext::CreateChildSHistory
    (this=this@entry=0x7f889dc120)
    at obj-build-mer-qt-xr/dist/include/mozilla/cxxalloc.h:33
#1  0x0000007fbc7c933c in mozilla::dom::BrowsingContext::DidSet
    (aOldValue=<optimized out>, this=0x7f889dc120)
    at docshell/base/BrowsingContext.cpp:2356
#2  mozilla::dom::syncedcontext::Transaction<mozilla::dom::BrowsingContext>::
    Apply(mozilla::dom::BrowsingContext*)::{lambda(auto:1)#1}::operator()
    <std::integral_constant<unsigned long, 37ul> >(std::integral_constant
    <unsigned long, 37ul>) const (this=<optimized out>, this=<optimized out>,
    idx=...)
    at obj-build-mer-qt-xr/dist/include/mozilla/dom/SyncedContextInlines.h:137
[...]
#8  0x0000007fbc7cb484 in mozilla::dom::BrowsingContext::SetHasSessionHistory
    <bool> (this=this@entry=0x7f889dc120, 
    aValue=aValue@entry=@0x7fa6e1da57: true)
    at obj-build-mer-qt-xr/dist/include/mozilla/OperatorNewExtensions.h:47
#9  0x0000007fbc7cb54c in mozilla::dom::BrowsingContext::InitSessionHistory
    (this=0x7f889dc120)
    at docshell/base/BrowsingContext.cpp:2316
#10 0x0000007fbc7cb590 in mozilla::dom::BrowsingContext::InitSessionHistory
    (this=this@entry=0x7f889dc120)
    at obj-build-mer-qt-xr/dist/include/mozilla/dom/BrowsingContext.h:161
#11 0x0000007fbc901fac in nsWebBrowser::Create (aContainerWindow=
    <optimized out>, aParentWidget=<optimized out>, 
    aBrowsingContext=aBrowsingContext@entry=0x7f889dc120,
    aInitialWindowChild=aInitialWindowChild@entry=0x0)
    at toolkit/components/browser/nsWebBrowser.cpp:158
#12 0x0000007fbca950e8 in mozilla::embedlite::EmbedLiteViewChild::
    InitGeckoWindow (this=0x7f88bca840, parentId=0, parentBrowsingContext=0x0, 
    isPrivateWindow=<optimized out>, isDesktopMode=false)
    at obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:847
[...]
#33 0x0000007fb735e89c in ?? () from /lib64/libc.so.6
(gdb) c
Continuing.
[...]

This could well be our smoking gun. I need to check back through the backtrace to understand the process happening on ESR 78 and then establish why something similar isn't happening on ESR 91. Progress!

Immediately on examining the backtrace it's clear something odd is happening. The callee is BrowsingContext::DidSet() which is the only location where the call is made in both ESR 78 and ESR 91. So that does rather beg the question of why it's not getting called in ESR 91.

Digging back through the backtrace further, it eventually materialises that the difference is happening in nsWebBrowser::Create(). There's this bit of code in ESR 78 that looks like this:

  // If the webbrowser is a content docshell item then we won't hear any
  // events from subframes. To solve that we install our own chrome event
  // handler that always gets called (even for subframes) for any bubbling
  // event.

  if (aBrowsingContext->IsTop()) {
    aBrowsingContext->InitSessionHistory();
  }

  NS_ENSURE_SUCCESS(docShellAsWin->Create(), nullptr);

  docShellTreeOwner->AddToWatcher();  // evil twin of Remove in SetDocShell(0)
  docShellTreeOwner->AddChromeListeners();

You can see the InitSessionHistory() call in there which eventually leads to the creation of our sessinHistory object. In ESR 91 that same bit of code looks like this:

  // If the webbrowser is a content docshell item then we won't hear any
  // events from subframes. To solve that we install our own chrome event
  // handler that always gets called (even for subframes) for any bubbling
  // event.

  nsresult rv = docShell->InitWindow(nullptr, docShellParentWidget, 0, 0, 0, 0);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return nullptr;
  }

  docShellTreeOwner->AddToWatcher();  // evil twin of Remove in SetDocShell(0)
  docShellTreeOwner->AddChromeListeners();

Where has the InitSessionHistory() gone? It should be possible to find out using a bit of git log searching. Here we're following the rule of using git blame to find out about lines that have been added and git log -S to find out about lines that have been removed.

$ git log -1 -S InitSessionHistory toolkit/components/browser/nsWebBrowser.cpp
commit 140a4164598e0c9ed537a377cf66ef668a7fbc25
Author: Randell Jesup <rjesup@wgate.com>
Date:   Mon Feb 1 22:57:12 2021 +0000

    Bug 1673617 - Create BrowsingContext::mChildSessionHistory more
    aggressively, r=nika
    
    Differential Revision: https://phabricator.services.mozilla.com/D100348

Just looking at this change, it removes the call to InitSessionHistory() in nsWebBrowser::Create() and moves it to nsFrameLoader::TryRemoteBrowserInternal. There are some related changes in the parent Bug 1673617, but looking through those it doesn't seem that they're anything we need to worry about.

Placing a breakpoint on nsFrameLoader::TryRemoteBrowserInternal() shows that it's not being called on ESR 91.

The interesting thing is that it appears that if EnsureRemoteBrowser() gets called in this bit of code:

BrowsingContext* nsFrameLoader::GetBrowsingContext() {
  if (mNotifyingCrash) {
    if (mPendingBrowsingContext && mPendingBrowsingContext->EverAttached()) {
      return mPendingBrowsingContext;
    }
    return nullptr;
  }
  if (IsRemoteFrame()) {
    Unused << EnsureRemoteBrowser();
  } else if (mOwnerContent) {
    Unused << MaybeCreateDocShell();
  }
  return GetExtantBrowsingContext();
}

If the first path in the second condition is followed then the InitSessionHistory() would get called too. If this gets called by the InitSessionHistory() doesn't, it would imply that IsRemoteFrame() must be false. But if it is false then MaybeCreateDocShell() could get called, which also has a call to InitSessionHistory() like this:

  // If we are an in-process browser, we want to set up our session history.
  if (mIsTopLevelContent && mOwnerContent->IsXULElement(nsGkAtoms::browser) &&
      !mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::disablehistory)) {
    // XXX(nika): Set this up more explicitly?
    mPendingBrowsingContext->InitSessionHistory();
  }

I wonder what's going on around about this code then. Checking with the debugger the answer turns out to be that apparently, nsFrameLoader::GetExtantBrowsingContext() simply doesn't get called either.

From here, things pan out. The EnsureRemoteBrowser() method is called by all of these methods:

1. nsFrameLoader::GetBrowsingContext()
2. nsFrameLoader::ShowRemoteFrame()
3. nsFrameLoader::ReallyStartLoadingInternal()

None of these are static methods and when I place a breakpoint on the nsFrameLoader constructor it doesn't get hit. So it's not possible for any of these methods to be called and there's no point trying to dig any deeper via them.

However this isn't true in ESR 78 where the constructor does get called. It's almost certainly worthwhile finding out about this difference. But unfortunately I'm out of time for today.

I have to wrap things up. As I mentioned previously, I'm taking a break for two weeks to give myself a bit more breathing space as I prepare for FOSDEM, which I'm really looking forward to. If you're travelling to Brussels yourself then I hope to see you there. You'll be able to find me mostly on the Linux on Mobile stand.

When I get back I'll be back to posting these dev diaries again. And as a note to myself, when I do, my first action must be to figure out why nsFrameLoader is being created in ESR 78 but not ESR 91. That might hold the key to why the sessionHistory isn't getting called in ESR 91. And as a last resort, I can always revert commit 140a4164598e0c9ed53.

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