flypig.co.uk

A small win this morning. The changes I've been making over the last week to the embedhelper.js code caused problems — or at least I though they did — with the responsiveness of the Qt user interface layer of the browser. I went as far as reverting the changes to try to fix this.

Last night I'd reverted them all and still found the problem persisted. So I made one more change, to remove the forced activation state. That meant an overnight rebuild. This morning the issue is resolved.

So it turned out not to be the embedhelper.js change after at all, but rather the change I'd added in directly prior to that. My question is now whether I should reintroduce the embedhelper.js changes or not. But I have something else to do before that.

Over the last few days I've also been digging into the render code more. What I really want to know is what happens inside the nsDisplayList::PaintRoot() method, assuming it's getting called. I plan to find out.

Immediately on trying to investigate this I hit my first problem, which is that while it's called cleanly on ESR 78, it's not called at all on ESR 91. I know exactly why that is; it's because the compositor isn't active. That's why I made the change I reverted last night. So I need to get it activated, and I want to get it activated higher up the stack than before to avoid it introducing this user interface glitch.

You might ask why I don't just live with the user interface glitch. It's because it messes up control of the browser, which will make things far harder to test and debug. So I want to get to the bottom (or in this case, in relation to the stack, the top) of it.

Here, for reference, is the change that's caused the problems:

--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -375,6 +375,7 @@ already_AddRefed<BrowsingContext> BrowsingContext::CreateDetached(
     if (aType == Type::Content) {
       // Content gets managed by the chrome front-end / embedder element and
       // starts as inactive.
-      return ExplicitActiveStatus::Inactive;
+      return ExplicitActiveStatus::Active;
     }

There is this comment there: "Content gets managed by the chrome front-end / embedder element and starts as inactive." I need to figure out what that means in practice, so I'm going to review, again, the changes that brought us here.

First of all there's this commit from December 2020. This makes big changes to the way the IsActive flag gets set. In particular, it essentially moves it from being the job of the docShell to manage to being the job of the presShell to manage.

$ git log -1 3987c781d028e
commit 3987c781d028e4edc599659f0776d26b747bfbd6
Author: Emilio Cobos Álvarez <emilio@crisal.io>
Date:   Fri Dec 11 15:43:19 2020 +0000

    Bug 1635914 - Move active flag handling explicitly to BrowsingContext. r=nika
    
    And have it mirror in the parent process more automatically.
    
    The docShellIsActive setter in the browser-custom-element side needs to
    be there rather than in the usual DidSet() calls because the
    AsyncTabSwitcher code relies on getting an exact amount of notifications
    as response to that specific setter. Not pretty, but...
    
    BrowserChild no longer sets IsActive() on the docshell itself for OOP
    iframes. This fixes bug 1679521. PresShell activeness is used to
    throttle rAF as well, which handles OOP iframes nicely as well.
    
    Differential Revision: https://phabricator.services.mozilla.com/D96072

In theory part of the fix is to switch from using the docShell to the presShell in the EmbedLite code that sets the active flag; so I'm amending it like this:

--- a/embedding/embedlite/embedshared/EmbedLiteViewChild.cpp
+++ b/embedding/embedlite/embedshared/EmbedLiteViewChild.cpp
@@ -626,12 +626,12 @@ mozilla::ipc::IPCResult EmbedLiteViewChild::RecvSetIsActive(const bool &aIsActiv
     widget->SetActive(aIsActive);
   }
 
-  // Update state via DocShell -> PresShell
-  nsCOMPtr<nsIDocShell> docShell = do_GetInterface(mWebNavigation);
-  if (NS_WARN_IF(!docShell)) {
+  if (NS_WARN_IF(!presShell)) {
     return IPC_OK();
   }
 
+  presShell->SetIsActive(aIsActive);
+
   mWidget->Show(aIsActive);
   mWebBrowser->SetVisibility(aIsActive);

Unfortunately this fails because PresShell:SetIsActive() is a private method. That surprised me because I could see it being used very similarly in the earlier upstream change set.

The reason is that the change that made it private came later. Here it is, from July 2021:

$ git log -1 a5f4c42f89d16
commit a5f4c42f89d1688dd037dfe7eeaedb1699d59e3c
Author: Emilio Cobos Álvarez <emilio@crisal.io>
Date:   Mon Jul 5 17:31:48 2021 +0000

    Bug 1717983 - Improve PresShell active flag handling. r=nika
    
    This moves the logic of whether a pres shell should be active to a
    single place to make it sane to reason about, and fixes the
    subdocument propagation when a BrowserChild becomes visible.
    
    Differential Revision: https://phabricator.services.mozilla.com/D118703

This does more than just make the method private. It switches it for a method PresShell::ActivenessMaybeChanged() that takes no parameters, but determines whether or not the document is active using some other criteria, rather than it being set explicitly.

I tried replacing the call to DocShell::IsActive() to use this magical method instead, but it didn't fix things. So I also tried updating the class to make the PresShell::SetIsActive() method public again. That had no obviously beneficial effect. But I'm building from here.

For completeness the call to EmbedLiteViewChild::RecvSetIsActive() that calls it is happening and PresShell::SetIsActive() is being called to activate the document. So my next step is to follow the calls to PresShell::ActivenessMaybeChanged() and PresShell::SetIsActive() in case one or other is deactivating the document.

If that's not the case, then maybe it's because the flag isn't propagating down to the level of nsLayoutUtils::PaintFrame(), which is the method that should be calling PaintRoot().

Incidentally, PresShell::ActivenessMaybeChanged() works by calling PresShell::ShouldBeActive(), which is returning false because in the following code bc->IsActive() is returning false:

  BrowsingContext* bc = doc->GetBrowsingContext();
  MOZ_LOG(gLog, LogLevel::Debug,
          (" > BrowsingContext %p  active: %d", bc, bc && bc->IsActive()));
  return bc && bc->IsActive();

Stepping through the code there's now a bit of back-and-forth enabling and disabling the activeness. The result of bc->IsActive() isn't always false but it looks like it would be worthwhile following this code manually to figure out the circumstances under which it's true or not. But even when the state is set to active, the PaintRoot() method still doesn't see to be being called. So in short there's still much more work to be done here.

I'll continue digging in to this tomorrow.

For all the other entries in my developer diary, check out the Gecko-dev Diary page.