constellation: Don't ignore inconsistent frame-tree states when determining when to take a screenshot.

[emilio]

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #__ (github issue number if applicable).

• There are tests for these changes OR
• These changes do not require tests because _____

Return a significative error instead.

I think this is the cause of most iframe-related intermittents: we stop
iterating early, so we don't detect frames that aren't closed now.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/constellation/constellation.rs

[emilio]

I plan to do a few try runs to see if this fixes any intermittent. Let's see.

r? @asajeffrey

@bors-servo: try

[bors-servo]

⌛ Trying commit 36b8c79 with merge 7f17ccb...

[bors-servo]

💔 Test failed - mac-rel-wpt

[emilio]

@bors-servo: retry

• Weird mac directory not empty errors... Not sure if it'd be fixed retrying.

[bors-servo]

⌛ Trying commit 36b8c79 with merge 70eda98...

[bors-servo]

💔 Test failed - mac-rel-css

[highfive]

New code was committed to pull request.

[emilio]

The initial code was incorrect (we already weren't bailing out, and I don't know how to read code). This code returns a ReadyToSave::InconsistentFrameTreeState if we find an unexpected pipeline missing.

I'm curious to see if this ever causes a timeout.

@bors-servo: try

• Previous failure was git things.

[bors-servo]

⌛ Trying commit 821ce5a with merge c142c9f...

[bors-servo]

💔 Test failed - mac-rel-wpt

[jdm]

@bors-servo: retry

• mac folder not empty

[bors-servo]

⌛ Trying commit 821ce5a with merge 882768d...

[bors-servo]

💔 Test failed - mac-rel-css

[jdm]

@bors-servo: retry

• git timeout

[bors-servo]

⌛ Trying commit 821ce5a with merge c43f78c...

[asajeffrey]

Sorry about the delay, this was a casualty of MozLondon.

I'm going to echo @ConnorGBrewster: can you say which intermittents this will fix? Looking through the code, I can't see what behaviour will be different, since the only difference is when the already-closed frames are filtered out. Currently they're filtered out by the iterator, with your patch they're filtered out by the consumer of the iterator. I may be missing something, can you give us a scenario where there's a difference?

[emilio]

Yes, so the point is to try tackling the intermittents where we have iframes that render nothing, presumably because we take the screenshot too early.

And yes, almost everything is getting refiltered as before, except...

---

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

components/constellation/constellation.rs, line 1864 [r4] (raw file):

                FrameTreeIteratorItem::PipelineNotFound(frame_id, frame) => {
                    warn!("Pipeline {:?} for frame {:?} not found", frame.current, frame_id);
                    return ReadyToSave::InconsistentFrameTreeState;

These lines, where instead of skipping the frames and assuming everything is ok, we return a state that isn't ReadyToSave::Ready, so we don't take the screenshot yet.

---

Comments from Reviewable

[asajeffrey]

Ah, I see.

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

components/constellation/constellation.rs, line 261 [r4] (raw file):

}

enum FrameTreeIteratorItem<'a> {

Rather than a whole new type, it might be better to use Result<&'a Frame, ()>? (Or some other error type if you prefer.)

Or leave FrameTreeIterator alone, and add a recursive method that checks to make sure all frame lookups succeed? (We're into code readability here, not issues about correctness / efficiency.)

---

components/constellation/constellation.rs, line 1864 [r4] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

These lines, where instead of skipping the frames and assuming everything is ok, we return a state that isn't ReadyToSave::Ready, so we don't take the screenshot yet.

Ah, I see the idea now!

---

Comments from Reviewable

[jdm]

It may be worth reenabling #10760 with this change, as that was seen to be a blank iframe.

[emilio]

@jdm: agreed, will do on my next push.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

components/constellation/constellation.rs, line 261 [r4] (raw file):

Previously, asajeffrey (Alan Jeffrey) wrote…

Rather than a whole new type, it might be better to use Result<&'a Frame, ()>? (Or some other error type if you prefer.)

Or leave FrameTreeIterator alone, and add a recursive method that checks to make sure all frame lookups succeed? (We're into code readability here, not issues about correctness / efficiency.)

What about adding a filter over this iterator, that just yields the `(pipeline, frame)` pair? So we would have a method like `infallible_frame_tree_iter()` or something, that filters over this one to yield a more convenient value when you just don't care about the inconsistent frame tree states.

---

Comments from Reviewable

[emilio]

Refactored it, I think it's more legible now, and also re-enabled sslfail.html.

Also fixed a bug on revoke_paint_permission in which we'll accidentally revoke paint permission to all the pipelines associated to a frame.

re-r? @asajeffrey

@bors-servo: try

[bors-servo]

⌛ Trying commit 07b98f9 with merge 8955896...

[bors-servo]

💔 Test failed - mac-rel-wpt

[highfive]

  ▶ FAIL [expected PASS] /_mozilla/mozilla/sslfail.html
  └   → /_mozilla/mozilla/sslfail.html 9d627658adfcf6a768983b0fbae728aa4edab3e0
/_mozilla/mozilla/sslfail-ref.html 0a7088f19a748be896762e4a5cbb67467342f6e8
Testing 9d627658adfcf6a768983b0fbae728aa4edab3e0 == 0a7088f19a748be896762e4a5cbb67467342f6e8

[cbrewster]

The reftest still shows /_mozilla/mozilla/sslfail.html as a blank iframe :(

[emilio]

Yup, I should arguably hide myself :/

Anyway, I think part of these changes are good to land, at least since they ignore the extra pipeline lookup, and the error handling associated with it, and also it fixes the bug of us calling revoke_paint_permission on everything.

I'd like feedback from @asajeffrey before keeping working on it though.

[asajeffrey]

I'm flying to NYC at the moment, I'll comment when I arrive!

[asajeffrey]

Oops, dropped the ball on this one.

How about if we restore FrameTreeIterator back to the way it was, but add a FrameIdTreeIterator which iterates through the FrameIds rather than the Frames? Then the code that cares about lookup failures can do the frameid lookup themselves.

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[asajeffrey]

@emilio is this still active?

[emilio]

I need to find more time to work on it, closing in the meantime since it's not super hi-pri.

[asajeffrey]

OK, cc me if you submit a new PR!