Support remote layer trees in popups on Windows

NEW
Assigned to

Status

()

Core
Graphics: Layers
P1
normal
2 months ago
7 hours ago

People

(Reporter: kmag, Assigned: spohl, NeedInfo)

Tracking

(Blocks: 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: tpi:+[qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 months ago
Since bug 630346, transparent popups on Linux use a BasicLayerManager to make it easier to compute shape masks. Unfortunately, as a side-effect of this, it's not possible for them to host remote frameloaders.

We need to be able to show remote browsers in popups for OOP WebExtensions, so I think we need to find a better solution for handling transparency here.
(Reporter)

Updated

2 months ago
Blocks: 1357487

Updated

2 months ago
Priority: -- → P3
Whiteboard: tpi:+

Comment 1

2 months ago
Marking as quantum flow p1 because this blocks our ability to run webextensions in a seperate process. However this is only for Linux and can understand if that gets triaged down because of our number of users on Linux.
Whiteboard: tpi:+ → tpi:+[qf:p1]
Whiteboard: tpi:+[qf:p1] → tpi:+[qf]
Linux only so p3
Whiteboard: tpi:+[qf] → tpi:+[qf:p3]
(Reporter)

Comment 3

2 months ago
It turns out I was wrong, and we also disable compositing for popups on other platforms. And force-enabling it causes weird issues with drop shadows and transparent areas.

I'm not sure what the solution for this is. I suppose we either need to find a way to support compositing in popups properly, or find a way to support RefLayers in BasicLayerManager. Perhaps we could have the compositor render to an intermediate surface, and draw that to the BLM.

Or we could possibly just embed a composited widget inside the popup widget, but I don't know if we could clip it properly for rounded corner support without running into the same transparency issues (and it feels an awful lot like windowed mode plugins).
Component: Widget: Gtk → Graphics: Layers
Priority: P3 → --
Summary: Support remote layer trees in popups on Linux → Support remote layer trees in popups
Whiteboard: tpi:+[qf:p3] → tpi:+[qf]
bas said he was going to look at some options here, since QF is highly motivated to have oop webextensions for 57.
Flags: needinfo?(bas)
Whiteboard: tpi:+[qf] → tpi:+[qf:p1]
Comment hidden (obsolete)
(Reporter)

Comment 6

2 months ago
It's not Linux-only. See comment 3.
Whiteboard: tpi:+[qf:p3] → tpi:+[qf:p1]
Ack, sorry for thrashing.
Flags: needinfo?(bas)
(In reply to Mike Conley (:mconley) from comment #7)
> Ack, sorry for thrashing.

Wait, I'm unclear now Mike, we -do- disable compositing for Pop-Ups on other platform. However, from what I understand the debate is about whether we need this for OOP WebExtensions. Mike, can you confirm that we do?
Flags: needinfo?(bas) → needinfo?(mconley)
(Reporter)

Comment 9

2 months ago
(In reply to Bas Schouten (:bas.schouten) from comment #8)
> Wait, I'm unclear now Mike, we -do- disable compositing for Pop-Ups on other
> platform. However, from what I understand the debate is about whether we
> need this for OOP WebExtensions. Mike, can you confirm that we do?

I can confirm that we do. In order to enable OOP WebExtensions, we need to be able to render remote layer trees in popups, and we currently can't do that in any widget that doesn't support compositing.
Flags: needinfo?(mconley)
Assignee: nobody → bas
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #9)
> (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > Wait, I'm unclear now Mike, we -do- disable compositing for Pop-Ups on other
> > platform. However, from what I understand the debate is about whether we
> > need this for OOP WebExtensions. Mike, can you confirm that we do?
> 
> I can confirm that we do. In order to enable OOP WebExtensions, we need to
> be able to render remote layer trees in popups, and we currently can't do
> that in any widget that doesn't support compositing.

Will it be sufficient for OOP web extensions if we supply a XUL attribute that can be set on a window to force using OMTC for this window?
Flags: needinfo?(mconley)
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #3)
> It turns out I was wrong, and we also disable compositing for popups on
> other platforms. And force-enabling it causes weird issues with drop shadows
> and transparent areas.

Did you try this on windows? Where did you see this issue, and if so which windows version? It seems to me like we're already disabling acceleration on Windows for transparent widgets. Using OMTC for it then -should- be okay afaict.
(Reporter)

Comment 12

a month ago
(In reply to Bas Schouten (:bas.schouten) from comment #10)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #9)
> > (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > > Wait, I'm unclear now Mike, we -do- disable compositing for Pop-Ups on other
> > > platform. However, from what I understand the debate is about whether we
> > > need this for OOP WebExtensions. Mike, can you confirm that we do?
> > 
> > I can confirm that we do. In order to enable OOP WebExtensions, we need to
> > be able to render remote layer trees in popups, and we currently can't do
> > that in any widget that doesn't support compositing.
> 
> Will it be sufficient for OOP web extensions if we supply a XUL attribute
> that can be set on a window to force using OMTC for this window?

No. I was considering doing that as a temporary workaround, but forcing compositing turns the transparent areas of the popups black, at least on Linux and OS-X, and causes strange drop shadow issues on OS-X.

(In reply to Bas Schouten (:bas.schouten) from comment #11)
> Did you try this on windows? Where did you see this issue, and if so which
> windows version? It seems to me like we're already disabling acceleration on
> Windows for transparent widgets. Using OMTC for it then -should- be okay
> afaict.

I haven't tested on Windows yet, only Linux and OS-X. My Windows VM is broken at the moment and I'm waiting for Windows test hardware.
Flags: needinfo?(kmaglione+bmo)
kmag answered, clearing ni.
Flags: needinfo?(mconley)
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #12)
> (In reply to Bas Schouten (:bas.schouten) from comment #10)
> > (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #9)
> > > (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > > > Wait, I'm unclear now Mike, we -do- disable compositing for Pop-Ups on other
> > > > platform. However, from what I understand the debate is about whether we
> > > > need this for OOP WebExtensions. Mike, can you confirm that we do?
> > > 
> > > I can confirm that we do. In order to enable OOP WebExtensions, we need to
> > > be able to render remote layer trees in popups, and we currently can't do
> > > that in any widget that doesn't support compositing.
> > 
> > Will it be sufficient for OOP web extensions if we supply a XUL attribute
> > that can be set on a window to force using OMTC for this window?
> 
> No. I was considering doing that as a temporary workaround, but forcing
> compositing turns the transparent areas of the popups black, at least on
> Linux and OS-X, and causes strange drop shadow issues on OS-X.
> 
> (In reply to Bas Schouten (:bas.schouten) from comment #11)
> > Did you try this on windows? Where did you see this issue, and if so which
> > windows version? It seems to me like we're already disabling acceleration on
> > Windows for transparent widgets. Using OMTC for it then -should- be okay
> > afaict.
> 
> I haven't tested on Windows yet, only Linux and OS-X. My Windows VM is
> broken at the moment and I'm waiting for Windows test hardware.

I will run some more tests, if those are successful, I will enable this on Windows at least. For OSX we probably need input from Jeff.
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jmuizelaar) → needinfo?(mstange)
For Mac we need to do some of the work from bug 1291457 and then some more work to get the shadow right when we enable OpenGL compositing for them. On Mac, using OpenGL is a prerequisite for off-main-thread compositing, which is a prerequisite for rendering remote content.
We should probably have a separate bug for the Mac part of this.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #15)
> For Mac we need to do some of the work from bug 1291457 and then some more
> work to get the shadow right when we enable OpenGL compositing for them. On
> Mac, using OpenGL is a prerequisite for off-main-thread compositing, which
> is a prerequisite for rendering remote content.
> We should probably have a separate bug for the Mac part of this.

That sounds like a good idea, I can't resolve that part of the problem, I'll make sure Windows gets done. Is this a fundamental limitation about software composition on OS X, or is this just that we don't have code to composite non-OGL off the main thread?
(Reporter)

Comment 17

a month ago
I have some initial patches to fix bug 1362621 that add a remote flag to popups and widgets that have remote browsers, and force enable compositing and APZ for those widgets. They don't fix any of the rendering issues for composited, but should be good enough for testing.

I'm going to upload them for review in a separate bug you (bas and mstange) for review, and if they overlap or conflict with any of the work that you're doing for this, I'll update them to deconflict.
(Reporter)

Updated

a month ago
Depends on: 1365660
This seems to be mostly fine on try, there is one issue here:

https://treeherder.mozilla.org/logviewer.html#?job_id=99827640&repo=try&lineNumber=7639

There's a bunch of those crashes in M-e10s, Mike or Jim, do you have any idea what these are about? These don't look graphics related so it seems like they'd somehow be related to something in widget and what happens on windows when popups get OMTC.
Flags: needinfo?(mconley)
Flags: needinfo?(jmathies)

Updated

a month ago
Priority: -- → P1

Comment 19

a month ago
(In reply to Bas Schouten (:bas.schouten) from comment #18)
> This seems to be mostly fine on try, there is one issue here:
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=99827640&repo=try&lineNumber=7639
> 
> There's a bunch of those crashes in M-e10s, Mike or Jim, do you have any
> idea what these are about? These don't look graphics related so it seems
> like they'd somehow be related to something in widget and what happens on
> windows when popups get OMTC.

The crash stack indicates the unsafe assertion happens in XPCOM shutdown in ~nsMemoryReporterManager() for graphics surface objects of type gfxASurface.

xul.dll!SurfaceMemoryReporter::Release() [gfxASurface.cpp:70305d34138f : 503 + 0x23]

The assertion is associated with nsAutoOwningThread::AssertCurrentThreadOwnsMe so some sort of thread ownership of memory issue here.

Looks like it should be pretty easy to reproduce.
Flags: needinfo?(jmathies) → needinfo?(bas)

Updated

a month ago
Flags: needinfo?(mconley)

Comment 20

29 days ago
Hi Bas,

Do you have an estimate on when this bug will be resolved?  This is the primary remaining blocker for landing WE code for speed performance fixes.

Comment 21

29 days ago
> Do you have an estimate on when this bug will be resolved?  This is the
> primary remaining blocker for landing WE code for speed performance fixes.

According to the comments here, there's work on various platforms to do:

OSX: parts of bug 1291457 + "more work" for shadows - comment 15
Win: Gfx changes by Bas plus dealing with fallout from those changes - comment 18, comment 19
GTK: unknown amount of work.

None of this work is currently owned. Is this bug an absolute blocker? if so, for what and when? Right now this bug isn't on anyone's radar so it will not get fixed unless it gets prioritized within teams who have the engineering talent to work on it.
Flags: needinfo?(sescalante)
(Reporter)

Comment 22

29 days ago
This bug is an absolute blocker, yes. We can't enable remote WebExtensions until we can display remote content in popups.

At the moment, though, the only platform where this is a P1 is Windows. We can enable OOP extensions on a per-platform basis, so as long as we can support Windows in the short term, we can defer work on other platforms until after 57.
Flags: needinfo?(sescalante)

Comment 23

29 days ago
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #22)
> This bug is an absolute blocker, yes. We can't enable remote WebExtensions
> until we can display remote content in popups.
> 
> At the moment, though, the only platform where this is a P1 is Windows. We
> can enable OOP extensions on a per-platform basis, so as long as we can
> support Windows in the short term, we can defer work on other platforms
> until after 57.

Deferring thanks to the various work arounds in bug 1365660?

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #22)
> This bug is an absolute blocker, yes. We can't enable remote WebExtensions
> until we can display remote content in popups.
> 
> At the moment, though, the only platform where this is a P1 is Windows. We
> can enable OOP extensions on a per-platform basis, so as long as we can
> support Windows in the short term, we can defer work on other platforms
> until after 57.

Ok sounds good. Lets make this bug about the change on Windows only and file follow ups on OSX and linux.
(Reporter)

Comment 24

28 days ago
(In reply to Jim Mathies [:jimm] from comment #23)
> Deferring thanks to the various work arounds in bug 1365660?

No, those will only be useful for early testers and QA until the UI issues are fixed. Except perhaps on Linux, where things seem to work surprisingly well as long as we disable transparency.

But we can defer enabling OOP on other platforms until after 57, since those platforms aren't part of the Quantum release criteria.
OS: Unspecified → Windows
Summary: Support remote layer trees in popups → Support remote layer trees in popups on Windows
(Reporter)

Comment 25

27 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c42dea515f79932ef9756ebd8ad4044f46ff3850

It looks like marking the surface memory reporter's refcnt as threadsafe is enough to fix the memory management issues:

https://hg.mozilla.org/try/rev/54f3ac2b9bf0d50aede35c954e7c3c44ca4b78e8#l1.12

I think this a legitimate change, but I'm not sure. Nick, do you know?


With that fixed, though, there are some other issued. One with the APZ child apparently sending a message with the wrong layer ID:

https://treeherder.mozilla.org/logviewer.html#?job_id=102458935&repo=try&lineNumber=3014

And one where we apparently try to draw to a target that's already being drawn to:

https://treeherder.mozilla.org/logviewer.html#?job_id=102457880&repo=try&lineNumber=15085


I suspect that those may have to do with something getting confused between the main window's widget and the popup's.
Flags: needinfo?(n.nethercote)
(Reporter)

Comment 26

27 days ago
Ah, and, I think that at least some of those issues are from recreating the menupopup's widget when we change its "remote" attribute. It didn't cause any problems when we were only switching from non-composited to composited, but there seem to be some synchronization issues when we switch from one composited widget to another:

https://treeherder.mozilla.org/logviewer.html#?job_id=102457774&repo=try&lineNumber=9541
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #25)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c42dea515f79932ef9756ebd8ad4044f46ff3850
> 
> It looks like marking the surface memory reporter's refcnt as threadsafe is
> enough to fix the memory management issues:
> 
> https://hg.mozilla.org/try/rev/54f3ac2b9bf0d50aede35c954e7c3c44ca4b78e8#l1.12
> 
> I think this a legitimate change, but I'm not sure. Nick, do you know?

It certainly can't hurt. If that change alone is fixing the assertion then I say go for it.
Flags: needinfo?(n.nethercote)
Comment hidden (mozreview-request)

Comment 29

24 days ago
mozreview-review
Comment on attachment 8872476 [details]
Bug 1356317: Part 1 - Mark SurfaceMemoryReporter refcounting as threadsafe.

https://reviewboard.mozilla.org/r/144010/#review147722

r=me as is, but it would be nice to have a brief comment above the NS_DECL_THREADSAFE_ISUPPORTS explaining why thread safety is required.
Attachment #8872476 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 30

24 days ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f5cefeae50760e32505de29136c8cee65eba43
Bug 1356317: Part 1 - Mark SurfaceMemoryReporter refcounting as threadsafe. r=njn
(Reporter)

Updated

24 days ago
Keywords: leave-open

Comment 31

24 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03f5cefeae50

Comment 32

22 days ago
after talking with Milan and Bas, unassigning this bug from Bas.  Bas is the right person to validate fixes because Windows GFX is the impacted area, but the changes need to happen in the widget code.

(on osx mstange could be the right person to validate and on linux it's karlt)
Assignee: bas → nobody

Updated

22 days ago
Flags: needinfo?(bas)

Updated

22 days ago
No longer depends on: 1365660
Blocks: 1369559

Updated

17 days ago
Blocks: 1369488
Assigning while Stephen assesses the issue.
Assignee: nobody → spohl.mozilla.bugs

Updated

7 days ago
Keywords: leave-open
(Assignee)

Comment 34

5 days ago
I kicked off a new try run to see where we stand:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b34297e8e0fd81c3e01757af47e547884c7a5af&selectedJob=107963440

We're dealing with bc5 and bc6 failures. Both of these are due to the fact that we fail this assertion:
Assertion failure: sIndirectLayerTrees[child].mParent->mOptions == mOptions, at z:/build/build/src/gfx/layers/ipc/CompositorBridgeParent.cpp:1584

This is because we only set popups to use APZ if they contain remote content and this assertion fails because we have APZ enabled for one compositor, but not the other. By reverting the change from attachment 8868663 [details] in bug 1365660 and always enabling APZ for popups, these try failures go away. Instead, we're now running into new assertion failures in devtools tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00ca03c4612cb1742fd143fba50ecbee319b50b2&selectedJob=107969875

These failures seem to be due to the fact that we're trying to set a displayport base in APZCCallbackHelper::InitializeRootDisplayport when we're not actually dealing with root content or a toplevel document. This causes us to fail the assertion that was added in attachment 8845767 [details] [diff] [review] from bug 1346109. By removing the assertion and only setting the displayport base when we're actually dealing with root content or toplevel documents, these failures go away and we have a green try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=feee9d5fc05c28c768b9e06ddf54f8bc1c3751cd

Unfortunately, I can't tell with certainty that enabling APZ for all popups, and if only setting a displayport base in APZCCallbackHelper::InitializeRootDisplayport when we're actually dealing with root content or toplevel documents, is acceptable, or if this indicates a deeper issue that we should fix instead. I'm going to post both patches and request some feedback.
(Assignee)

Comment 35

5 days ago
Created attachment 8878891 [details] [diff] [review]
Enable APZ for all popups
Attachment #8878891 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 36

5 days ago
Created attachment 8878892 [details] [diff] [review]
Only set a displayport base in APZCCallbackHelper::InitializeRootDisplayport when we're dealing with root content or toplevel documents
Attachment #8878892 - Flags: feedback?(kmaglione+bmo)

Updated

3 days ago
Duplicate of this bug: 1372917
(Assignee)

Comment 38

3 days ago
Comment on attachment 8878891 [details] [diff] [review]
Enable APZ for all popups

Redirecting. Kats, is this acceptable, or do we instead need to support adopting children from one compositor to another if they have different APZ options? I'm not sure how hard this would be.
Attachment #8878891 - Flags: feedback?(kmaglione+bmo) → feedback?(bugmail)
(Assignee)

Comment 39

3 days ago
Comment on attachment 8878892 [details] [diff] [review]
Only set a displayport base in APZCCallbackHelper::InitializeRootDisplayport when we're dealing with root content or toplevel documents

Redirecting. Kats, is this acceptable, or do we need to figure out what we're dealing with here (presumably not root content or toplevel documents) and set an appropriate displayport base instead? I'm not sure how hard this would be. Also see comment 34. Thanks!
Attachment #8878892 - Flags: feedback?(kmaglione+bmo) → feedback?(bugmail)
Thanks for the detailed comments explaining the current state of things!

(In reply to Stephen A Pohl [:spohl] from comment #34)
> We're dealing with bc5 and bc6 failures. Both of these are due to the fact
> that we fail this assertion:
> Assertion failure: sIndirectLayerTrees[child].mParent->mOptions == mOptions,
> at z:/build/build/src/gfx/layers/ipc/CompositorBridgeParent.cpp:1584
> 
> This is because we only set popups to use APZ if they contain remote content
> and this assertion fails because we have APZ enabled for one compositor, but
> not the other.

It's not clear to me which content is getting adopted from one compositor to the other. Can you elaborate on this a little bit?
Comment on attachment 8878891 [details] [diff] [review]
Enable APZ for all popups

Review of attachment 8878891 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Stephen A Pohl [:spohl] from comment #38)
> Kats, is this acceptable, or do we instead need to support
> adopting children from one compositor to another if they have different APZ
> options? I'm not sure how hard this would be.

Adopting children from one compositor to another with different APZ options shouldn't be too hard, but adopting from one compositor to another with arbitrary differences in their CompositorOptions will be very hard. So I would rather just turn on APZ in the popups like you're doing in your patch than try to handle this adoption case in the compositor. The main concern with turning on APZ in the popups is that performance might suffer because we now have more overhead in painting stuff in the popup. If the performance penalty is acceptable to you as a tradeoff for getting this working, then it's fine by me.
Attachment #8878891 - Flags: feedback?(bugmail) → feedback+
Comment on attachment 8878892 [details] [diff] [review]
Only set a displayport base in APZCCallbackHelper::InitializeRootDisplayport when we're dealing with root content or toplevel documents

Review of attachment 8878892 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting this to :tnikkel. It could be that only setting the displayport base in these conditions will invalidate assumptions elsewhere in the code, I'm not really sure.
Attachment #8878892 - Flags: feedback?(bugmail) → feedback?(tnikkel)
Comment on attachment 8878892 [details] [diff] [review]
Only set a displayport base in APZCCallbackHelper::InitializeRootDisplayport when we're dealing with root content or toplevel documents

(In reply to Stephen A Pohl [:spohl] from comment #34)
> These failures seem to be due to the fact that we're trying to set a
> displayport base in APZCCallbackHelper::InitializeRootDisplayport when we're
> not actually dealing with root content or a toplevel document. This causes
> us to fail the assertion that was added in attachment 8845767 [details] [diff] [review]
> [diff] [review] from bug 1346109. By removing the assertion and only setting
> the displayport base when we're actually dealing with root content or
> toplevel documents, these failures go away and we have a green try run:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=feee9d5fc05c28c768b9e06ddf54f8bc1c3751cd
> 
> Unfortunately, I can't tell with certainty that enabling APZ for all popups,
> and if only setting a displayport base in
> APZCCallbackHelper::InitializeRootDisplayport when we're actually dealing
> with root content or toplevel documents, is acceptable, or if this indicates
> a deeper issue that we should fix instead. I'm going to post both patches
> and request some feedback.

From the tryrun it looks like APZCCallbackHelper::InitializeRootDisplayport is being called from ChromeProcessController::InitializeRoot. Do we expect a ChromeProcessController to be created for a widget whose associated document isn't a toplevel document in it's process? That feels unexpected to me.
Flags: needinfo?(bugmail)
(Reporter)

Comment 44

3 days ago
Comment on attachment 8878891 [details] [diff] [review]
Enable APZ for all popups

Review of attachment 8878891 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit worried about this, since we wound up seeing checkerboarding issues in autocomplete popups that had no remote content the first time I landed this.

Would it be enough to add a separate attribute ("enableapz" maybe) to popups that might ever create remote browsers, and never change UseAPZ for a popup after it's created?
(In reply to Timothy Nikkel (:tnikkel) from comment #43)
> From the tryrun it looks like APZCCallbackHelper::InitializeRootDisplayport
> is being called from ChromeProcessController::InitializeRoot. Do we expect a
> ChromeProcessController to be created for a widget whose associated document
> isn't a toplevel document in it's process? That feels unexpected to me.

It might be. Kris or Stephen, can you please expand on the stuff in comment 40? It's not really clear to me where the content is living and why it's getting transplanted and understanding that flow better would probably help answer this question.
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(bugmail)
(Reporter)

Comment 46

2 days ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #45)
> It might be. Kris or Stephen, can you please expand on the stuff in comment
> 40? It's not really clear to me where the content is living and why it's
> getting transplanted and understanding that flow better would probably help
> answer this question.

At the moment, we currently only enable compositing for popups when they have the "remote" attribute, and when that attribute changes (which happens when we open an extension sub-view in an existing panel), we create a new widget with the new compositor settings. I assume that's what we're talking about here, but I haven't looked at it in-depth.

After these changes, we always use compositing on Windows, though, and we wind up creating a new widget where both the old and new widget use compositors. I think we should actually be able to skip this step when the existing widget already has compositing and APZ enabled, though, which may solve the problem on its own.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #46)
> At the moment, we currently only enable compositing for popups when they
> have the "remote" attribute, and when that attribute changes (which happens
> when we open an extension sub-view in an existing panel), we create a new
> widget with the new compositor settings. I assume that's what we're talking
> about here, but I haven't looked at it in-depth.

This seems plausible... maybe. Continuing along this line of reasoning, what might be happening is that when we create this new compositing widget, we also create a new layer manager go with it. The extension sub-view would be hosted inside the remote rendering machinery (RenderFrameParent/RenderFrameChild) and perhaps as part of setting that up, we're running through the code at [1]. That is currently the only place that sends the "AdoptChild" IPC message to the compositor, which we were seeing in comment 38. However, if this hypothesis is correct, then the "mLayerManager" at [1] would be a layer manager of a type that goes with the old non-compositing window, (i.e. probably a BasicLayerManager) as opposed to "lm" which would be a ClientLayerManager. We should be able to check for that and skip the AdoptChild call in that case. That would probably get things working again. The AdoptChild message is AFAIK only needed when dragging a tab from one window to another, and in that case both "lm" and "mLayerManager" are ClientLayerManager instances.

> After these changes,

Which changes?

> we always use compositing on Windows, though, and we
> wind up creating a new widget where both the old and new widget use
> compositors. I think we should actually be able to skip this step when the
> existing widget already has compositing and APZ enabled, though, which may
> solve the problem on its own.

If so, great. Would OS X and Linux still have this problem though? And do we care about those platforms for this purpose?

[1] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/layout/ipc/RenderFrameParent.cpp#227
(Reporter)

Comment 48

7 hours ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #47)
> (In reply to Kris Maglione [:kmag] from comment #46)
> > At the moment, we currently only enable compositing for popups when they
> > have the "remote" attribute, and when that attribute changes (which happens
> > when we open an extension sub-view in an existing panel), we create a new
> > widget with the new compositor settings. I assume that's what we're talking
> > about here, but I haven't looked at it in-depth.
>
> This seems plausible... maybe. Continuing along this line of reasoning, what
> might be happening is that when we create this new compositing widget, we
> also create a new layer manager go with it. The extension sub-view would be
> hosted inside the remote rendering machinery
> (RenderFrameParent/RenderFrameChild) and perhaps as part of setting that up,
> we're running through the code at [1].

I'm not sure that's possible. We currently always set the remote=true
attribute before inserting the browser, so the layer manager should have
already changed by then. However:

> ... The AdoptChild message is AFAIK only needed when dragging a tab from one
> window to another, and in that case both "lm" and "mLayerManager" are
> ClientLayerManager instances.

When loading popups that use the CustomizeableUI/PanelUI framework, we
currently create a hidden <browser> in a hidden <panel> to begin loading its
content before the panel appears. We then create a new <browser> in the final
popup and swap docshells. It looks we don't set the "remote" attribute on the
original, hidden panel, so it seems fairly likely that the docshell swap is in
fact the problem.

> > After these changes,
>
> Which changes?

Sorry, after the changes discussed above (comment 18), which would enable
compositing on all popups on Windows.

> > we always use compositing on Windows, though, and we
> > wind up creating a new widget where both the old and new widget use
> > compositors. I think we should actually be able to skip this step when the
> > existing widget already has compositing and APZ enabled, though, which may
> > solve the problem on its own.
>
> If so, great. Would OS X and Linux still have this problem though? And do we
> care about those platforms for this purpose?

We're not initially targeting OS-X and Linux for OOP extensions, so it's not a
concern at the moment. After 57, we'll need to add support, but in the mean
time, it's preffed off on those platforms.
You need to log in before you can comment on or make changes to this bug.