Support remote <browser> elements in windowless browsers

RESOLVED FIXED in Firefox 54

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: billm, Assigned: dvander)

Tracking

(Blocks 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: triaged)

Attachments

(1 attachment, 2 obsolete attachments)

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
Flags: needinfo?(matt.woodrow)
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?
Flags: needinfo?(matt.woodrow)
Summary: Use remove <browser> element for background page → Use remote <browser> element for background page
Whiteboard: triaged
Blocks: 1282021
This was the easiest way I could think of doing this. It seems to work okay.
Attachment #8788263 - Flags: review?(matt.woodrow)
Attachment #8788263 - Flags: review?(matt.woodrow) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/663cde4dd044
Give PuppetWidgets in parent process a compositor bridge (r=mattwoodrow)
Sorry, this was green a few days ago.
Assignee: nobody → wmccloskey
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: -- → P2
webextensions: --- → +
Priority: P2 → P1
Duplicate of this bug: 1280028
It looks like other work is also blocking this (bug  1280028). David said he would work on it soon.
Flags: needinfo?(dvander)
Posted patch fix (obsolete) — Splinter Review
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.
Assignee: wmccloskey → dvander
Attachment #8788263 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(dvander)
Attachment #8834272 - Flags: review?(matt.woodrow)
This version does not use a retained BasicLayerManager.
Attachment #8834272 - Attachment is obsolete: true
Attachment #8834272 - Flags: review?(matt.woodrow)
Attachment #8835258 - Flags: review?(matt.woodrow)
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? :-)
Attachment #8835258 - Flags: review?(matt.woodrow) → review+
Blocks: 1338215
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).
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61e17a4c8397
Give browser-process PuppetWidgets a BasicLayerManager. (bug 1287004, r=mattwoodrow)
https://hg.mozilla.org/mozilla-central/rev/61e17a4c8397
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Summary: Use remote <browser> element for background page → Support remote <browser> elements in windowless browsers
Blocks: 1339144
See Also: → 1352287
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.