Closed Bug 1287004 Opened 4 years ago Closed 4 years ago

Support remote <browser> elements in windowless browsers

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: billm, Assigned: dvander)

References

(Blocks 2 open bugs)

Details

(Whiteboard: triaged)

Attachments

(1 file, 2 obsolete files)

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)
Attached 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)
Attached patch bug1287004.patchSplinter Review
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: 4 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
See Also: → 1644915
You need to log in before you can comment on or make changes to this bug.