1479181 - OOP Webextension panels on Linux: white background instead of transparency

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect)

Comment 1

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

STR:
1) Install an add-on, that has a toolbar button with a popup, when you click on it.
2) Enable WebRender (gfx.webrender.all).
3) Restart FF.
4) Click the toolbar button.
5) A white are is shown around the opened popup. Note it happens only for add-on button, not for Firefox ones (like hamburger menu or Library).

Running mozregression now to find the cause.

Comment 2

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Attachment: add-on popup with white area behind

Comment 3

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Attachment: Library button popup (no issue)

Comment 4

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

With mozregression I came to commits from bug 1357487 and bug 1464938, both from :kmag. Is that possible those are really related?

> 34:21.93 INFO: No more inbound revisions, bisection finished.
> 34:21.93 INFO: Last good revision: 1c52d8baf78986cb9497d48b8b88291686a681ca
> 34:21.93 INFO: First bad revision: c034e7ddbacdbe80ec8ce5b844e4299fcac9287c
> 34:21.93 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1c52d8baf78986cb9497d48b8b88291686a681ca&tochange=c034e7ddbacdbe80ec8ce5b844e4299fcac9287c

Comment 5

[Darkspirit]

This is the linux version of bug 1444595.

Comment 6

[Sotaro Ikeda [:sotaro]]

Implementation of nsBaseWidget::AllowWebRenderForThisWindow() seems to be related to the problem. The code is like the following. It allows webrender usage for "(WindowType() == eWindowType_popup && HasRemoteContent())" case. Then OOP webextensions seems to trigger the problem.

------------------------------------------------------------

bool
nsBaseWidget::AllowWebRenderForThisWindow()
{
  return WindowType() == eWindowType_toplevel ||
         WindowType() == eWindowType_child ||
         WindowType() == eWindowType_dialog ||
         (WindowType() == eWindowType_popup && HasRemoteContent());
}

Comment 7

[Sotaro Ikeda [:sotaro]]

It seems that rgba visual is not allocated with GLContextGLX::FindVisual() even when argument "useAlpha" is true.

Comment 8

[Sotaro Ikeda [:sotaro]]

It seems better to avoid to use webrender when widget uses AlphaVisual for now. glXChooseVisual() did not choose rgba visual even when GLX_ALPHA_SIZE was requested on my 2 Ubuntu PCs. And rgba visual by gdk_screen_get_rgba_visual did work with glx on my one Ubuntu PC.

Comment 9

[Sotaro Ikeda [:sotaro]]

It should be OK, since we already disabled webrender on Windows when a widget has transparency.
  https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#8316

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Disable WebRender when widget uses AlphaVisual

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Disable WebRender when widget uses AlphaVisual

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Disable WebRender when widget uses AlphaVisual

Comment 13

[Sotaro Ikeda [:sotaro]]

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

Comment 14

[Nicolas Silva [:nical]]

Comment on attachment 8996518 [details] [diff] [review]
patch - Disable WebRender when widget uses AlphaVisual

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

::: widget/gtk/nsWindow.cpp
@@ +3734,5 @@
>  
> +        // Disable WebRender when widget uses AlphaVisual.
> +        // glXChooseVisual would not choose rgba visual even when GLX_ALPHA_SIZE
> +        // is requested. And rgba visual by gdk_screen_get_rgba_visual would
> +        // not work with glx.

I find it really odd that we aren't able to get an alpha channel. From reading around it looks like there's something nvidia related, and also I wonder if we are actually getting a visual with alpha but not selecting the right framebuffer config, since we don't seem to request ay alpha channel when calling fChooseFBConfig.

I'm fine with this workaround but could you add to this comment that we can probably make this work with alpha in some cases if we spend some more time investigating?

Comment 15

[Martin Stránský [:stransky] (ni? me)]

(In reply to Nicolas Silva [:nical] from comment #14)
> Comment on attachment 8996518 [details] [diff] [review]
> patch - Disable WebRender when widget uses AlphaVisual
> 
> Review of attachment 8996518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gtk/nsWindow.cpp
> @@ +3734,5 @@
> >  
> > +        // Disable WebRender when widget uses AlphaVisual.
> > +        // glXChooseVisual would not choose rgba visual even when GLX_ALPHA_SIZE
> > +        // is requested. And rgba visual by gdk_screen_get_rgba_visual would
> > +        // not work with glx.
> 
> I find it really odd that we aren't able to get an alpha channel. From
> reading around it looks like there's something nvidia related, and also I
> wonder if we are actually getting a visual with alpha but not selecting the
> right framebuffer config, since we don't seem to request ay alpha channel
> when calling fChooseFBConfig.
> 
> I'm fine with this workaround but could you add to this comment that we can
> probably make this work with alpha in some cases if we spend some more time
> investigating?

Nicolas, I already investigated that and it's at https://bugzilla.mozilla.org/show_bug.cgi?id=1478454#c29

Would be great if you can comment there.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

(In reply to Martin Stránský [:stransky] from comment #15)
> > I'm fine with this workaround but could you add to this comment that we can
> > probably make this work with alpha in some cases if we spend some more time
> > investigating?

Also a simple fix here is to just use the visual from X drawable as it's selected/configured at nsWidnow::Create():

https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/widget/gtk/nsWindow.cpp#3739

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8996518 [details] [diff] [review]
patch - Disable WebRender when widget uses AlphaVisual

IMHO this patch does not address the core cause which is visual mismatch at AreCompatibleVisuals().

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Attachment: copy visual patch

This simple patch just copies WebRender compatible visual from Xwindow. The correct visual was selected at nsWidow::Create() so there's no need to configure it again.

Comment 19

[Darkspirit]

(In reply to Martin Stránský [:stransky] from comment #18)
> Created attachment 8996656 [details] [diff] [review]
> copy visual patch
> 
> This simple patch just copies WebRender compatible visual from Xwindow. The correct visual was selected at nsWidow::Create() so there's no need to configure it again.

Proprietary Nvidia driver:
My local build indicates that you have just fixed bug 1478454, but there's a black background around the panel (OpenGL and Basic have transparency). Still better than Basic.
And I noticed this in my terminal:
> (firefox:14060): Gtk-WARNING **: 14:21:27.899: Theme parsing error: <data>:1:34: Expected ')' in color definition

Comment 20

[Darkspirit]

Nouveau, latest m-c:
mozregression --launch af6a7edf0069 --pref gfx.webrender.all:true gfx.webrender.debug.profiler:true -a https://addons.mozilla.org/en-US/firefox/addon/ublock-origin/

uBlock's panel has a black background instead of transparency around it (and it uses WR because there's a WR profiler in it). 
My local build from comment 19 looks the same, so comment 18 as fix for bug 1478454 wouldn't break anything.

Comment 21

[Nicolas Silva [:nical]]

Comment on attachment 8996518 [details] [diff] [review]
patch - Disable WebRender when widget uses AlphaVisual

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

Thanks a lot for shimming in Martin. You definitely have a better understanding of this stuff than me, I'll make sure add you to the loop for other widget related reviews that come my way.
What you are saying makes a lot of sense. I tried your patch and it improves the situation a lot for the test case I have handy. I still see white where the widget should be transparent but I suspect that will go away with another of sotaro's patches that make us use the right clear color.

Sotaro, since we have something to try, let's explore this before we drop transparent widgets altogether.

Comment 22

[Sotaro Ikeda [:sotaro]]

I already noticed the problem of attachment 8996656 [details] [diff] [review] during investigating this bug and confirmed that it did not address the transparency problem on my 2 linux pcs.

It could address the caused failure of creating GLXContext by visual mismatch. But it does not address the problem of allocating rgba visual.

Comment 23

[Sotaro Ikeda [:sotaro]]

I did not include the change of attachment 8996656 [details] [diff] [review] in attachment 8996518 [details] [diff] [review] since it is not related to rgba visual allocation problem and when attachment 8996518 [details] [diff] [review] was applied, I did not see the visual mismatch on my 2 linux pcs. Then visual mismatch could be handled as a different problem.

Comment 24

[Sotaro Ikeda [:sotaro]]

So during investigating this bug, I locally applied same change of attachment 8996656 [details] [diff] [review]. But it was not included to attachment 8996518 [details] [diff] [review] because of comment 23.

Comment 25

[Darkspirit]

Some people see this problem even without WebRender: bug 1479135 (OpenGL itself is fine for me.)

Comment 26

[Sotaro Ikeda [:sotaro]]

It seems that glXChooseVisual() seems not provide a visual that is requested by gecko. The following document has a description "glXChooseVisual is implemented as a client-side utility using only XGetVisualInfo and glXGetConfig."

https://www.khronos.org/registry/OpenGL-Refpages/gl2.1/xhtml/glXChooseVisual.xml

Then, it seems that we could address the problem by using XGetVisualInfo and glXGetConfig.

Comment 27

[Sotaro Ikeda [:sotaro]]

I locally confirmed that Comment 26 address the problem for me.

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Expose glxGetConfig() in GLXLibrary

Comment 29

[Sotaro Ikeda [:sotaro]]

Chromium also seems to use XGetVisualInfo() and glXGetConfig().

https://cs.chromium.org/chromium/src/ui/gl/gl_visual_picker_glx.cc?type=cs&q=XGetVisualInfo&sq=package:chromium&g=0&l=158

https://cs.chromium.org/chromium/src/ui/gl/gl_visual_picker_glx.cc?type=cs&q=GLX_USE_GL&g=0&l=44

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Remove glXChooseVisual() usage

Comment 31

[Sotaro Ikeda [:sotaro]]

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

Comment 32

[Martin Stránský [:stransky] (ni? me)]

FYI: glxinfo will print all available visuals, you can easily check which visual is selected and if that selection is correct. But the same visual has to be used for drawable (xid) of nsWindow and for glxContext, we'll get BadMatch X error otherwise.

Comment 33

[Sotaro Ikeda [:sotaro]]

:stransky, thanks for the info!

In this bug, I work for fixing the problem of visual selection in GLContextGLX::FindVisual(). And intentionally, I did not work for fixing bad match in this bug.

attachment 8996656 [details] [diff] [review] that you created, addresses the bat mach. Can you create a new bug for fixing the bad match?

Comment 34

[Martin Stránský [:stransky] (ni? me)]

(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> :stransky, thanks for the info!
> 
> In this bug, I work for fixing the problem of visual selection in
> GLContextGLX::FindVisual(). And intentionally, I did not work for fixing bad
> match in this bug.

Sure. I did the visual comparsion and the only difference is the depth. 0x051 is selected by FindVisual and 0x0e6 is chosen by your patch. 0x0e6 works fine with transparent windows, co the 32-bit depth seems to be the trick here.

0x051 24 tc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
0x0e6 32 tc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None

So as your patch is functional I wonder if we can do something less intrusive and without hardcoding the rgb masks.

> attachment 8996656 [details] [diff] [review] that you created, addresses the
> bat mach. Can you create a new bug for fixing the bad match?

It's already filed, Bug 1478454 (don't be confused by NVIDA here).

Comment 35

[Sotaro Ikeda [:sotaro]]

It seems that we could remove the hardcoding by using default visual, like chromium does in the following.

XVisualInfo GLVisualPickerGLX::PickBestSystemVisual(
    const std::vector<XVisualInfo>& visuals) const {
  Visual* default_visual = DefaultVisual(display_, DefaultScreen(display_));
  auto it = std::find_if(visuals.begin(), visuals.end(),
                         [default_visual](const XVisualInfo& visual_info) {
                           return visual_info.visual == default_visual;
                         });
  DCHECK(it != visuals.end());

  const XVisualInfo& default_visual_info = *it;
  std::vector<XVisualInfo> filtered_visuals;
  std::copy_if(
      visuals.begin(), visuals.end(), std::back_inserter(filtered_visuals),
      [&default_visual_info](const XVisualInfo& visual_info) {
        const XVisualInfo& v1 = visual_info;
        const XVisualInfo& v2 = default_visual_info;
        return v1.c_class == v2.c_class && v1.depth == v2.depth &&
               v1.red_mask == v2.red_mask && v1.green_mask == v2.green_mask &&
               v1.blue_mask == v2.blue_mask &&
               v1.colormap_size == v2.colormap_size &&
               v1.bits_per_rgb == v2.bits_per_rgb;
      });
  return PickBestGlVisual(filtered_visuals, IsArgbVisual(default_visual_info));
}

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Remove glXChooseVisual() usage

Remove hardcoded mask values.

Comment 38

[Martin Stránský [:stransky] (ni? me)]

(In reply to Martin Stránský [:stransky] from comment #34)

> 0x051 24 tc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None
> 0x0e6 32 tc  0  32  0 r  y .   8  8  8  8 .  .  0 24  8  0  0  0  0  0 0 None

I think I understand what's going on here and why we have an alpha visual but the result is not transparent. The first selected visual (0x51) has 32-bit depth color buffer with RGBA but maps to only 24-bit depth X window surface - the X11 depth is only 24 so the alpha is discarded in X.

Comment 39

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8996971 [details] [diff] [review]
patch part 2 - Remove glXChooseVisual() usage

IMHO this should be reviewed by gl peer. But let me comment it a bit:

-    XVisualInfo* visuals = sGLXLibrary.fChooseVisual(display, screen, attribs);
-    if (!visuals) {
+    XVisualInfo visualTemplate;
+    visualTemplate.screen = screen;

I think more template values can be passed here (depth for instance).

+   // Get default visual
+
+   Visual* defaultVisual = DefaultVisual(display, screen);
+   auto it = std::find_if(visuals.begin(), visuals.end(),
+                         [defaultVisual](const XVisualInfo& visualInfo) {
+                           return visualInfo.visual == defaultVisual;
+                         });
+
+   MOZ_ASSERT(it != visuals.end());
+   if (it == visuals.end()) {
+        return false;
+   }
+   const XVisualInfo& defaultVisualInfo = *it;
+   const int depth = useAlpha ? 32 : 24;
+
+    // Get visuals that are compatible to default visual.
+
+    std::vector<XVisualInfo> compatibleVisuals;
+
+    std::copy_if(
+        visuals.begin(), visuals.end(), std::back_inserter(compatibleVisuals),
+        [&defaultVisualInfo, depth](const XVisualInfo& visual) {
+            return visual.depth == depth &&
+                   visual.c_class == defaultVisualInfo.c_class &&
+                   visual.red_mask == defaultVisualInfo.red_mask &&
+                   visual.green_mask == defaultVisualInfo.green_mask &&
+                   visual.blue_mask == defaultVisualInfo.blue_mask;
+        });

AFAIK there's no need to compare bit mask of DefaultVisual. We just need to make sure we have 32-bit X visual depth (visual.depth) for transparent surfaces and 8 bit R G B (A) components along the 24bit depth buffer.

-                mHasAlphaVisual = true;
+                mHasAlphaVisual = needsAlphaVisual;

That's really not relevant here as it controls shape masks for non-composting WM. If you don't mind I'd put that to an extra patch.

Comment 40

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8996656 [details] [diff] [review]
copy visual patch

No need to fix it here.

Comment 41

[Martin Stránský [:stransky] (ni? me)]

(In reply to Martin Stránský [:stransky] from comment #39)
> -                mHasAlphaVisual = true;
> +                mHasAlphaVisual = needsAlphaVisual;
> 
> That's really not relevant here as it controls shape masks for
> non-composting WM. If you don't mind I'd put that to an extra patch.

Filed as Bug 1480409

Comment 43

[Sotaro Ikeda [:sotaro]]

(In reply to Martin Stránský [:stransky] from comment #39)
> Comment on attachment 8996971 [details] [diff] [review]
> patch part 2 - Remove glXChooseVisual() usage
> 
> IMHO this should be reviewed by gl peer. But let me comment it a bit:
> 
> -    XVisualInfo* visuals = sGLXLibrary.fChooseVisual(display, screen,
> attribs);
> -    if (!visuals) {
> +    XVisualInfo visualTemplate;
> +    visualTemplate.screen = screen;
> 
> I think more template values can be passed here (depth for instance).

Passing more template values did not affect to the result. And chromium also pass only screen. Then it seems that there are no need to pass the more values.

> +    std::copy_if(
> +        visuals.begin(), visuals.end(),
> std::back_inserter(compatibleVisuals),
> +        [&defaultVisualInfo, depth](const XVisualInfo& visual) {
> +            return visual.depth == depth &&
> +                   visual.c_class == defaultVisualInfo.c_class &&
> +                   visual.red_mask == defaultVisualInfo.red_mask &&
> +                   visual.green_mask == defaultVisualInfo.green_mask &&
> +                   visual.blue_mask == defaultVisualInfo.blue_mask;
> +        });
> 
> AFAIK there's no need to compare bit mask of DefaultVisual. We just need to
> make sure we have 32-bit X visual depth (visual.depth) for transparent
> surfaces and 8 bit R G B (A) components along the 24bit depth buffer.

I will remove the mask check.

> 
> -                mHasAlphaVisual = true;
> +                mHasAlphaVisual = needsAlphaVisual;
> 
> That's really not relevant here as it controls shape masks for
> non-composting WM. If you don't mind I'd put that to an extra patch.

Yea, it is not related to the patch. I will remove it.

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Remove glXChooseVisual() usage

Apply the comments.

Comment 45

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Remove glXChooseVisual() usage

Comment 46

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8996929 [details] [diff] [review]
patch part 1 - Expose glxGetConfig() in GLXLibrary

:jgilbert, can you review the patches?

Comment 47

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8997209 [details] [diff] [review]
patch part 2 - Remove glXChooseVisual() usage

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

Let me know what you think of these recommendations.

::: gfx/gl/GLContextProviderGLX.cpp
@@ +908,5 @@
> +    std::vector<XVisualInfo> visuals;
> +    for (int i = 0; i < visualsLen; i++) {
> +      visuals.push_back(xVisuals[i]);
> +    }
> +    XFree(xVisuals);

For ranged-for syntax:
https://dxr.mozilla.org/mozilla-central/source/mfbt/Range.h
For memory management:
https://dxr.mozilla.org/mozilla-central/source/mfbt/ScopeExit.h

@@ +914,5 @@
> +
> +   // Get default visual
> +
> +   Visual* defaultVisual = DefaultVisual(display, screen);
> +   auto it = std::find_if(visuals.begin(), visuals.end(),

const auto

@@ +915,5 @@
> +   // Get default visual
> +
> +   Visual* defaultVisual = DefaultVisual(display, screen);
> +   auto it = std::find_if(visuals.begin(), visuals.end(),
> +                         [defaultVisual](const XVisualInfo& visualInfo) {

Just capture with [&] if the lambda is stack-scoped.

@@ +919,5 @@
> +                         [defaultVisual](const XVisualInfo& visualInfo) {
> +                           return visualInfo.visual == defaultVisual;
> +                         });
> +
> +   MOZ_ASSERT(it != visuals.end());

Just `MOZ_ASSERT(false)` inside `if (it == visuals.end())` instead of repeating the conditional here.

@@ +922,5 @@
> +
> +   MOZ_ASSERT(it != visuals.end());
> +   if (it == visuals.end()) {
> +        return false;
> +   }

Consider:
const Range visualInfos(xVisuals, xVisuals+visualsLen);
const auto defaultVisualInfo = [&]() -> const XVisualInfo* {
    for (const auto& cur : visualInfos) {
        if (cur.visual == defaultVisual)
            return &cur;
    }
    return nullptr;
}();
if (!defaultVisualInfo)
    return false;

@@ +923,5 @@
> +   MOZ_ASSERT(it != visuals.end());
> +   if (it == visuals.end()) {
> +        return false;
> +   }
> +   const XVisualInfo& defaultVisualInfo = *it;

I think this would be more clear as:
const auto defaultVisualInfo = [&]() -> const XVisualInfo* {
  for (const auto& visualInfo : visuals) {
    if (visualInfo.visual == defaultVisual)
      return visualInfo

@@ +924,5 @@
> +   if (it == visuals.end()) {
> +        return false;
> +   }
> +   const XVisualInfo& defaultVisualInfo = *it;
> +   const int depth = useAlpha ? 32 : 24;

s/depth/bpp/

@@ +935,5 @@
> +        visuals.begin(), visuals.end(), std::back_inserter(compatibleVisuals),
> +        [&defaultVisualInfo, depth](const XVisualInfo& visual) {
> +            return visual.depth == depth &&
> +                   visual.c_class == defaultVisualInfo.c_class;
> +        });

This is one extra line:
for (const auto& x : visuals) {
  if (x.depth == depth &&
      x.c_class == defaultVisualInfo.c_class)
  {
    compatibleVisuals.push_back(x);
  }
}

@@ +942,5 @@
> +
> +    const int resquestedAlphaSize = useAlpha ? 8 : 0;
> +    const int resquestedDepthSize = useWebRender ? 24 : 0;
> +
> +    for (auto& visual : compatibleVisuals) {

So you're gathering all the compatible visuals, and then looping over them to choose the one you want.
Merge these two loops, looping over /all/ visuals, skipping ones that aren't compatible, and then skipping ones that don't match our needs.

@@ +945,5 @@
> +
> +    for (auto& visual : compatibleVisuals) {
> +        int supportsGl, doubleBuffer, redSize, greenSize, blueSize, alphaSize, depthSize;
> +        if (!sGLXLibrary.fGetConfig(display, &visual, LOCAL_GLX_USE_GL, &supportsGl) &&
> +            supportsGl &&

const auto fnConfigMatches = [&](const int pname, const int expected) {
  int actual;
  if (!sGLXLibrary.fGetConfig(display, &visual, pname, &actual))
    return false;
  return actual == expected;
};

if (fnConfigMatches(LOCAL_GLX_USE_GL, 1) &&
    fnConfigMatches(LOCAL_GLX_RED_SIZE, 8)...

Comment 48

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #47)
> Comment on attachment 8997209 [details] [diff] [review]
> patch part 2 - Remove glXChooseVisual() usage
> 
> Review of attachment 8997209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me know what you think of these recommendations.

Thanks for the advice:) They make the patch cleaner.

> 
> ::: gfx/gl/GLContextProviderGLX.cpp
> @@ +908,5 @@
> > +    std::vector<XVisualInfo> visuals;
> > +    for (int i = 0; i < visualsLen; i++) {
> > +      visuals.push_back(xVisuals[i]);
> > +    }
> > +    XFree(xVisuals);
> 
> For ranged-for syntax:
> https://dxr.mozilla.org/mozilla-central/source/mfbt/Range.h
> For memory management:
> https://dxr.mozilla.org/mozilla-central/source/mfbt/ScopeExit.h

I will update in a next patch.

> 
> @@ +914,5 @@
> > +
> > +   // Get default visual
> > +
> > +   Visual* defaultVisual = DefaultVisual(display, screen);
> > +   auto it = std::find_if(visuals.begin(), visuals.end(),
> 
> const auto


I will update in a next patch.

> 
> @@ +915,5 @@
> > +   // Get default visual
> > +
> > +   Visual* defaultVisual = DefaultVisual(display, screen);
> > +   auto it = std::find_if(visuals.begin(), visuals.end(),
> > +                         [defaultVisual](const XVisualInfo& visualInfo) {
> 
> Just capture with [&] if the lambda is stack-scoped.

I will update in a next patch.

> 
> @@ +919,5 @@
> > +                         [defaultVisual](const XVisualInfo& visualInfo) {
> > +                           return visualInfo.visual == defaultVisual;
> > +                         });
> > +
> > +   MOZ_ASSERT(it != visuals.end());
> 
> Just `MOZ_ASSERT(false)` inside `if (it == visuals.end())` instead of
> repeating the conditional here.

I will update in a next patch.

> 
> @@ +922,5 @@
> > +
> > +   MOZ_ASSERT(it != visuals.end());
> > +   if (it == visuals.end()) {
> > +        return false;
> > +   }
> 
> Consider:
> const Range visualInfos(xVisuals, xVisuals+visualsLen);
> const auto defaultVisualInfo = [&]() -> const XVisualInfo* {
>     for (const auto& cur : visualInfos) {
>         if (cur.visual == defaultVisual)
>             return &cur;
>     }
>     return nullptr;
> }();
> if (!defaultVisualInfo)
>     return false;

I will update in a next patch.

> 
> @@ +923,5 @@
> > +   MOZ_ASSERT(it != visuals.end());
> > +   if (it == visuals.end()) {
> > +        return false;
> > +   }
> > +   const XVisualInfo& defaultVisualInfo = *it;
> 
> I think this would be more clear as:
> const auto defaultVisualInfo = [&]() -> const XVisualInfo* {
>   for (const auto& visualInfo : visuals) {
>     if (visualInfo.visual == defaultVisual)
>       return visualInfo

I will update in a next patch.

> 
> @@ +924,5 @@
> > +   if (it == visuals.end()) {
> > +        return false;
> > +   }
> > +   const XVisualInfo& defaultVisualInfo = *it;
> > +   const int depth = useAlpha ? 32 : 24;
> 
> s/depth/bpp/

I will update in a next patch.

> 
> @@ +935,5 @@
> > +        visuals.begin(), visuals.end(), std::back_inserter(compatibleVisuals),
> > +        [&defaultVisualInfo, depth](const XVisualInfo& visual) {
> > +            return visual.depth == depth &&
> > +                   visual.c_class == defaultVisualInfo.c_class;
> > +        });
> 
> This is one extra line:
> for (const auto& x : visuals) {
>   if (x.depth == depth &&
>       x.c_class == defaultVisualInfo.c_class)
>   {
>     compatibleVisuals.push_back(x);
>   }
> }


This part will be removed when the two loops is merged.

> 
> @@ +942,5 @@
> > +
> > +    const int resquestedAlphaSize = useAlpha ? 8 : 0;
> > +    const int resquestedDepthSize = useWebRender ? 24 : 0;
> > +
> > +    for (auto& visual : compatibleVisuals) {
> 
> So you're gathering all the compatible visuals, and then looping over them
> to choose the one you want.
> Merge these two loops, looping over /all/ visuals, skipping ones that aren't
> compatible, and then skipping ones that don't match our needs.

I will update in a next patch.

> 
> @@ +945,5 @@
> > +
> > +    for (auto& visual : compatibleVisuals) {
> > +        int supportsGl, doubleBuffer, redSize, greenSize, blueSize, alphaSize, depthSize;
> > +        if (!sGLXLibrary.fGetConfig(display, &visual, LOCAL_GLX_USE_GL, &supportsGl) &&
> > +            supportsGl &&
> 
> const auto fnConfigMatches = [&](const int pname, const int expected) {
>   int actual;
>   if (!sGLXLibrary.fGetConfig(display, &visual, pname, &actual))
>     return false;
>   return actual == expected;
> };
> 
> if (fnConfigMatches(LOCAL_GLX_USE_GL, 1) &&
>     fnConfigMatches(LOCAL_GLX_RED_SIZE, 8)...

I will update in a next patch. glXGetConfig() returns 0 on success.

Comment 49

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Remove glXChooseVisual() usage

Comment 50

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Remove glXChooseVisual() usage

Comment 51

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8997289 [details] [diff] [review]
patch part 2 - Remove glXChooseVisual() usage

:jgilbert, can you review the patch again?

Comment 52

[Kelsey Gilbert [:jgilbert]]

Is bug 1478454 related to this?

Comment 54

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #52)
> Is bug 1478454 related to this?

It is related but orthogonal problem. This bug is for fixing how to choose visual. bug 1478454 is for fixing the GlxContext creation failure by bad match error. I did not see the bad match error when attachment 8997289 [details] [diff] [review] is applied.

As in Comment 34 and Comment 22, the bad match error is going to be addressed in bug 1478454.

Comment 56

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8997289 [details] [diff] [review]
patch part 2 - Remove glXChooseVisual() usage

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

Great, thanks!

::: gfx/gl/GLContextProviderGLX.cpp
@@ +952,5 @@
> +            fnConfigMatches(LOCAL_GLX_RED_SIZE, 8) &&
> +            fnConfigMatches(LOCAL_GLX_GREEN_SIZE, 8) &&
> +            fnConfigMatches(LOCAL_GLX_BLUE_SIZE, 8) &&
> +            fnConfigMatches(LOCAL_GLX_ALPHA_SIZE, alphaSize) &&
> +            fnConfigMatches(LOCAL_GLX_DEPTH_SIZE, depthSize)) {

Prefer { on its own line for multi-line conditionals like this.

Comment 57

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #56)
> Comment on attachment 8997289 [details] [diff] [review]
> patch part 2 - Remove glXChooseVisual() usage
> 
> Review of attachment 8997289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks!
> 
> ::: gfx/gl/GLContextProviderGLX.cpp
> @@ +952,5 @@
> > +            fnConfigMatches(LOCAL_GLX_RED_SIZE, 8) &&
> > +            fnConfigMatches(LOCAL_GLX_GREEN_SIZE, 8) &&
> > +            fnConfigMatches(LOCAL_GLX_BLUE_SIZE, 8) &&
> > +            fnConfigMatches(LOCAL_GLX_ALPHA_SIZE, alphaSize) &&
> > +            fnConfigMatches(LOCAL_GLX_DEPTH_SIZE, depthSize)) {
> 
> Prefer { on its own line for multi-line conditionals like this.

Thanks for the review! I will update in a next patch.

Comment 58

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Remove glXChooseVisual() usage

Apply the comment.

Comment 59

[Sotaro Ikeda [:sotaro]]

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

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6764dc17395
part 1 - Expose glxGetConfig() in GLXLibrary r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d745e4118de7
part 2 - Remove glXChooseVisual() usage r=jgilbert

Comment 61

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e6764dc17395
https://hg.mozilla.org/mozilla-central/rev/d745e4118de7

Comment 62

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

I cannot reproduce the issue in Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0 ID:20180807100107 anymore. good work!