Closed Bug 1529594 Opened 2 years ago Closed 2 years ago

Make TabParent::GetTabParentFromLayersId() work for out-of-process iframes

Categories

(Core :: DOM: Content Processes, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: hsivonen, Unassigned)

References

Details

Steps to reproduce:

  1. Apply the stack of 6 patches from bug 1524240 and backwards from Phabricator.
  2. Create a pref browser.fission.oopif.attribute with the value true.
  3. Enable WebRender.
  4. Navigate to https://hsivonen.fi/fission-host.html

Actual results:
Crash on assertion. APZ/WebRender had access to LayersIds that TabParent::GetTabParentFromLayersId didn't know about.

Expected results:
Expected TabParent::GetTabParentFromLayersId to be able to map LayersIds associated with the top levels of out-of-process iframes.

nika, rhunt, is this something that's supposed to work but is broken or something that hasn't been implemented yet?

Flags: needinfo?(rhunt)
Flags: needinfo?(nika)

Hmm, so in RemoteFrameParent, we do call InitRendering, which will add the entry to the table, so I was expecting it'd work out.

We also (AFAICT) only remove the element from the table in ActorDestroy which should be running at the right time.

I'm a bit surprised by this not working out. :rhunt, could you perhaps take a look at it?

Flags: needinfo?(nika)

Sorry about this. This was a bug on my side. (Yay for C++'s unsafe defaults.)

Flags: needinfo?(rhunt)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID

In addition to the bug on my side, it still appears that WR hit testing can yield a LayersId that TabParent::GetTabParentFromLayersId() does not know about.

Status: RESOLVED → REOPENED
Flags: needinfo?(rhunt)
Resolution: INVALID → ---

Do you know if that's during a "steady state" (tabs aren't being opened/closed recently)? Because there might be race conditions around tabs opening and closing where the layers id is deregistered from the table in TabParent before you try to look it up. If it happens during a steady state that would be surprising to me.

If that's happening then most likely we're hitting an item inside a WR pipeline that doesn't match a layers tree. This happens in the AsyncPipelineManager (for async video updating) - we create an "async pipeline" here and then update it as new video frames come in. However that display list should just have the one image item and no hit-test items, so I'd be surprised if we're producing that as a hit result. But it might be worth checking if the WrPipelineId coming back from the hit-test matches one that was passed in by the AsyncPipelineManager.

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

Do you know if that's during a "steady state" (tabs aren't being opened/closed recently)? Because there might be race conditions around tabs opening and closing where the layers id is deregistered from the table in TabParent before you try to look it up. If it happens during a steady state that would be surprising to me.

I don't have that level of detail, but that is a plausible explanation. I could try either dropping events or delivering them to the top-most content process if there's no TabParent by LayersId. Not sure which is better.

(In reply to Henri Sivonen (:hsivonen) from comment #6)

or delivering them to the top-most content process if there's no TabParent by LayersId.

I'll try this.

(In reply to Henri Sivonen (:hsivonen) from comment #4)

In addition to the bug on my side, it still appears that WR hit testing can
yield a LayersId that TabParent::GetTabParentFromLayersId() does not know
about.

What assert are you getting? Does it look related to bug 1527380? Until that's fixed, webrender doesn't work for me with the oop-if patches.

Flags: needinfo?(rhunt)

(In reply to Ryan Hunt [:rhunt] from comment #8)

(In reply to Henri Sivonen (:hsivonen) from comment #4)

In addition to the bug on my side, it still appears that WR hit testing can
yield a LayersId that TabParent::GetTabParentFromLayersId() does not know
about.

What assert are you getting?

It was an assertion I added to check that TabParent::GetTabParentFromLayersId() return non-null. I took it away per comment 7. Let's close this again until I have more actionable info.

Does it look related to bug 1527380?

No, it seemed to happen when bug 1527380 didn't stop things first.

Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.