1418840 - New sub-doc display item intermittently missing when transitioning between mochitests

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Found by comparing merged display list and non-retained one:
- Display list after merge with a previously-retained one:
> SolidColor p=0x7f1c26996218 f=0x7f1c35f67018(Viewport(-1)) key=47 bounds(0,0,76800,57960) layerBounds(0,0,76800,57960) visible(0,0,76800,57960) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform ref=0x7f1c35f67018 agr=0x7f1c35f67018 (opaque 0,0,76800,57960) (rgba 255,255,255,255) layer=0x7f1c308cc800
> [...]
> Border p=0x7f1c26995b18 f=0x7f1c268423f0(FrameOuter(iframe src=/tests/dom/media/mediasource/test/test_HaveMetadataUnbufferedSeek.html)(0) id:testframe) key=7 bounds(660,15564,30240,18240) layerBounds(660,15564,30240,18240) visible(0,0,76800,57960) componentAlpha(0,0,0,0) clip(0,0,76800,57960) asr(<0x7f1c35f671d8>) clipChain(<0,0,76800,57960> [0x7f1c35f671d8], <0,0,76800,57960> [root asr]) ref=0x7f1c35f67018 agr=0x7f1c35f670a8 layer=0x7f1c308ccc00
> Text p=0x7f1c26995018 f=0x7f1c268426e8(Text(0)"Show Non-Tests") key=62 bounds(600,33744,5040,1020) layerBounds(600,33744,5040,1020) visible(0,0,76800,57960) componentAlpha(600,33744,5040,1020) clip(0,0,76800,57960) asr(<0x7f1c35f671d8>) clipChain(<0,0,76800,57960> [0x7f1c35f671d8], <0,0,76800,57960> [root asr]) ref=0x7f1c35f67018 agr=0x7f1c35f670a8 ("Show Non-Tests") layer=0x7f1c308ccc00
- Non-retained display list:
> SolidColor p=0x7f1c3073b458 f=0x7f1c35f67018(Viewport(-1)) key=47 bounds(0,0,76800,57960) layerBounds(0,0,76800,57960) visible(0,0,76800,57960) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform ref=0x7f1c35f67018 agr=0x7f1c35f67018 (opaque 0,0,76800,57960) (rgba 255,255,255,255) layer=0x7f1c308cc800
> [...]
> Border p=0x7f1c2659aa18 f=0x7f1c268423f0(FrameOuter(iframe src=/tests/dom/media/mediasource/test/test_HaveMetadataUnbufferedSeek.html)(0) id:testframe) key=7 bounds(660,15564,30240,18240) layerBounds(660,15564,30240,18240) visible(0,0,76800,57960) componentAlpha(0,0,0,0) clip(0,0,76800,57960) asr(<0x7f1c35f671d8>) clipChain(<0,0,76800,57960> [0x7f1c35f671d8], <0,0,76800,57960> [root asr]) ref=0x7f1c35f67018 agr=0x7f1c35f670a8 layer=0x7f1c308ccc00
> SubDocument p=0x7f1c3073b118 f=0x7f1c226a0018(Viewport(-1)) key=49 bounds(780,15684,30000,18000) layerBounds(780,15684,30000,18000) visible(0,0,76800,57960) componentAlpha(0,0,0,0) clip(780,15684,30000,18000) asr(<0x7f1c35f671d8>) clipChain(<780,15684,30000,18000> [0x7f1c35f671d8], <0,0,76800,57960> [root asr]) ref=0x7f1c35f67018 agr=0x7f1c35f670a8
>   LayerEventRegions p=0x7f1c2659ae18 f=0x7f1c226a0018(Viewport(-1)) key=26 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,76800,57960) componentAlpha(0,0,0,0) clip(780,15684,30000,18000) asr(<0x7f1c35f671d8>) clipChain(<780,15684,30000,18000> [0x7f1c35f671d8], <0,0,76800,57960> [root asr]) ref=0x7f1c35f67018 agr=0x7f1c35f670a8 (hitRegion < (x=780, y=15684, w=30000, h=18000); >)
>   CanvasBackgroundColor p=0x7f1c3073b018 f=0x7f1c226a00a8(Canvas(html)(-1)) key=15 bounds(780,15684,30000,18000) layerBounds(780,15684,30000,18000) visible(780,15684,30000,18000) componentAlpha(0,0,0,0) clip(780,15684,30000,18000) asr(<0x7f1c35f671d8>) clipChain(<780,15684,30000,18000> [0x7f1c35f671d8], <0,0,76800,57960> [root asr]) uniform ref=0x7f1c35f67018 agr=0x7f1c35f670a8 (rgba 0,0,0,0)
> Text p=0x7f1c3073b358 f=0x7f1c268426e8(Text(0)"Show Non-Tests") key=62 bounds(600,33744,5040,1020) layerBounds(600,33744,5040,1020) visible(0,0,76800,57960) componentAlpha(600,33744,5040,1020) clip(0,0,76800,57960) asr(<0x7f1c35f671d8>) clipChain(<0,0,76800,57960> [0x7f1c35f671d8], <0,0,76800,57960> [root asr]) ref=0x7f1c35f67018 agr=0x7f1c35f670a8 ("Show Non-Tests") layer=0x7f1c308ccc00

Notice that the 'SubDocument' tree is missing. (The issue is not visually obvious, as a lot happens during mochitests!)

A try [1] with the verifying code reveals that this happens a lot, e.g.: Search ' **** ' in [2], it happens 441 times in this 689-test run.

A naive solution [3] (that forces a full rebuild when a sub-doc does not have any frames yet) removed the issue.
Discussing with Matt Woodrow, he thought this was probably not the appropriate solution, as it fixes symptoms instead of the underlying issue.
I haven't been able to progress on this much further, so I'm opening this bug now so it's not forgotten.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9355b789f9920dc5ce0bb0a0a97fd3783d43d5c
[2] https://public-artifacts.taskcluster.net/BifLkLJhRUGjJi3LMvk2ow/0/public/logs/live_backing.log
[3] https://hg.mozilla.org/try/rev/3a78cc2176e0a477041d0a9feff9dc203bf17071

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Extracts from my discussion with Matt, as it has some valuable information:
<mattwoodrow> Is it that nsSubDocumentFrame has GetSubdocumentPresShellForPainting and the RDL logic doesn’t so we might be finding a different pres shell? one without a root frame, and one with
<gerald> indeed the pres shell used for the subdoc is different during GetModifiedFrames and during the non-retained build
<mattwoodrow> I think we need GetModifiedFrames to find the same presshell as nsSubDocumentFrame::BuildDisplayList does
<mattwoodrow> I also suspect if that ever changes, we want to treat the entire doc as invalid
<mattwoodrow> Possibly there’s code that runs when the changes happens, and we can call MarkNeedsDisplayItemRebuild on the nsSubDocumentFrame but because of the presShell that GetModifiedFrames sees, there are no frames there can could be marked
<mattwoodrow> Well, we’d call MarkNeedsDisplayItemRebuild on the subdoc frame which is in the outer document
<mattwoodrow> But these are separate bugs and I think we want to fix both

<gerald> Do you think my "fix" is the wrong way to go? (preventing a partial rebuild when there are no frames)
<mattwoodrow> Yeah, I think it is, because it’s a symptom, not the underlying issue
<mattwoodrow> It seems plausible that both documents in question could have frames but we’d be considering invalidations for the one that isn’t painted
<mattwoodrow> I think we want to duplicate the GetSubdocumentPresShellForPainting logic but in a way that works with the document hierarchy

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: subdoc WIP

I think this is roughly what we want to do to fix this.

Haven't tested it, but the asserts should fire if I'm wrong on something.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try (along with the enabled retained-vs-non-retained checker from bug 1420298) :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e985a9882012ea5a13a2428a3f97a05ce5560a26

Good news: I cannot see retained builder issues related to this bug.

Bad news: Crashes in your code (instead?), e.g.:
https://treeherder.mozilla.org/logviewer.html#?job_id=147568120&repo=try&lineNumber=6892

I think it's because you're not handling a null presShell at line 540:
> 539  nsIPresShell* presShell = aDocument->GetShell();
> 540  nsView* rootView = presShell->GetViewManager()->GetRootView();
I'll add a null-check for that and try again...

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try with the null check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=87bc78105936a540358e103646b0fdb6f8dce013
Much better with no more crashes.

And I don't see missing sub-docs anymore. For 'mda' tests, there used to be literally *hundreds* of them.

However, there are new issues revealing themselves *under* the sub-doc.
They are relatively few, only a handful per mda test run.
It makes a bit of sense, because most of the mochitest actual activity happens in the sub-doc, and the previous missing sub-doc issues swamped tests (so I may have missed those other few ones if they happened) and/or just removed them from view because the sub-doc was just not there to start with.

So I think this fix definitely helps, and we can chase the other issues separately after that.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1418840-mattw-wip-fixed.patch

Small update to Matt's patch, adding a nullptr check of the first presshell.

This fixed sub-doc issues in try tests.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6ea086117d
Use the modified frames list for the subdoc that painting will use, which isn't always the current subdoc of the FrameOuter. r=mstange

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7d6ea086117d