Closed Bug 1353060 Opened 8 years ago Closed 7 years ago

Remote <browser>s are not visible as children of XUL <popup>s

Categories

(Core :: Graphics: Layers, defect, P1)

Product:
Core
Component:
Graphics: Layers
Version:
49 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mixedpuppy, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [triaged])

Attachments

(3 files)

Bug 1353060: Allow APZ in popup window widgets.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1353060: Use the correct layer manager for frameloaders in <popup>s.
59 bytes, text/x-review-board-request
kats
: review+
mattwoodrow
: review+
Details
Bug 1353060: Disable remote frameloaders in small popup widgets.
59 bytes, text/x-review-board-request
kats
: review+
Details
Looking into a context menu failure in bug 1299371, I see that browserAction is blank, though I know the page is loaded and js executed. This is a recent pull of m-c (from two days ago).
See Also: → 1353073
See Also: → 1299371
really busted, so putting it on another radar list.
webextensions: --- → +
Assignee: nobody → kmaglione+bmo
Whiteboard: [triaged]
Bill, Jim, do you have any idea where I should start looking here? I'm having trouble making progress. On Linux, the first problem is apparently that we create transparent popup panels with a BasicLayerManager, which causes RenderFrameParent::BuildLayer to fail. I still don't have a fix for that, but even after working around it, this still fails. As far as I can tell, the remote docshell is being initialized, shown, and layed out correctly. And from the layer logs[1], it looks like everything is being layed out and clipped properly, and rendered (the remote browser is the 800x600 layer with class:webextension-popup-browser, under panel-arrowcontent), but nothing is showing up. [1]: https://gist.github.com/8ce33f573709089868986d3f2311ec8d
webextensions: + → ---
Component: WebExtensions: Frontend → Graphics: Layers
Flags: needinfo?(wmccloskey)
Flags: needinfo?(jmathies)
Product: Toolkit → Core
Summary: popup panels empty with webextensions.remote=true → Remote <browser>s are not visible as children of XUL <popup>s
Maybe the remote layer tree isn't being attached properly? This normally happens via AsyncCompositionManager::ResolveRefLayers. The parent process will have a RefLayer that represents the remote <browser>. It has an ID that represents the ID of the remote layer tree that will be "attached" there when we composite. At that time, we look that layer tree ID up in a global map and add it as a child to the RefLayer. Maybe the IDs are screwed up or something like that? Matt Woodrow probably has some better ideas here. This might be a more basic architectural problem related to popups.
Flags: needinfo?(wmccloskey) → needinfo?(matt.woodrow)
(In reply to Bill McCloskey (:billm) from comment #3) > Maybe the remote layer tree isn't being attached properly? This normally > happens via AsyncCompositionManager::ResolveRefLayers. The parent process > will have a RefLayer that represents the remote <browser>. It has an ID that > represents the ID of the remote layer tree that will be "attached" there > when we composite. At that time, we look that layer tree ID up in a global > map and add it as a child to the RefLayer. Maybe the IDs are screwed up or > something like that? Hm. I think you may be right. This was one of the first places I looked, which led me to the BasicLayerManager issue on Linux. When I dealt with that, the RefLayer for the browser at least showed up in the iterator, but the pointer address of the referent doesn't show up anywhere in the painting logs for the panel version, but does for other remote browsers.
OK, that was the clue I needed. The RefLayer and the referent wind up with different layer managers here, so ConnectReferentLayer silently fails.
See Also: → 1356317
Blocks: webext-oop
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jmathies)
I tried to change the reviewer to :kats but I'm not sure if MozReview accepted it. If not, could you r? him instead, Kris? I think he's better qualified to look at this change.
Attachment #8858066 - Flags: review?(dvander) → review?(bugmail)
(In reply to David Anderson [:dvander] from comment #8) > I tried to change the reviewer to :kats but I'm not sure if MozReview > accepted it. If not, could you r? him instead, Kris? I think he's better > qualified to look at this change. Done. Thanks. I've never figured out how changing reviewers for other people's patches is supposed to work, but it's never worked for me.
Comment on attachment 8858066 [details] Bug 1353060: Allow APZ in popup window widgets. https://reviewboard.mozilla.org/r/130052/#review133156 It's not clear to me why this change is needed. From the comments in the bug it sounds like the second patch here should be sufficient. Why is APZ necessary on popup windows? (Even better than replying on the bug would be to put the answer in the commit message).
Attachment #8858066 - Flags: review?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > It's not clear to me why this change is needed. From the comments in the bug > it sounds like the second patch here should be sufficient. Why is APZ > necessary on popup windows? (Even better than replying on the bug would be > to put the answer in the commit message). I'll update the commit message, but essentially: Before this change, we only ever had remote browsers in toplevel and child window widgets. After this change, we also have them in popup windows. I suppose we could technically have APZ disabled for those browsers and enabled everywhere else, but that would mean they would have uniquely bad performance under load compared to the rest of the browser, which doesn't seem desirable.
Do you know what kind of popup windows can now have remote browsers (in terms of the nsPopupType enum)? Because if it's just ePopupTypePanel (which is what I would expect) I would like that to be a part of the condition as well, ideally by reusing the IsSmallPopup function.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > Do you know what kind of popup windows can now have remote browsers (in > terms of the nsPopupType enum)? Because if it's just ePopupTypePanel (which > is what I would expect) I would like that to be a part of the condition as > well, ideally by reusing the IsSmallPopup function. I don't think there's anything stopping them from being loaded into any kind of popup, but ePopupTypePanel is the only type they should actually be loaded in into practice. I don't mind only enabling APZ for ePopupTypePanel, though. We might even be able to bail out of creating a TabParent (or even initializing the frameloader at all) for other kinds, but it would take some additional work.
(In reply to Kris Maglione [:kmag] from comment #13) > I don't think there's anything stopping them from being loaded into any kind > of popup, but ePopupTypePanel is the only type they should actually be > loaded in into practice. I don't mind only enabling APZ for ePopupTypePanel, > though. Thanks, please do that. There's some overhead to enabling APZ, and I'd like to avoid enabling it on things like tooltips and other popup panels that don't need it.
s/other popup panels/other popup widgets/
Comment on attachment 8858066 [details] Bug 1353060: Allow APZ in popup window widgets. https://reviewboard.mozilla.org/r/130052/#review133492
Attachment #8858066 - Flags: review?(bugmail) → review+
Comment on attachment 8858067 [details] Bug 1353060: Use the correct layer manager for frameloaders in <popup>s. https://reviewboard.mozilla.org/r/130054/#review133494 Some comments below but overall the patch looks sane. I'm flagging mattwoodrow because I think he should take a look as well, I'm not 100% comfortable with this code. Also please make sure to do a try push with at least Linux and Android, changes to this code tends to easily break things. ::: dom/base/nsContentUtils.h:2021 (Diff revision 2) > * widget for documents in display:none frames that have no presentation. > */ > static nsIWidget* WidgetForDocument(const nsIDocument* aDoc); > > /** > + * Returns the widget for this element if there is one. I think this comment should probably explain how it differs from doing |WidgetForDocument(aContent->GetComposedDoc())| for example. ::: layout/ipc/RenderFrameParent.h:90 (Diff revision 2) > > void TakeFocusForClickFromTap(); > > void EnsureLayersConnected(); > > + LayerManager* GetLayerManager(); I would really prefer if this function were named something different so that it's more obvious that it has side-effects. PuppetWidget::GetLayerManager is in the same boat - it has the side-effect of creating a layer manager if one didn't already exist, and that has bitten me a number of times. ::: layout/ipc/RenderFrameParent.h:112 (Diff revision 2) > // compositor and so this flag is false. > bool mLayersConnected; > > RefPtr<nsFrameLoader> mFrameLoader; > RefPtr<ContainerLayer> mContainer; > + RefPtr<LayerManager> mLayerManager; It's probably a good idea to set mLayerManager = nullptr in the Destroy() and/or ActorDestroy() functions. As it stands I'm not sure if this will create a reference cycle or something. The references between these classes are not easy to track.
Attachment #8858067 - Flags: review?(bugmail) → review+
Comment on attachment 8858922 [details] Bug 1353060: Disable remote frameloaders in small popup widgets. https://reviewboard.mozilla.org/r/130918/#review133498
Attachment #8858922 - Flags: review?(bugmail) → review+
Comment on attachment 8858067 [details] Bug 1353060: Use the correct layer manager for frameloaders in <popup>s. https://reviewboard.mozilla.org/r/130054/#review133494 > I would really prefer if this function were named something different so that it's more obvious that it has side-effects. PuppetWidget::GetLayerManager is in the same boat - it has the side-effect of creating a layer manager if one didn't already exist, and that has bitten me a number of times. Yeah, I was a bit worried about that with the non-obvious side-effect in OwnerDocChanged(). How about AttachLayerManager()?
AttachLayerManager() sounds good, thanks!
Comment on attachment 8858067 [details] Bug 1353060: Use the correct layer manager for frameloaders in <popup>s. https://reviewboard.mozilla.org/r/130054/#review133544 I'm not sure I understand the various relationships well enough here to give a good review. The main question I have is, in what situations does WidgetForContent give a different result to WidgetForDocument? Are the other callers of WidgetForDocument correct, or should they be switched to WidgetForContent? Both methods attempt to find a display root, which I'd expect to be the root frame/view for the popup. This is the one that should be creating a ClientLayerManager, and be able to support OOP content. What are we finding instead as our display root? Something to do with the frameloader itself instead of the popup that it's contained within? Is it correct to have that treated as a display root? And why is this specific to frameloaders within a popup? Sorry, lot of questions and very few answers.
(In reply to Matt Woodrow (:mattwoodrow) from comment #24) > The main question I have is, in what situations does WidgetForContent give a > different result to WidgetForDocument? The only case I know of where they should be different (and I don't think there are any others) is XUL popups. Those get their own nsWindow widgets, and their children are supposed to use those rather than the document's main widget. > Are the other callers of WidgetForDocument correct, or should they be > switched to WidgetForContent? That's a good question, which I've been wondering about. This should only ever be an issue for XUL elements, except for things in documents which happen to be loaded into XUL popups. Once we've switched to remote browsers, that case should no longer matter. But even so, I suspect that the answer is yes. There are probably things that currently work correctly in popups only by accident. > Both methods attempt to find a display root, which I'd expect to be the root > frame/view for the popup. This is the one that should be creating a > ClientLayerManager, and be able to support OOP content. Right, but the problem is that WidgetForDocument finds the main widget for the document, walking to parent documents if necessary. But it doesn't consider the case where it's a child of an element that has a different widget from the rest of the document. > What are we finding instead as our display root? The widget created for the <popup> element: http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/view/nsView.cpp#622 > Something to do with the frameloader itself instead of the popup that it's > contained within? No, the frameloader just happens to be a child of a different widget than the widget for the root document. > Is it correct to have that treated as a display root? Yes. > And why is this specific to frameloaders within a popup? Because other frameloaders belong to the main widget of the root document, so they find the correct widget (and therefore layer manager) with the current logic.
Ok, so we were previously finding the widget for the main window, and now (with this patch) we find the widget for the <popup>? That makes sense, and seems like the right thing to do! Do you know why WidgetForDocument skips over the popups widget? Is it because we just jump to the root view? Is it possible to fix that instead of adding a second function? I think most of the callers of WidgetForDocument want the behaviour of WidgetForContent, so I think it's worth converting them across (and removing WidgetForDocument), or fixing WidgetFromDocument.
CC'ing tn, since he might know if we can easily find the popup when traversing up the views.
(In reply to Matt Woodrow (:mattwoodrow) from comment #26) > Ok, so we were previously finding the widget for the main window, and now > (with this patch) we find the widget for the <popup>? Right. > Do you know why WidgetForDocument skips over the popups widget? Is it > because we just jump to the root view? Is it possible to fix that instead of > adding a second function? I think we could make it work when it's called for a sub-document of the popup. That would at least handle the first workaround I tried, which was embedding the remote browser in a non-remote browser. But for an element in the same document as the <popup> element, there's no document we could pass it that would return the right widget.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20) > Also please make sure to do a try push with at least Linux and Android, > changes to this code tends to easily break things. Some runs eventually turned up a race where, which we create and quickly remove a remote browser in debug builds, we get: Assertion failure: mDPI > 0 (Must not ask for DPI before OwnerElement is received!), at /home/worker/workspace/build/src/dom/ipc/TabParent.cpp:2305 Which happens because the previous code still returned the document's widget after the browser/frame element had been removed, but the new code doesn't. So I suppose the two options are a) return the document widget when no widget can be found for the view, or b) ignore those assertions when they happen after the frame element has been removed.
(In reply to Matt Woodrow (:mattwoodrow) from comment #27) > CC'ing tn, since he might know if we can easily find the popup when > traversing up the views. You could GetClosestView and then GetDisplayRootFor on the view, but I think GetDisplayRoot on the frame will be just as fast in this case, and faster in the general case because we can skip right the fast case of GetDisplayRoot because no popup bits are set. I'd agree that it seems most (all?) callers want WidgetForContent. I'd add a clarifying comment that when you call WidgetForContent on mFrameElement you are not getting the widget that the frame element "owns" for the subdocument.
(In reply to Kris Maglione [:kmag] from comment #28) > > Do you know why WidgetForDocument skips over the popups widget? Is it > > because we just jump to the root view? Is it possible to fix that instead of > > adding a second function? > > I think we could make it work when it's called for a sub-document of the > popup. That would at least handle the first workaround I tried, which was > embedding the remote browser in a non-remote browser. But for an element in > the same document as the <popup> element, there's no document we could pass > it that would return the right widget. Right, good point. In that case I think we should aim to use the 'ForContent' version everywhere.
Blocks: 1357486
Marking as quantum flow p1 because this blocks our ability to run webextensions in a seperate process. Currently waiting on review.
Whiteboard: [triaged] → [triaged][qf:p1]
Gentle review ping?
Flags: needinfo?(matt.woodrow)
Apologies, I was waiting on a response to comment 31, but that wasn't made overly clear. I'd like to see us switch to the ForContent version everywhere, but that's probably ok to do in a followup.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8858067 [details] Bug 1353060: Use the correct layer manager for frameloaders in <popup>s. https://reviewboard.mozilla.org/r/130054/#review139014
Attachment #8858067 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #38) > Apologies, I was waiting on a response to comment 31, but that wasn't made > overly clear. > > I'd like to see us switch to the ForContent version everywhere, but that's > probably ok to do in a followup. Sorry, I didn't know you were waiting on a response to that. I updated the API docs in response to that discussion, but actually updating the other consumers seems like it should happen in a separate bug.
Blocks: 1342708
Is this bug about empty / not appearing browserAction popups with OOP WebExtensions enabled? I am asking because the patches landed two days ago and it's still not fixed in yesterday's Nightly (tested on macOS 10.12 with Snooze Tabs and Container Tabs, both Test Pilot experiments). Or is this another bug?
Depends on: 1362621
I intend to back out this bug for one nightly, to see if it fixes the checkerboarding regression in bug 1362987. I'll do it later today unless somebody can come up with a good reason for me not to do this.
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/236c11578dff Use the correct layer manager for frameloaders in <popup>s. r=kats,mattwoodrow https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea7676f8735 Disable remote frameloaders in small popup widgets. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/63a1460c3804 Allow APZ in popup window widgets. r=kats
Debian Testing 64 bit, extensions.webextensions.remote;true , own build with "ac_add_options --enable-debug", my build id is 20170512210621: uBlock Origin's "bubble" is still empty (white) like before, but the mouse shows a "finger" where I could normally click something. If I click on the white top of the white "bubble", uBlock Origin's settings page gets opened.
(In reply to Darkspirit from comment #48) Not fixed with today's 20170513100302 on Linux. Fixed on Windows 10.
And also not fixed on macOS (as already said in comment 43).
It's partially fixed. The complete fix depends on bug 1356317, but in the mean time we're probably going to force-enable compositing for panels containing remote content, which will have the unfortunate side-effect of making their transparent areas opaque.
Depends on: 1365448
Depends on: 1362378
Depends on: 1366103
Depends on: 1365984
Depends on: 1374025

Is this related to the non-resolved bugs 1627139, 1629734, 1592310?

They all exhibit the same issue: blank/empty extension menus.

Flags: needinfo?(kmaglione+bmo)

(In reply to diamondavocado22 from comment #52)

Is this related to the non-resolved bugs 1627139, 1629734, 1592310?

No.

Flags: needinfo?(kmaglione+bmo)
Performance Impact: --- → P1
Whiteboard: [triaged][qf:p1] → [triaged]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: