flypig.co.uk

It was nice to get such appreciative comments after my last post, thank you for those who got in touch saying they were interested to hear my thoughts on the Jolla C2. I'm glad something good came out of my stupidity in sending my laptop to sleep mid-build! A special mention has to go to Leif-Jöran Olsson (ljo) for his amazing poetry in describing the situation!
 
This really made my day; thank you ljo! And I needed it to because I'm just not happy with how all this patching, or more precisely reverting, has been going. As ljo aludes to in his poem, yesterday I applied a three-way merge of the Sailfish patch 0044. This patch was designed to revert upstream commit 1deccd7ac14706ad, but it's intended to be applied on top of the ESR 78 codebse. Things have changed with ESR 91, so the patch no longer applies directly.

For reference, here's the description that goes alongside patch 0044.

commit c3d0a8fb29cbb2998c238a2c85801059ed636070 (HEAD -> 
    FIREFOX_ESR_91_9_X_RELBRANCH_patches)
Author: Andrew den Exter <andrew.den.exter@qinetic.com.au>
Date:   Mon Sep 6 09:52:04 2021 +0000

    Revert &quot;Bug 1445570 - Remove EnsureEventualAfterPaint timer. 
    r=tnikkel&quot;

    This reverts commit 1deccd7ac14706ad1849343cfb2b93df191a1c42.

And here's the description of the commit that the patch is aiming to revert.

$ git log -1 --grep "Bug 1445570"
commit 1deccd7ac14706ad1849343cfb2b93df191a1c42
Author: Mantaroh Yoshinaga <mantaroh@gmail.com>
Date:   Thu Sep 6 02:21:39 2018 +0000

    Bug 1445570 - Remove EnsureEventualAfterPaint timer. r=tnikkel
    
    MozReview-Commit-ID: C7WICJ5Q0ES
    
    Differential Revision: https://phabricator.services.mozilla.com/D5005
    
    --HG--
    extra : moz-landing-system : lando

I'm copying these out here because I think they're going to be helpful as we continue onwards.

Since patching has been so unsuccessful, rather than trying to apply the Sailfish patch for ESR 78, why don't I go directly to the source and attempt to revert commit 1deccd7ac14706ad instead? Maybe that will bring better results.

It seems like a good plan, but in practice when I try to do this I get a bunch of conflicts.

$ git revert 1deccd7ac14706ad1849343cfb2b93df191a1c42
Auto-merging dom/base/nsDOMWindowUtils.cpp
CONFLICT (content): Merge conflict in dom/base/nsDOMWindowUtils.cpp
Auto-merging layout/base/nsPresContext.cpp
CONFLICT (content): Merge conflict in layout/base/nsPresContext.cpp
Auto-merging layout/base/nsPresContext.h
CONFLICT (content): Merge conflict in layout/base/nsPresContext.h
Auto-merging layout/base/nsRefreshDriver.cpp
CONFLICT (content): Merge conflict in layout/base/nsRefreshDriver.cpp
Auto-merging layout/base/nsRefreshDriver.h
CONFLICT (content): Merge conflict in layout/base/nsRefreshDriver.h
Auto-merging layout/reftests/text/reftest.list
CONFLICT (content): Merge conflict in layout/reftests/text/reftest.list
Auto-merging layout/style/test/test_restyles_in_smil_animation.html
error: could not revert 1deccd7ac147... Bug 1445570 - Remove 
    EnsureEventualAfterPaint timer. r=tnikkel
hint: After resolving the conflicts, mark them with
hint: &quot;git add/rm <pathspec>&quot;, then run
hint: &quot;git revert --continue&quot;.
hint: You can instead skip this commit with &quot;git revert --skip&quot;.
hint: To abort and get back to the state before &quot;git revert&quot;,
hint: run &quot;git revert --abort&quot;.

I don't much enjoy conflicts so I wonder if there's a different revert strategy that I can use to make this go more smoothly. Checking the git-revert documentation does indeed throw up some promising suggestions.

$ man git-revert
[...]
       --strategy=<strategy>
           Use the given merge strategy. Should only be used once. See the 
    MERGE STRATEGIES section in git-merge(1) for details.

This suggests checking the MERGE STRATEGIES section of the git-merge man pages. But that section is rather long, so rather than copying it all out here, I recommend to check out the online documentation instead. According to this page, alongside the standard ort ("Ostensibly Recursive’s Twin") strategy — which is what I used here — there's also a so-called recursive strategy. The ort approach didn't go great, so lets try the alternative.

$ git revert --strategy=recursive 1deccd7ac14706ad1849343cfb2b93df191a1c42
[...]

Unfortunately that gives me similarly poor results. What I really want is a strategy that goes back through the history, removes the commit to be reverted, applies all subsequent commits on top of that, then generates a diff of the result against the current codebase. It's possible this is exactly what these strategies are doing already, but I'm not smart enough to figure that out from the documentation.

Actually, after more reflection, where the documentation talks about common ancestors, I think it's this that it's refering to. Nevertheless it didn't give good results, so I want to try a different approach.

Rather than applying the patch, or reverting the commit, there is a third option. And that's to figure out how NotifyInvalidation() ought to be called in the existing ESR 91 codebase. If there is such a way then it's possible this patch could be skipped entirely, which would have the nice side-effect of making future maintenance easier.

I'll explore this third option a little further over the next few days (I say "few days" because it's going to take at least one full build, so this isn't a task that can be completed in a day).

The first thing I notice is that the code that's been removed isn't the only place that nsPresContext::NotifyDidPaintForSubtree() gets called. It also gets called from inside the nsAutoNotifyDidPaint class destructor:

class nsAutoNotifyDidPaint {
 public:
  nsAutoNotifyDidPaint(PresShell* aShell, PaintFlags aFlags)
      : mShell(aShell), mFlags(aFlags) {}
  ~nsAutoNotifyDidPaint() {
    if (!!(mFlags & PaintFlags::PaintComposite)) {
      mShell->GetPresContext()->NotifyDidPaintForSubtree();
    }
  }

 private:
  PresShell* mShell;
  PaintFlags mFlags;
};

This destructor does get called as a result of in instance being created on the stack in the PresShell::Paint() method. At the end of the method the instance is destroyed and the destructor called.

However, in this case the NotifyDidPaintForSubtree() method doesn't get called because !!(mFlags & PaintFlags::PaintComposite) evaluates to false.

There's also a call inside nsView::DidCompositeWindow() but this never gets called. In this case the method is part of an implementation of the nsIWidgetListener interface. We do have some instances of this interfaces in use, most notably inherited from nsBaseWidget into EmbedLitePuppetWidget:

class EmbedLitePuppetWidget : public PuppetWidgetBase
{
[...]

class PuppetWidgetBase : public nsBaseWidget
{
[...]

class nsBaseWidget : public nsIWidget, public nsSupportsWeakReference {
[...]
  nsIWidgetListener* mWidgetListener;
  nsIWidgetListener* mAttachedWidgetListener;
[...]

However whilst these are used in the EmbedLitePuppetWidget code they don't seem to be set there. The mAttachedWidgetListener variable gets set by nsView but that's a direction we already explored. The mWidgetListener variable is also set by nsView in addition to AppWindow. The AppWindow class is never used, but although the nsView class is instantiated we already know that nsView::DidCompositeWindow() is never called. I'm now not entirely sure why it doesn't get called. But it doesn't.

None of this is directly helping us to solve the problem, but it does allow us to explore possibilties and rule things out. This is all going to require a bit more thought. I'll continue with this line of investigation tomorrow.

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