1582372 - pdf.js draws parts of pages when gfx.canvas.remote=true

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[David Parks [:handyman]]

I am trying out gfx.canvas.remote=true and this appeared in the first couple of days. I don't know if its remoting or WR or what so I leave that to Bob.

Viewing a fairly large pdf [1], opened from a file:// on my local drive, with the "odd spreads" option (so 2 pages side-by-side) and the sidebar opened to an outline, I sometimes get strange graphics glitches.
I've seen a page go missing (as in not appear while scrolling -- just grey background).
I've seen pages appear partially -- like only on the top 10% of the display -- but always rectangles. Scrolling then would produce strange strobing effects that behaved as though the missing part was due to a clip region.
Another weird part is that I have the 2 pages side-by-side setup so one going missing seems odd.
Probably the weirdest part is that the rendering will update itself -- sometimes in a few seconds and sometimes, seemingly, in response to window events like mouseout. For example, the top 10% deal I mentioned above. When I moved the mouse to the taskbar to take a screenshot (after much scrolling and waiting -- pretty sure it wasn't just slow), it rendered fully.
Finally, I have not seen more than one page trashed at a time. Again, since there are always at least 2 on screen, it is somehow about the data.

I've seen this twice in several hours of using pdf.js. I haven't been able to do it since. I use pdf.js with local files a lot though so, if this becomes a thing, I can always run a special build to collect info.

[1] https://github.com/hmemcpy/milewski-ctfp-pdf

Comment 1

[David Parks [:handyman]]

Bob, I know you said that you have reproduced this but don't have a cause -- I've seen it too. This time, I got some pix, which was easy since the tie to window events I thought I saw last time wasn't apparently real. I hadn't been using the view -- it was the front tab in a background window that I'm sure was fine when I tabbed away but I noticed had developed these graphics glitches while it was in the background.

I'm still open to running a special build if you come up with something that will help diagnose this. But in the mean time, some pix for posterity.

Comment 2

[David Parks [:handyman]]

Attachment: pdfjs-error-1.png

Comment 3

[David Parks [:handyman]]

Attachment: pdfjs-error-2.png

Comment 4

[David Parks [:handyman]]

Attachment: pdfjs-error-3.png

Comment 5

[David Parks [:handyman]]

Attachment: pdfjs-error-4.png

Comment 6

[Bob Owen (:bobowen)]

OK, I think I've finally worked out what is going wrong here.
There are two (possibly three) issues that have similar symptoms.

1. If a texture isn't forwarded before it is deleted. the descriptor for it is still in the map in the GPU process. If a new DrawTarget is created in the content process with the same pointer, then it can be forwarded before the new descriptor is replaced in the GPU process and the old invalid one is picked up. To fix this I need to make sure the descriptor is removed when the texture is deleted in the GPU process.

2. When I clean up all the canvas IPC classes (if they have not been used for 5 seconds), the recorder can still have stored objects and the associated objects still be alive in the content process. This is particularly a problem with tab switch. I need to make sure the recorder is empty (and hence all objects destroyed) before cleaning it up.

3. The way that the CompositableTextureHostRef counting happens with webrender enabled means that the TextureHosts remain read locked in the GPU process for longer. This causes an issue with the check at [1]. This would need to be increased to at least 6 (depending on webrender and gfx.webrender.triple-buffering.enabled). I'm actually not at all sure that the way the holding of the reference is extended is necessary, because as far as I can tell the texture is used (locked and unlocked) well before it would normally have been released anyway. I'm also not sure whether this causes these symptoms, but it might contribute to the above two being more frequent. Given the uncertainty, I'll look at this in another bug.

[1] https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/gfx/layers/PersistentBufferProvider.cpp#309

Comment 7

[Bob Owen (:bobowen)]

Try push with patches for 1 and 2 from comment 6:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3d66b68fa5b5ec7687a26458252def1e16625ca

handyman, jimm: it would be great if you could try the shippable windows build from this to see if you still have a problem. I can't reproduce with these.

Comment 8

[David Parks [:handyman]]

Will do.

Comment 9

[Bob Owen (:bobowen)]

Attachment: Bug 1582372 Part 1: Ensure recorded texture SurfaceDescriptor is not in map for compositor when texture dies. r=mattwoodrow

Comment 10

[Bob Owen (:bobowen)]

Attachment: Bug 1582372 Part 2: Only clean up Canvas IPC objects when recorder has no stored objects. r=jrmuizel

The recorder can stil contain UnscaledFonts, but they are never removed
currently, so we can't wait for them and they will be re-recorded safely anyway.

Depends on D49757

Comment 11

[Jim Mathies [:jimm]]

(In reply to Bob Owen (:bobowen) from comment #7)

Try push with patches for 1 and 2 from comment 6:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3d66b68fa5b5ec7687a26458252def1e16625ca

handyman, jimm: it would be great if you could try the shippable windows build from this to see if you still have a problem. I can't reproduce with these.

will do!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf450077b7d3
Part 1: Ensure recorded texture SurfaceDescriptor is not in map for compositor when texture dies. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/1b369258bfad
Part 2: Only clean up Canvas IPC objects when recorder has no stored objects. r=jrmuizel

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cf450077b7d3
https://hg.mozilla.org/mozilla-central/rev/1b369258bfad