Fixes a number of race conditions and reliability issues with reftests and compositor.

[glennw]

The basic idea is it's safe to output an image for reftest by testing:

• That the compositor doesn't have any animations active.
• That the compositor is not waiting on any outstanding paint messages to arrive.
• That the script tasks are "idle" and therefore won't cause reflow.
  • This currently means page loaded, onload fired, reftest-wait not active, first reflow triggered.
  • It could easily be expanded to handle pending timers etc.
• That the "epoch" that the layout tasks have last laid out after script went idle, is reflected by the compositor in all visible layers for that pipeline.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/4984

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[glennw]

r? @larsbergstrom (or anyone else who has comments).

I haven't been able to repro any intermittent reftest failures with this patch applied, so hopefully it covers all the known issues.

Despite being a small-ish patch, this has a lot of subtle fixes - I lost count of the # of edge cases and races that I encountered in the reftests while working through this. I've tried to document it, but I'm sure I missed some parts or could explain bit of it better - please just ask when you run into something that doesn't make sense :)

[larsbergstrom]

Reviewed files:

• components/compositing/compositor.rs @ r1
• components/compositing/compositor_layer.rs @ r1
• components/compositing/compositor_task.rs @ r1
• components/compositing/constellation.rs @ r1
• components/compositing/headless.rs @ r1
• components/compositing/windowing.rs @ r1
• components/gfx/paint_task.rs @ r1
• components/layout/layout_task.rs @ r1
• components/layout_traits/lib.rs @ r1
• components/msg/compositor_msg.rs @ r1
• components/msg/constellation_msg.rs @ r1
• components/script/layout_interface.rs @ r1
• ports/cef/window.rs @ r1
• ports/glutin/window.rs @ r1
• ports/gonk/src/window.rs @ r1

---

^{components/compositing/compositor.rs, line 443 [r1] (raw file):}
Is it worth having any assertions here about what the previous value of ready_to_save_state is for this block?

---

^{components/compositing/compositor.rs, line 741 [r1] (raw file):}
Is there any debug info that would be useful here? i.e., what does it mean when you hit the else branch here and is that something we're going to need to debug? We've had a lot of epoch-related paint issues in the past.

---

^{components/compositing/compositor.rs, line 1117 [r1] (raw file):}
What in the world was this code doing?

---

^{components/compositing/compositor.rs, line 1149 [r1] (raw file):}
Why is the clone necessary here? Shouldn't you just be able to borrow self, or does borrowchk get confused?

---

^{components/compositing/compositor.rs, line 1170 [r1] (raw file):}
Are there any strange cases here, such as when we want to paint the image output of just the viewport, but there are layers outside of the viewport, so it will never be ready to paint the image?

---

^{components/compositing/constellation.rs, line 952 [r1] (raw file):}
nit: s/not/no

---

^{components/compositing/constellation.rs, line 973 [r1] (raw file):}
nit: is idle

---

Comments from the review on Reviewable.io

[larsbergstrom]

I left some comments. The files in script appear correct to me, but it would be best if @jdm or @Ms2ger could take a quick look to ensure there isn't anything missing!

---

Comments from the review on Reviewable.io

[glennw]

^{components/compositing/compositor.rs, line 443 [r1] (raw file):}
Done.

---

^{components/compositing/compositor.rs, line 741 [r1] (raw file):}
Done.

---

^{components/compositing/compositor.rs, line 1117 [r1] (raw file):}
It was just plain broken - it was causing test failures with the new code until I fixed the hashmap usage :)

---

^{components/compositing/compositor.rs, line 1149 [r1] (raw file):}
Done.

---

^{components/compositing/compositor.rs, line 1170 [r1] (raw file):}
This is handled by the check in does_layer_have_outstanding_paint_messages that only looks at layers where the requested epoch == current epoch (this fixes a failure I was getting in acid2 ref test which has layers that are not visible).

---

^{components/compositing/constellation.rs, line 952 [r1] (raw file):}
Done.

---

^{components/compositing/constellation.rs, line 973 [r1] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[larsbergstrom]

Reviewed files:

• components/compositing/compositor.rs @ r2
• components/compositing/constellation.rs @ r2

---

Comments from the review on Reviewable.io

[jdm]

Reviewed files:

• components/script/script_task.rs @ r1
• components/script_traits/lib.rs @ r1

---

^{components/script/script_task.rs, line 900 [r2] (raw file):}
Rather than any sort of heuristics here, I would rather tests rely on the explicit reftest-wait solution instead. I don't think this TODO is necessary.

---

^{components/script/script_task.rs, line 906 [r2] (raw file):}
Why not just use expect above and avoid the indentation?

---

^{components/script_traits/lib.rs, line 66 [r2] (raw file):}
DocumentLoaded.

---

^{components/script_traits/lib.rs, line 67 [r2] (raw file):}
Let's remove the comment and call this state DocumentLoading.

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Comments from the review on Reviewable.io

[glennw]

^{components/script/script_task.rs, line 900 [r2] (raw file):}
Done.

---

^{components/script/script_task.rs, line 906 [r2] (raw file):}
Done.

---

^{components/script_traits/lib.rs, line 66 [r2] (raw file):}
Done.

---

^{components/script_traits/lib.rs, line 67 [r2] (raw file):}
Done.

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review +S-needs-squash

---

Reviewed files:

• components/compositing/constellation.rs @ r3
• components/script/script_task.rs @ r3
• components/script_traits/lib.rs @ r3

---

Comments from the review on Reviewable.io

[glennw]

Squashed

[jdm]

@bors-servo: r=larsberg,jdm

[bors-servo]

📌 Commit 57d3bf1 has been approved by larsberg,jdm

[bors-servo]

⌛ Testing commit 57d3bf1 with merge 7b6c837...

[bors-servo]

💔 Test failed - gonk

[glennw]

@bors-servo: r=larsberg,jdm

[bors-servo]

📌 Commit eec3fad has been approved by larsberg,jdm

[bors-servo]

⌛ Testing commit eec3fad with merge 5e61eba...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2