1287004 - Support remote <browser> elements in windowless browsers

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This will be one of the last things we do for bug 1190679.

I tried this a pretty big problem. Currently we create a <browser> element inside a windowless docshell [1]. The windowless docshell uses a PuppetWidget. The layers code doesn't seem to be able to cope with hosting remote content inside of a PuppetWidget. In the parent process, we crash at [2] because GetRemoteRenderer() returns null. That makes sense since we don't have a CompositorChild for the windowless docshell.

In the child process, we assert at [3]. I don't really understand that part.

Matt, can you offer any advice here? Should we try to create a CompositorChild for the windowless docshell, or should we try to avoid accessing that object. We're not going to ever draw these things, but we do need to support things like canvas getImageData (but only in the remote browser element).

[1] http://searchfox.org/mozilla-central/rev/e269c68bc594b4a858624d73f0d9eb002f3c0844/xpfe/appshell/nsAppShellService.cpp#494

[2] http://searchfox.org/mozilla-central/rev/e269c68bc594b4a858624d73f0d9eb002f3c0844/layout/ipc/RenderFrameParent.cpp#123

[3] http://searchfox.org/mozilla-central/rev/e269c68bc594b4a858624d73f0d9eb002f3c0844/gfx/layers/ipc/ShadowLayers.cpp#243

Comment 1

[Matt Woodrow (:mattwoodrow)]

TabChild::InitRenderingState is supposed to be called when we setup the tab in the child document, and that provides the shadow manager (PLayerTransactionChild) to use for forwarding layers to the parent.

If we don't want to ever render, we shouldn't need to do this.

The call site at [3] seems like it is happening from rendering though, can we stop this from happening?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: create compositor child for windowless browsers

This was the easiest way I could think of doing this. It seems to work okay.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/663cde4dd044
Give PuppetWidgets in parent process a compositor bridge (r=mattwoodrow)

Comment 4

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/908976ec5526 for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=35802161&repo=mozilla-inbound

Comment 5

[Phil Ringnalda (:philor)]

Or if you like Android crashes, as nobody does, https://treeherder.mozilla.org/logviewer.html#?job_id=35798400&repo=mozilla-inbound

Or if you like Windows assertions and then crashes, https://treeherder.mozilla.org/logviewer.html#?job_id=35800549&repo=mozilla-inbound

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, this was green a few days ago.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks like other work is also blocking this (bug  1280028). David said he would work on it soon.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

It looks like the ClientLayerManager approach would break because RenderFrame assumes it'll have a CompositorBridgeChild singleton. In addition we'd have no CompositorWidget, and we would probably get into trouble anyway without an HWND (at least, for d3d11).

Is there any reason we can't just use a BasicLayerManager instead? It seems to pass all the extension tests, and try looks good as well.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: bug1287004.patch

This version does not use a retained BasicLayerManager.

Comment 11

[:Gijs (he/him)]

FWIW, the previous patch fixed the crashes I was seeing in bug 1280028 (haven't checked the new one, but I presume it'll work, too). I wonder if it's worth a crash test of sorts? :-)

Comment 12

[:Gijs (he/him)]

Testing with this more, my stdout/stderr is spammed with lots of messages like this:


|[C90][GFX1-]: ClientLayerManager::BeginTransaction with IPC channel down. GPU process may have died. (t=36.4959) [GFX1-]: ClientLayerManager::BeginTransaction with IPC channel down. GPU process may have died.

Could be coincidental, but seems related. Doesn't happen on today's nightly run with the same profile (but without this change and my patch for bug 1338215).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61e17a4c8397
Give browser-process PuppetWidgets a BasicLayerManager. (bug 1287004, r=mattwoodrow)

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/61e17a4c8397