Contents on popup of Addons are shown as white when gfx.webrender.split-render-roots is true

REOPENED
Assigned to

Status

()

defect
P3
normal
REOPENED
2 months ago
2 days ago

People

(Reporter: yamadat501, Assigned: dthayer)

Tracking

(Blocks 1 bug, {platform-parity})

Trunk
mozilla68
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 disabled, firefox69 disabled)

Details

Attachments

(6 attachments)

Reporter

Description

2 months ago
Posted image popup_bug.jpg

Similar bugs like Bug 1512998 have been reported, but I report this as new bug because this bug trigger is different from others.

STR:

  1. Install ublock origin and set its toolbar icon.
  2. Set gfx.webrender.split-render-roots true.
  3. Open ublock origin's popup by clicking icon.

Expected result:
Contents on popup are shown.

Actual result:
Contents on popup are white(But works normally)

Priority: -- → P3

Not reproducible on Macbook with Nightly 68.0a1 (2019-04-30) (64-Bit)
built from https://hg.mozilla.org/mozilla-central/rev/90234f4c094dcc794df28fdd464793dfe065f943.

Comment 2

2 months ago

At least, I can reproduce on Windows10.
Build ID 20190430121130
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: platform-parity
OS: Unspecified → Windows
See Also: → 1548688
Assignee: nobody → dothayer
Assignee

Comment 3

Last month

I'm not sure when/if this stopped working, but it appears the -moz-stack
display of the stack element doesn't send us through the admittedly
brittle pathways that allow us to create nsDisplayRenderRoots. This
brittleness is a consequence of copying the use of the "layer"
attribute and is discussed in the core document splitting bug. It's
outside the scope of this bug to fix that.

Assignee

Comment 4

Last month

Please advise if there is a better alternative. After ensuring that
extension popups are wired up to create an nsDisplayRenderRoot, the
bottom bit of the popup's content is cut off. This fixes that issue,
but I'm not certain if it is the most robust option.

Depends on D31006

Comment 5

Last month
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/888907b9ffc3
Set renderroot of extension popup on browser element r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/cb6320b272f9
Use offset relative to reference frame for nsDisplayRenderRoot r=mattwoodrow

Comment 6

Last month
bugherder
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Reporter

Comment 7

Last month

I checked 20190517214313 nightly(rev 1ae707852b608ea77dc82c892f25e169cbc316b5) contains these patches and the problem still happened for me.

Comment 8

Last month

I also reproduce the problem.
Build ID 20190517214313
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0
Built from: https://hg.mozilla.org/mozilla-central/rev/1ae707852b608ea77dc82c892f25e169cbc316b5

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

I see the same issue without enabling splitting. The panel is white and gets drawn on hover. I wonder if it's a different bug.

Assignee

Comment 10

28 days ago

Hmm. The fix is working for me on OSX where I developed it, but not on Windows. Taking another look. If anyone is still experiencing this on OSX though please let me know.

Assignee

Comment 11

27 days ago

Matt, I'm thinking this might be the wrong approach overall. Right now we have to play whack-a-mole with all the things that house stuff rendered in a content process, but I think it's going to make more sense long-term if we think of it more on an opt-in basis. "Normal" content in a window that has chrome at the top should get a render root of content, and everything else, be it the contents of a WebExtensions popup or the contents of a picture-in-picture player, should get a default render root. However, I don't know of any way to conveniently access that information. I don't suppose you might?

Specifically, I think it would be nice if I could ask "is this normal content?" either in WebRenderLayerManager or in WebRenderBridgeParent. I don't suppose you have any ideas on this?

Flags: needinfo?(matt.woodrow)

Can we avoid content needing to know which RenderRoot it belongs to?

It seems that content knows which iframe it belongs to (PipelineId), and the parent process knows which iframes belong to which RenderRoot.

So it seems like a parent SetDisplayList call (for a given RenderRoot) could include a list of what iframes are within that DL.

Then when we get a new DL from content, we can ask the root WebRenderBridgeParent which RenderRoot we belong to, and access the appropriate WrAPI.

Flags: needinfo?(matt.woodrow)
Assignee

Comment 13

24 days ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #12)

Can we avoid content needing to know which RenderRoot it belongs to?

It seems that content knows which iframe it belongs to (PipelineId), and the parent process knows which iframes belong to which RenderRoot.

So it seems like a parent SetDisplayList call (for a given RenderRoot) could include a list of what iframes are within that DL.

Then when we get a new DL from content, we can ask the root WebRenderBridgeParent which RenderRoot we belong to, and access the appropriate WrAPI.

Are there potential ordering difficulties here? Can I always guarantee that the display list containing the iframe comes in before the content display list? Assuming yes, it would also be simplest if I could just do this by setting CompositorBridgeParent::GetIndirectShadowTree(remoteLayersId)->mWrBridge->mRenderRoot from the root WebRenderBridgeParent's RecvSetDisplayList, but that only works if I can guarantee that the content WebRenderBridgeParent has been created by the time we get the displaylist containing the iframe.

Flags: needinfo?(matt.woodrow)

(In reply to Doug Thayer [:dthayer] from comment #13)

Are there potential ordering difficulties here? Can I always guarantee that the display list containing the iframe comes in before the content display list? Assuming yes, it would also be simplest if I could just do this by setting CompositorBridgeParent::GetIndirectShadowTree(remoteLayersId)->mWrBridge->mRenderRoot from the root WebRenderBridgeParent's RecvSetDisplayList, but that only works if I can guarantee that the content WebRenderBridgeParent has been created by the time we get the displaylist containing the iframe.

Yeah, I think that can happen (and probably does, I think we try to pre-render content when tab switching).

That's a bit of a problem, since it probably also means we can't have the parent notify content of which RR it belongs to.

Having the content process try to guess, and trying to keep them in sync feels pretty error prone though :(

Flags: needinfo?(matt.woodrow)
Assignee

Comment 15

24 days ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)

Yeah, I think that can happen (and probably does, I think we try to pre-render content when tab switching).

That's a bit of a problem, since it probably also means we can't have the parent notify content of which RR it belongs to.

Having the content process try to guess, and trying to keep them in sync feels pretty error prone though :(

Hmm, can we just defer processing a display list in WebRenderBridgeParent::RecvSetDisplayList if we haven't yet received an indication of where it belongs? I'm not sure exactly what that would look like - moving the whole RenderRootDisplayListData into a map somewhere, and making sure we clear it out if we get a DeallocateLayerTreeId?

Kats, does that sound basically sane or no?

Flags: needinfo?(kats)

(In reply to Matt Woodrow (:mattwoodrow) from comment #14)

Yeah, I think that can happen (and probably does, I think we try to pre-render content when tab switching).

So yes, that's true, but IIRC in this situation we never actually do anything with the content display list. Because it's not attached anywhere to the parent display list, it doesn't participate in the scene build or rendering, it mostly just gets ignored on the WR side until the parent-side DL with the iframe element shows up.

(In reply to Doug Thayer [:dthayer] from comment #15)

Hmm, can we just defer processing a display list in WebRenderBridgeParent::RecvSetDisplayList if we haven't yet received an indication of where it belongs? I'm not sure exactly what that would look like - moving the whole RenderRootDisplayListData into a map somewhere, and making sure we clear it out if we get a DeallocateLayerTreeId?

Kats, does that sound basically sane or no?

... so I think this does sound basically sane, but it's going to add yet more implementation complexity. But absent a better solution (which I can't think of right now, given it's Friday evening) it sounds like something worth trying.

Flags: needinfo?(kats)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Assignee

Comment 23

13 days ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16)

... so I think this does sound basically sane, but it's going to add yet more implementation complexity. But absent a better solution (which I can't think of right now, given it's Friday evening) it sounds like something worth trying.

Quick update so no one's in the dark: turns out this is super annoying to do. Still working out all of the kinks and not sure when they'll stop popping up.

Assignee

Comment 24

12 days ago

This implements the idea of automatically setting a content proc's
render root based on the render root enclosing the iframe that
points to it. There was a bit of cleanup in here that was a bit
tricky to extract from the core patch revolving around how we
use the Api(...) helper. This was to avoid the situation where
we use the Api(...) helper before our render root is initialized,
when we don't actually have to. I.e., when we just want the root
WebRenderAPI in all cases.

An alternative to this approach could be to fully built out the
WebRender transactions and just queue those up to be sent. However,
transaction building has various side effects which are committed
before the transaction is actually sent, so we would have to build
out some scheme for deferring those as well. This seemed simpler.

Assignee

Comment 25

2 days ago

This splits out the inner bit of RecvEmptyTransaction to just iterate over
the documents once, rather than iterating over them individually. Originally
I ran into difficulties with this and then left it on the table, but I think
it was enabled by splitting out the epochs in pipeline info by renderroot.

You need to log in before you can comment on or make changes to this bug.