flypig.co.uk

Yesterday we were comparing ESR 78 and ESR 91 to see why the session history is initialised in the former but not in the latter. We reached this bit of code in ESR 91:

  // 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();
  }

As you can see, this could potentially initialise the session history as long as the condition is true going in to it.

But when we stepped through the program we found it wasn't true. That's because mIsTopLevelContent was set to false, which itself was because BrowsingContext::mParentWindow was non-null.

That makes sense intuitively: if the window has a parent, then it's not a top level element.

However with ESR 78 we have a similar situation, because the code looks like this:

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

But this is being executed, suggesting that in this case there is no parent window. Why the difference?

Although that seems like a fair question, there's another question I'd like to try to answer first. The D100348 change that caused this problem in the first place moves the InitSessionHistory() call from nsWebBrowser::Create() to sFrameLoader::TryRemoteBrowserInternal(). So clearly the authors of that change thought the execution flow would go via the latter method. So it would be good to know why it isn't for us using EmbedLite.

Looking through the code, there is at least one route to getting to InitSessionHistory() via TryRemoteBrowserInternal() that looks like this (I routed this by hand... quite laborious):

nsFrameLoader::LoadURI()
Document::InitializeFrameLoader()
Document::MaybeInitializeFinalizeFrameLoaders()
nsFrameLoader::ReallyStartLoading()
nsFrameLoader::ReallyStartLoadingInternal()
nsFrameLoader::EnsureRemoteBrowser()
nsFrameLoader::TryRemoteBrowser()
nsFrameLoader::TryRemoteBrowserInternal()
BrowsingContext::InitSessionHistory()

In other words a call to LoadURI() can then call InitializeFrameLoader() which can then call MaybeInitializeFinalizeFrameLoaders() and so on, all the way to InitSessionHistory().

The top half of this, from LoadURI() to ReallyStartLoadingInternal() is certainly being called when the code is executed. We can see that by placing some breakpoints on various methods and checking the results:

Thread 10 "GeckoWorkerThre" hit Breakpoint 4, nsFrameLoader::
    LoadURI (this=this@entry=0x7fc1419ef0, aURI=0x7fc15feb90,
    aTriggeringPrincipal=0x7fc11a47d0, aCsp=0x7ee8004780,
    aOriginalSrc=aOriginalSrc@entry=true) at dom/base/nsFrameLoader.cpp:600
600                                     bool aOriginalSrc) {
(gdb) c
Continuing.

Thread 10 "GeckoWorkerThre" hit Breakpoint 3, mozilla::dom::Document::
    InitializeFrameLoader (this=this@entry=0x7fc11a1960, 
    aLoader=aLoader@entry=0x7fc1419ef0) at dom/base/Document.cpp:8999
8999    nsresult Document::InitializeFrameLoader(nsFrameLoader* aLoader) {
(gdb) c
Continuing.

Thread 10 "GeckoWorkerThre" hit Breakpoint 2, mozilla::dom::Document::
    MaybeInitializeFinalizeFrameLoaders (this=0x7fc11a1960)
    at dom/base/Document.cpp:9039
9039    void Document::MaybeInitializeFinalizeFrameLoaders() {
(gdb) c
Continuing.
[...]
Thread 10 "GeckoWorkerThre" hit Breakpoint 9, nsFrameLoader::
    ReallyStartLoadingInternal (this=this@entry=0x7fc147afe0)
    at dom/base/nsFrameLoader.cpp:664
664     nsresult nsFrameLoader::ReallyStartLoadingInternal() {
(gdb) p mIsRemoteFrame
$3 = false
(gdb) 

But that's as far as it goes. Beyond that it's the fact that mIsRemoteFrame is false that prevents the InitSessionHistory() from being called, because the relevant code inside ReallyStartLoadingInternal() looks like this:

  if (IsRemoteFrame()) {
    if (!EnsureRemoteBrowser()) {
      NS_WARNING("Couldn't create child process for iframe.");
      return NS_ERROR_FAILURE;
    }
[...]

We can understand this better by also considering what IsRemoteFrame() is doing:

bool nsFrameLoader::IsRemoteFrame() {
  if (mIsRemoteFrame) {
    MOZ_ASSERT(!GetDocShell(), "Found a remote frame with a DocShell");
    return true;
  }
  return false;
}

If it jumps past this, then it looks like the next place where InitSessionHistory() could potentially get called is in nsFrameLoader::MaybeCreateDocShell(). Maybe that's the place it's supposed to happen for non-remote frames. But in that case we're back to the same problem of mPendingBrowsingContext being false that we started with.

Once again, the reason is that there's a parent browsing context:

(gdb) p mPendingBrowsingContext.mRawPtr->mParentWindow.mRawPtr->
    mBrowsingContext.mRawPtr
$8 = (mozilla::dom::BrowsingContext *) 0x7fc0ba3f00

Contrast this with the ESR 78 way of doing things. In that case the call to >nsWebBrowser::Create() is coming from EmbedLiteViewChild::InitGeckoWindow(). There the BrowsingContext is explicitly created detached and so has no parent widget.

The route via LoadURI() in ESR 91 is far less direct. Given this, one potential solution, which I think I rather like, is to explicitly initialise the session history where EmbedLite initialises the window, like this:

void
EmbedLiteViewChild::InitGeckoWindow(const uint32_t parentId,
                                    mozilla::dom::BrowsingContext
                                    *parentBrowsingContext,
                                    const bool isPrivateWindow,
                                    const bool isDesktopMode,
                                    const bool isHidden)
{
[...]
  RefPtr<BrowsingContext> browsingContext = BrowsingContext::CreateDetached
      (nullptr, parentBrowsingContext, nullptr, EmptyString(),
      BrowsingContext::Type::Content);
  browsingContext->SetUsePrivateBrowsing(isPrivateWindow); // Needs to be called before attaching
  browsingContext->EnsureAttached();
  browsingContext->InitSessionHistory();
[...]
  mWebBrowser = nsWebBrowser::Create(mChrome, mWidget, browsingContext,
                                     nullptr);

In this case the session history is created broadly speaking where it would have been in ESR 78. This should be safe given that it isn't initialised at any later time. And the nice thing about it is that it doesn't require any changes to the core gecko code, only to the EmbedLite code.

So, with this change made, it's time to set off the build. It'll take a while to complete, but once it has, we can give it a spin to test it.

For the record, and while the build is running, here is the backtrace for nsFrameLoader::MaybeCreateDocShell():

(gdb) bt
#0  nsFrameLoader::MaybeCreateDocShell (this=this@entry=0x7fc1476da0)
    at dom/base/nsFrameLoader.cpp:2241
#1  0x0000007ff2def6c0 in nsFrameLoader::ReallyStartLoadingInternal
    (this=this@entry=0x7fc1476da0)
    at dom/base/nsFrameLoader.cpp:751
#2  0x0000007ff2defa88 in nsFrameLoader::ReallyStartLoading
    (this=this@entry=0x7fc1476da0)
    at dom/base/nsFrameLoader.cpp:656
#3  0x0000007ff2d25198 in mozilla::dom::Document::
    MaybeInitializeFinalizeFrameLoaders (this=0x7fc11f88d0)
    at dom/base/Document.cpp:9068
#4  0x0000007ff2cceebc in mozilla::detail::RunnableMethodArguments<>::applyImpl
    <mozilla::dom::Document, void (mozilla::dom::Document::*)()>
    (mozilla::dom::Document*, void (mozilla::dom::Document::*)(),
    mozilla::Tuple<>&, std::integer_sequence<unsigned long>)
    (args=..., m=<optimized out>, o=<optimized out>)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsThreadUtils.h:1151
#5  mozilla::detail::RunnableMethodArguments<>::apply<mozilla::dom::Document,
    void (mozilla::dom::Document::*)()> (m=<optimized out>, o=<optimized out>, 
    this=<optimized out>)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsThreadUtils.h:1154
[...]
#20 0x0000007ff4e7e564 in js::RunScript (cx=cx@entry=0x7fc0233bd0, state=...)
    at js/src/vm/Interpreter.cpp:395
#21 0x0000007ff4e7e9b0 in js::InternalCallOrConstruct (cx=cx@entry=0x7fc0233bd0,
    args=..., construct=construct@entry=js::NO_CONSTRUCT, 
    reason=reason@entry=js::CallReason::Call) at js/src/vm/Interpreter.cpp:543

If we get the right outcome from this build this backtrace may not be useful any more, but it's worth taking a record just in case.

The build is running, so that's it for today. We'll find out whether it worked or not tomorrow.

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