flypig.co.uk

It's a good job I stopped working on this last night when I did. On returning to it this morning with a clearer head it didn't take long to identify that the changes needed to revert the D5005 upstream code changes had already been wrapped up into a patch — patch 0044 to be precise — that I'd not yet applied.

If the patch can be applied cleanly, it could save me a lot of time. So first step is to do a dry run of applying the patch. This won't actually make any changes to the code, but it will tell me whether the patch is going to apply without incident if I attempt it properly.

$ patch --dry-run -d gecko-dev -p1 < rpm/
    0044-Revert-Bug-1445570-Remove-EnsureEventualAfterPaint-t.patch 
checking file dom/base/nsDOMWindowUtils.cpp
Hunk #1 succeeded at 407 (offset 51 lines).
checking file layout/base/nsDocumentViewer.cpp
Hunk #1 succeeded at 1573 (offset -44 lines).
Hunk #2 succeeded at 1718 (offset -45 lines).
Hunk #3 succeeded at 3649 (offset -254 lines).
checking file layout/base/nsPresContext.cpp
Hunk #1 succeeded at 314 (offset 3 lines).
Hunk #2 FAILED at 804.
Hunk #3 succeeded at 2021 (offset 66 lines).
Hunk #4 FAILED at 2039.
Hunk #5 FAILED at 2121.
Hunk #6 FAILED at 2613.
Hunk #7 succeeded at 2716 with fuzz 2 (offset -132 lines).
4 out of 7 hunks FAILED
checking file layout/base/nsPresContext.h
Hunk #1 succeeded at 355 (offset -5 lines).
Hunk #2 succeeded at 912 (offset -4 lines).
Hunk #3 FAILED at 1363.
Hunk #4 FAILED at 1455.
2 out of 4 hunks FAILED
checking file layout/base/nsRefreshDriver.cpp
Hunk #1 FAILED at 1166.
Hunk #2 FAILED at 1239.
Hunk #3 FAILED at 2337.
Hunk #4 FAILED at 2396.
Hunk #5 FAILED at 2404.
5 out of 5 hunks FAILED
checking file layout/base/nsRefreshDriver.h
Hunk #1 FAILED at 507.
1 out of 1 hunk FAILED
checking file layout/style/test/test_restyles_in_smil_animation.html

There are quite a few nice succeeded lines there. But also a disturbing number of FAILED lines as well. That's FAILED in all caps, just in case the word itself doesn't sufficiently convey the damage being done.

The advice Raine would give me in this situation is to apply a three-way merge using git. So that's what I'm going to try.

$ git am --3way ../rpm/
    0044-Revert-Bug-1445570-Remove-EnsureEventualAfterPaint-t.patch
Applying: Revert "Bug 1445570 - Remove EnsureEventualAfterPaint timer. 
    r=tnikkel"
Using index info to reconstruct a base tree...
M       dom/base/nsDOMWindowUtils.cpp
M       layout/base/nsDocumentViewer.cpp
M       layout/base/nsPresContext.cpp
M       layout/base/nsPresContext.h
M       layout/base/nsRefreshDriver.cpp
M       layout/base/nsRefreshDriver.h
Falling back to patching base and 3-way merge...
Auto-merging layout/base/nsRefreshDriver.h
CONFLICT (content): Merge conflict in layout/base/nsRefreshDriver.h
Auto-merging layout/base/nsRefreshDriver.cpp
CONFLICT (content): Merge conflict in layout/base/nsRefreshDriver.cpp
Auto-merging layout/base/nsPresContext.h
CONFLICT (content): Merge conflict in layout/base/nsPresContext.h
Auto-merging layout/base/nsPresContext.cpp
CONFLICT (content): Merge conflict in layout/base/nsPresContext.cpp
Auto-merging layout/base/nsDocumentViewer.cpp
Auto-merging dom/base/nsDOMWindowUtils.cpp
error: Failed to merge in the changes.
Patch failed at 0001 Revert "Bug 1445570 - Remove EnsureEventualAfterPaint 
    timer. r=tnikkel"
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am 
    --abort".

This has done a better job than the patch did, but there are still some problems which require manual intervention. But after lots of manual changes, comparing the source code against the patch file, eventually I'm able to get it there.

git am --continue

It's still the morning and before I start work. It would be nice to kick off the build before I have to move on to my proper work day. That way there's an outside chance that the build will be completed and ready to test by this evening. So although I'd normally read carefully through the changes to check whether they look sound, I'm just going to kick off the build and hope for the best. Here we go!

sfdk build -d -p --with git_workaround

Sadly as the build trundles on into my work day it hits an error in the code.

27:10.41 In file included from ${PROJECT}/obj-build-mer-qt-xr/dist/include/
    mozilla/dom/DocumentTimeline.h:16,
27:10.41                  from ${PROJECT}/gecko-dev/dom/animation/Animation.cpp:
    15,
27:10.41                  from Unified_cpp_dom_animation0.cpp:2:
27:10.41 ${PROJECT}/gecko-dev/layout/base/nsRefreshDriver.h: In member function 
    ‘bool nsRefreshDriver::AtPendingTransactionLimit()’:
27:10.41 ${PROJECT}/gecko-dev/layout/base/nsRefreshDriver.h:514:12: error: 
    ‘mPendingTransactions’ was not declared in this scope
27:10.41      return mPendingTransactions.Length() == 2;
27:10.42             ^~~~~~~~~~~~~~~~~~~~
27:10.42 ${PROJECT}/gecko-dev/layout/base/nsRefreshDriver.h:514:12: note: 
    suggested alternative: ‘mPendingTransaction’
27:10.42      return mPendingTransactions.Length() == 2;
27:10.42             ^~~~~~~~~~~~~~~~~~~~
27:10.42             mPendingTransaction
27:10.42 ${PROJECT}/gecko-dev/layout/base/nsRefreshDriver.h: In member function 
    ‘bool nsRefreshDriver::TooManyPendingTransactions()’:
27:10.42 ${PROJECT}/gecko-dev/layout/base/nsRefreshDriver.h:517:12: error: 
    ‘mPendingTransactions’ was not declared in this scope
27:10.42      return mPendingTransactions.Length() >= 2;
27:10.43             ^~~~~~~~~~~~~~~~~~~~
27:10.43 ${PROJECT}/gecko-dev/layout/base/nsRefreshDriver.h:517:12: note: 
    suggested alternative: ‘mPendingTransaction’
27:10.43      return mPendingTransactions.Length() >= 2;
27:10.43             ^~~~~~~~~~~~~~~~~~~~
27:10.43             mPendingTransaction
27:24.10 make[4]: *** [${PROJECT}/gecko-dev/config/rules.mk:694: 
    Unified_cpp_dom_animation0.o] Error 1
27:24.10 make[3]: *** [${PROJECT}/gecko-dev/config/recurse.mk:72: dom/animation/
    target-objects] Error 2

These errors are clearly all caused by the patches I applied. Unfortunately I can't fix them while I'm at work, which means looking in to them in the evening. If I can get them done today there's a chance I'll be able to run the build overnight, but let's see.

The error is about use of mPendingTransactions in the nsRefreshDriver class. It's true that the member variable isn't defined there. Should it be?

It's not defined in the code in the upstream changeset on either side of the diff. Nor does it appear in the patch that we applied. Which is a little odd, except that there is reference in all of those to a mPendingTransaction member variable. Note the subtle difference: mPendingTransaction vs. mPendingTransactions (singular vs. plural).

And looking more carefully at the patch we can see the reason. Here are the changes that the patch tried to apply:

diff --git a/layout/base/nsRefreshDriver.h b/layout/base/nsRefreshDriver.h
index 07feab12e079..deec935f25f4 100644
--- a/layout/base/nsRefreshDriver.h
+++ b/layout/base/nsRefreshDriver.h
@@ -507,12 +507,7 @@ class nsRefreshDriver final : public mozilla::layers::
    TransactionIdAllocator,
   RefPtr<nsRefreshDriver> mRootRefresh;
 
   // The most recently allocated transaction id.
-  TransactionId mNextTransactionId;
-  // This number is mCompletedTransaction + (pending transaction count).
-  // When we revoke a transaction id, we revert this number (since it's
-  // no longer outstanding), but not mNextTransactionId (since we don't
-  // want to reuse the number).
-  TransactionId mOutstandingTransactionId;
+  TransactionId mPendingTransaction;
   // The most recently completed transaction id.
   TransactionId mCompletedTransaction;

But this failed and I made a manual intervention; one which I thought looked pretty similar, but which in fact included a crucial destructive change:

diff --git a/layout/base/nsRefreshDriver.h b/layout/base/nsRefreshDriver.h
index 29c595ba5ba8..b9a0fa4bb2ed 100644
--- a/layout/base/nsRefreshDriver.h
+++ b/layout/base/nsRefreshDriver.h
@@ -528,8 +528,9 @@ class nsRefreshDriver final : public mozilla::layers::
    TransactionIdAllocator,
   RefPtr<nsRefreshDriver> mRootRefresh;
 
   // The most recently allocated transaction id.
-  TransactionId mNextTransactionId;
-  AutoTArray<TransactionId, 3> mPendingTransactions;
+  TransactionId mPendingTransaction;
+  // The most recently completed transaction id.
+  TransactionId mCompletedTransaction;

As you can see, my change removed the mPendingTransactions (plural) variable. The good news is that after applying the patch this mPendingTransactions variable is only used in two places:

  bool AtPendingTransactionLimit() {
    return mPendingTransactions.Length() == 2;
  }
  bool TooManyPendingTransactions() {
    return mPendingTransactions.Length() >= 2;
  }

Neither of these two methods is used anywhere, so it should be safe to remove them. On the other hand, it seems that by applying the patch I've trampled over rather more changes than I'd anticipated. So while removing these two methods to get the build to pass is a simple solution, I'm also going to need to spend some time checking the other changes.

But, having worked through the patch and read through it side-by-side with the changes made to the ESR 91 code, it all looks sensible, albeit rather messy. It looks like the ESR 91 code now matches the code that would have been expected had the patch been applied to ESR 78 as a base.

I've therefore set the build running again. If it builds that'll be a good sign. The good news is also that once it's built, due to the work I've been doing over the last few days, it should be really easy to check whether this change has had the desired effect. All I need to do is check whether the OnFirstPaint() method is being called. If it is, then that's a good sign things have worked out.

Since it's building now I can't do much else until it completes, so I'm going to call it a night. By the morning it should be done and we can continue with some testing.

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