Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2630773002:
Avoid blocking while mapping an X11 window (Closed)
|
Created:
3 years, 11 months ago by Tom (Use chromium acct) Modified:
3 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tdresser+watch_chromium.org, tfarina, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid blocking while mapping an X11 window
For most window managers, calling XMapWindow causes a corresponding
MapNotify event to fire, indicating it is ready for drawing. Since this
usually happens immediately, but asynchronously, chromium previously
blocked the UI thread until it got this event, to avoid calling any
further X11 commands on the window until it was ready.
Unfortunately, not all window managers immediately map the window
after a MapRequest. For example, the i3 window manager will not map
the created window while you're in fullscreen mode (i.e. youtube
video). This means chromium blocks indefinitely until the user
manually exits out of fullscreen mode.
The fix here is to avoid blocking, since we cannot make the assumption
that we'll get a MapNotify event quickly. By simply not blocking on
window map, we create two issues.
The first issue occurs when dragging tabs outside a window to create a
new one. The newly created window would fail to size itself properly
according to the space given to it by the window manager. This was
because the delayed_resize_task_ would get cancelled by SetBounds during
the time the thread would have otherwise been blocked.
This issue was fixed by only cancelling the task when the bounds changed
in that function, since that's the only time it will call OnHostResized
itself.
The second issue occurs during TooltipControllerTests, where a tooltip
is shown/hidden, and then immediately calls IsVisible() on the
widget (which eventually calls it on the underlying native
window). Since the mapping is now async, IsVisible() was false until
MapNotify was called.
The fix for this was to keep track of an additional variable for when we
are waiting for a MapNotify event, and using that to make IsVisible()
true during this time (after calling MapWindow, and before getting
MapNotify).
BUG=505669, 670959
patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001)
Review-Url: https://codereview.chromium.org/2630773002
Cr-Commit-Position: refs/heads/master@{#456098}
Committed: https://chromium.googlesource.com/chromium/src/+/662c24fc6debb33ee34a927d21dc26154b8973d6
Patch Set 1 #Patch Set 2 : Fix a few tests #Patch Set 3 : Fix CrOS build #
Total comments: 2
Patch Set 4 : fix flaky tests #
Total comments: 10
Patch Set 5 : Address feedback #
Messages
Total messages: 94 (57 generated)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thomasanderson@google.com changed reviewers: + sadrul@chromium.org
sadrul@ PTAL This was coped from http://crrev.com/2329323002#ps120001 A few tests have become flaky: ManagePasswordsBubbleViewTest.AutoSigninNoFocus SSLClientCertificateSelectorMultiProfileTest.Escape SSLClientCertificateSelectorMultiProfileTest.SelectDefault Do you know why these might be failing? Maybe just disable them for now and file a bug?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/01/26 23:37:39, Tom Anderson wrote: > sadrul@ PTAL > This was coped from http://crrev.com/2329323002#ps120001 > > A few tests have become flaky: > ManagePasswordsBubbleViewTest.AutoSigninNoFocus > SSLClientCertificateSelectorMultiProfileTest.Escape > SSLClientCertificateSelectorMultiProfileTest.SelectDefault > > Do you know why these might be failing? Maybe just disable them for now and > file a bug? That makes me a little bit nervous. Are you able to repro the failures locally? I looked at ManagePasswordsBubbleViewTest.AutoSigninNoFocus, and it looks like it expects that the Browser is activated/shown when it probably isn't the case. It may be possible to use views::test::WidgetActivationWaiter for this? Not sure. (from a quick look at SSLClientCertificateSelectorMultiProfileTest, something similar may fix those two too)
https://codereview.chromium.org/2630773002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/2630773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:120: #if defined(OS_LINUX) && defined(USE_X11) You can use just 'defined(USE_X11)'
On 2017/01/27 21:40:51, sadrul wrote: > On 2017/01/26 23:37:39, Tom Anderson wrote: > > sadrul@ PTAL > > This was coped from http://crrev.com/2329323002#ps120001 > > > > A few tests have become flaky: > > ManagePasswordsBubbleViewTest.AutoSigninNoFocus > > SSLClientCertificateSelectorMultiProfileTest.Escape > > SSLClientCertificateSelectorMultiProfileTest.SelectDefault > > > > Do you know why these might be failing? Maybe just disable them for now and > > file a bug? > > That makes me a little bit nervous. Are you able to repro the failures locally? > > I looked at ManagePasswordsBubbleViewTest.AutoSigninNoFocus, and it looks like > it expects that the Browser is activated/shown when it probably isn't the case. > It may be possible to use views::test::WidgetActivationWaiter for this? Not > sure. (from a quick look at SSLClientCertificateSelectorMultiProfileTest, > something similar may fix those two too) Thanks sadrul, WidgetActivationWaiter seemed to do the trick https://codereview.chromium.org/2630773002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/2630773002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:120: #if defined(OS_LINUX) && defined(USE_X11) On 2017/01/27 21:41:01, sadrul wrote: > You can use just 'defined(USE_X11)' Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
cool! lgtm https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2175: has_window_focus_ = false; Should this update is_visible_ too?
https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2175: has_window_focus_ = false; On 2017/02/04 02:27:51, sadrul wrote: > Should this update is_visible_ too? no, is_visible is updated when the (un)map notify request is sent, window_mapped is updated on a (un)map notify
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by thomasanderson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thomasanderson@google.com changed reviewers: + sky@chromium.org
+sky for OWNERS approval for: chrome/browser/extensions/extension_commands_global_registry_apitest.cc chrome/browser/ui/views/passwords/manage_passwords_bubble_view_interactive_uitest.cc chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc
I suspect your change is going to introduce all sorts of racyness... https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:182: views::DesktopWindowTreeHostX11* host = Your description makes the cases for production code, but in test code can't we block until mapped?
https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:182: views::DesktopWindowTreeHostX11* host = On 2017/02/06 22:25:28, sky wrote: > Your description makes the cases for production code, but in test code can't we > block until mapped? We could. Where would we add such code?
On 2017/02/06 22:31:08, Tom Anderson wrote: > https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... > File chrome/browser/extensions/extension_commands_global_registry_apitest.cc > (right): > > https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/extension_commands_global_registry_apitest.cc:182: > views::DesktopWindowTreeHostX11* host = > On 2017/02/06 22:25:28, sky wrote: > > Your description makes the cases for production code, but in test code can't > we > > block until mapped? > > We could. Where would we add such code? Maybe some where in ui/base/test?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@ I don't believe this CL introduces any flakiness, can we land it as-is? browser_tests on linux_chromium_rel_ng on the latest PS had 11 retries. I checked 2 other CLs and they had 12 retries each, so this doesn't seem out of the ordinary.
Description was changed from ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers fire MapNotify in all cases. For example, the i3 window manager will not fire MapNotify for a newly created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) ========== to ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers fire MapNotify in all cases. For example, the i3 window manager will not fire MapNotify for a newly created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) ==========
sky@chromium.org changed reviewers: + derat@chromium.org, erg@chromium.org
+more X experts (derat and erg) as I'm still nervous this is going to cause all sorts of problem. Having to cache the visible is just one thing, I worry it'll end up being a bunch more.
i share scott's concerns about this having the potential to introduce tricky bugs. i'm skeptical that doing this to try to fix behavior under obscure window managers is worth the risk (and i say this as someone who uses an obscure window manager). i'm also still confused about why override-redirect windows aren't used for dragged tabs (assuming that that's still the case; see e.g. https://codereview.chromium.org/1456963002/). https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1216: XWithdrawWindow(xdisplay_, xwindow_, 0); this also unmaps the window, right? is there a reason why we don't set window_mapped_ to false here and instead wait for UnmapNotify?
It seems wrong that apps have to work around the quirks of window managers. At what point is it fair to say the window manager is at fault? -Scott On Thu, Mar 9, 2017 at 7:07 AM, <derat@chromium.org> wrote: > i share scott's concerns about this having the potential to introduce tricky > bugs. i'm skeptical that doing this to try to fix behavior under obscure > window > managers is worth the risk (and i say this as someone who uses an obscure > window > manager). > > i'm also still confused about why override-redirect windows aren't used for > dragged tabs (assuming that that's still the case; see e.g. > https://codereview.chromium.org/1456963002/). > > > https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc > (right): > > https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1216: > XWithdrawWindow(xdisplay_, xwindow_, 0); > this also unmaps the window, right? is there a reason why we don't set > window_mapped_ to false here and instead wait for UnmapNotify? > > https://codereview.chromium.org/2630773002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thomasanderson@google.com changed reviewers: + danakj@chromium.org
+danakj On 2017/03/09 15:07:22, Daniel Erat wrote: > i share scott's concerns about this having the potential to introduce tricky > bugs. What sorts of bugs might this cause, specifically? No other toolkit does this blocking. Not Gtk, not Qt, only Aura. Most X11 APIs are agnostic to whether or not the window is mapped. The only one that is not is XGrabPointer, which will fail on an unmapped window. However, in my testing of various obscure window managers, tab-dragging still works, and if it did break, you would just have to restart the drag. On 2017/03/09 17:29:52, sky wrote: > It seems wrong that apps have to work around the quirks of window > managers. At what point is it fair to say the window manager is at > fault? > > -Scott I think the workaround is having to do the blocking in the first place.
On 2017/03/09 17:29:52, sky wrote: > It seems wrong that apps have to work around the quirks of window > managers. At what point is it fair to say the window manager is at > fault? My understanding is we're already in the weeds of an informally specced system. A lot of this stuff is more social consensus than technical. While this is only a problem in some minority window managers, we support the Linux port for Googlers, and Googlers are much more likely to run weird window managers than the general linux population. And, as Tom has pointed out, aura is the only toolkit that does this sort of blocking.
I'm not an expert here. If Evan and Elliot are ok with this, I'm ok with it. -Scott On Thu, Mar 9, 2017 at 11:04 AM, <erg@chromium.org> wrote: > On 2017/03/09 17:29:52, sky wrote: >> It seems wrong that apps have to work around the quirks of window >> managers. At what point is it fair to say the window manager is at >> fault? > > My understanding is we're already in the weeds of an informally specced > system. > A lot of this stuff is more social consensus than technical. While this is > only > a problem in some minority window managers, we support the Linux port for > Googlers, and Googlers are much more likely to run weird window managers > than > the general linux population. > > And, as Tom has pointed out, aura is the only toolkit that does this sort of > blocking. > > https://codereview.chromium.org/2630773002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I am confused by this claim: > Unfortunately, not all window managers fire MapNotify in all cases. For > example, the i3 window manager will not fire MapNotify for a newly > created window while you're in fullscreen mode (i.e. youtube > video). This means chromium blocks indefinitely until the user manually > exits out of fullscreen mode. The window manager doesn't send MapNotify directly to chrome. The X server generates that when the window manager maps the window. If the window manager doesn't map the window, then it won't be visible. Here's a concrete example of a WM making the client (chrome) window visible: https://github.com/danakj/openbox/blob/9e8813e111cbe6c1088f6abbc771a29470f05f... A badly behaved WM could reparent the client, and map it before mapping the frame, making MapNotify occur even tho its parent has not been mapped, causing the window to not be visible yet. But I don't understand how MapNotify could just not happen, why the window did become visible. My intuition is that some other event would have to replace the MapNotify, but I don't know what that would be and find no supporting documentation for it. https://tronche.com/gui/x/xlib/window/map.html Expose events can tell if you are actually visible as opposed to mapped, but take into account occlusion (when no compositing manager is used). https://tronche.com/gui/x/xlib-tutorial/ mentions that "X has a stateless drawing model, the content of the window may be lost when the window isn't on the screen." in reference to waiting for MapNotify before drawing. Blocking is generally bad, but we should probably not rely on drawing before MapNotify happens? You can expect broadly different results in this regard with GPU vs Software outputs probably too, and with a compositing manager or not. https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:186: auto observer = base::MakeUnique<GlobalCommandTreeHostObserver>(); nit: can u pass a lambda (or closure) to the observer that has the actions u want it to do (the key events) so that code stays down here? https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:301: bool is_visible_; I would name these more literally as the state they represent. window_mapped_in_client_ window_mapped_in_server_ or something like that.
On 2017/03/09 18:47:10, Tom Anderson wrote: > +danakj > > On 2017/03/09 15:07:22, Daniel Erat wrote: > > i share scott's concerns about this having the potential to introduce tricky > > bugs. > > What sorts of bugs might this cause, specifically? No other toolkit does > this blocking. Not Gtk, not Qt, only Aura. > > Most X11 APIs are agnostic to whether or not the window is mapped. The > only one that is not is XGrabPointer, which will fail on an unmapped > window. However, in my testing of various obscure window managers, > tab-dragging still works, and if it did break, you would just have to > restart the drag. you're not supposed to draw on a window before it's mapped, i believe (which dana mentioned below). i'm concerned that changes around whether we wait for windows to be mapped or not could make it easy to introduce bugs here. with that said, i'm not advocating for chrome blocking while waiting for the window manager to map newly-created windows (which it's allowed to not do at all if it wants). i'm just saying that ICCCM and EWMH are poor specs and that changes around chrome's interactions with the WM often introduce new problems. for tab-dragging, i maintain that we'd probably be better off if we avoided the WM entirely by using override-redirect windows. if this works for metacity/compiz/*box/etc. and doesn't completely break other window managers, i'm okay with it. > On 2017/03/09 17:29:52, sky wrote: > > It seems wrong that apps have to work around the quirks of window > > managers. At what point is it fair to say the window manager is at > > fault? > > > > -Scott > > I think the workaround is having to do the blocking in the first place.
On Thu, Mar 9, 2017 at 4:13 PM, <derat@chromium.org> wrote: > On 2017/03/09 18:47:10, Tom Anderson wrote: > > +danakj > > > > On 2017/03/09 15:07:22, Daniel Erat wrote: > > > i share scott's concerns about this having the potential to introduce > tricky > > > bugs. > > > > What sorts of bugs might this cause, specifically? No other toolkit does > > this blocking. Not Gtk, not Qt, only Aura. > > > > Most X11 APIs are agnostic to whether or not the window is mapped. The > > only one that is not is XGrabPointer, which will fail on an unmapped > > window. However, in my testing of various obscure window managers, > > tab-dragging still works, and if it did break, you would just have to > > restart the drag. > > you're not supposed to draw on a window before it's mapped, i believe > (which > dana mentioned below). i'm concerned that changes around whether we wait > for > windows to be mapped or not could make it easy to introduce bugs here. > > with that said, i'm not advocating for chrome blocking while waiting for > the > window manager to map newly-created windows (which it's allowed to not do > at all > if it wants). i'm just saying that ICCCM and EWMH are poor specs and that > changes around chrome's interactions with the WM often introduce new > problems. > for tab-dragging, i maintain that we'd probably be better off if we > avoided the > WM entirely by using override-redirect windows. > With the old tab-dragging UI I'd agree but now the tab drag just makes a new window and initiates a drag on it, which is different. Using override-redirect could produce some surprising behaviour on a window that looks like a normal chrome window. For instance - When the drag ends you want the window to stop being override-redirect. This means unmapping and remapping it. Surely unpleasant. - If you have WM decorations on your window, when you tab-drag, the new window wouldn't get them, and then on release it would acquire them again. if this works for metacity/compiz/*box/etc. and doesn't completely break > other > window managers, i'm okay with it. > > > On 2017/03/09 17:29:52, sky wrote: > > > It seems wrong that apps have to work around the quirks of window > > > managers. At what point is it fair to say the window manager is at > > > fault? > > > > > > -Scott > > > > I think the workaround is having to do the blocking in the first place. > > > > https://codereview.chromium.org/2630773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/09 21:23:45, danakj wrote: > On Thu, Mar 9, 2017 at 4:13 PM, <mailto:derat@chromium.org> wrote: > > > On 2017/03/09 18:47:10, Tom Anderson wrote: > > > +danakj > > > > > > On 2017/03/09 15:07:22, Daniel Erat wrote: > > > > i share scott's concerns about this having the potential to introduce > > tricky > > > > bugs. > > > > > > What sorts of bugs might this cause, specifically? No other toolkit does > > > this blocking. Not Gtk, not Qt, only Aura. > > > > > > Most X11 APIs are agnostic to whether or not the window is mapped. The > > > only one that is not is XGrabPointer, which will fail on an unmapped > > > window. However, in my testing of various obscure window managers, > > > tab-dragging still works, and if it did break, you would just have to > > > restart the drag. > > > > you're not supposed to draw on a window before it's mapped, i believe > > (which > > dana mentioned below). i'm concerned that changes around whether we wait > > for > > windows to be mapped or not could make it easy to introduce bugs here. > > > > with that said, i'm not advocating for chrome blocking while waiting for > > the > > window manager to map newly-created windows (which it's allowed to not do > > at all > > if it wants). i'm just saying that ICCCM and EWMH are poor specs and that > > changes around chrome's interactions with the WM often introduce new > > problems. > > for tab-dragging, i maintain that we'd probably be better off if we > > avoided the > > WM entirely by using override-redirect windows. > > > > With the old tab-dragging UI I'd agree but now the tab drag just makes a > new window and initiates a drag on it, which is different. Using > override-redirect could produce some surprising behaviour on a window that > looks like a normal chrome window. For instance > - When the drag ends you want the window to stop being override-redirect. > This means unmapping and remapping it. Surely unpleasant. > - If you have WM decorations on your window, when you tab-drag, the new > window wouldn't get them, and then on release it would acquire them again. thanks, the decoration issue is a good point -- i wasn't sure to what extent people turn off the custom frame. i use ion3/notion, under which tab-dragging has always been completely broken and unusable, so i'm happy to defer to others on this point. :-) > if this works for metacity/compiz/*box/etc. and doesn't completely break > > other > > window managers, i'm okay with it. > > > > > On 2017/03/09 17:29:52, sky wrote: > > > > It seems wrong that apps have to work around the quirks of window > > > > managers. At what point is it fair to say the window manager is at > > > > fault? > > > > > > > > -Scott > > > > > > I think the workaround is having to do the blocking in the first place. > > > > > > > > https://codereview.chromium.org/2630773002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers fire MapNotify in all cases. For example, the i3 window manager will not fire MapNotify for a newly created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) ========== to ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers immediately map the window after a MapRequest. For example, the i3 window manager will not map the created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) ==========
On 2017/03/09 20:45:34, danakj wrote: > I am confused by this claim: > > > Unfortunately, not all window managers fire MapNotify in all cases. For > > example, the i3 window manager will not fire MapNotify for a newly > > created window while you're in fullscreen mode (i.e. youtube > > video). This means chromium blocks indefinitely until the user manually > > exits out of fullscreen mode. > > The window manager doesn't send MapNotify directly to chrome. The X server > generates that when the window manager maps the window. If the window manager > doesn't map the window, then it won't be visible. Changed that paragraph. The original CL description was copied from here https://codereview.chromium.org/2329323002#ps120001 On 2017/03/09 21:13:43, Daniel Erat wrote: > you're not supposed to draw on a window before it's mapped, i believe (which > dana mentioned below). i'm concerned that changes around whether we wait for > windows to be mapped or not could make it easy to introduce bugs here. > We redraw on an Expose https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win... Is this sufficient, or do we need to redraw on a MapNotify too? (I can't see any bad drawing atm) https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry_apitest.cc (right): https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry_apitest.cc:186: auto observer = base::MakeUnique<GlobalCommandTreeHostObserver>(); On 2017/03/09 20:45:34, danakj wrote: > nit: can u pass a lambda (or closure) to the observer that has the actions u > want it to do (the key events) so that code stays down here? Done (sort of, I couldn't figure out how to use the lambda). https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1216: XWithdrawWindow(xdisplay_, xwindow_, 0); On 2017/03/09 15:07:22, Daniel Erat wrote: > this also unmaps the window, right? is there a reason why we don't set > window_mapped_ to false here and instead wait for UnmapNotify? Before, we had wait_for_unmap_ that sort of did this. But, now there's no point in adding a BlockUntilWindowUnmapped https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:301: bool is_visible_; On 2017/03/09 20:45:34, danakj wrote: > I would name these more literally as the state they represent. > > window_mapped_in_client_ > window_mapped_in_server_ > > or something like that. Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers immediately map the window after a MapRequest. For example, the i3 window manager will not map the created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) ========== to ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers immediately map the window after a MapRequest. For example, the i3 window manager will not map the created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669,670959 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) ==========
On 2017/03/10 01:29:58, Tom Anderson wrote: > On 2017/03/09 20:45:34, danakj wrote: > > I am confused by this claim: > > > > > Unfortunately, not all window managers fire MapNotify in all cases. For > > > example, the i3 window manager will not fire MapNotify for a newly > > > created window while you're in fullscreen mode (i.e. youtube > > > video). This means chromium blocks indefinitely until the user manually > > > exits out of fullscreen mode. > > > > The window manager doesn't send MapNotify directly to chrome. The X server > > generates that when the window manager maps the window. If the window manager > > doesn't map the window, then it won't be visible. > > Changed that paragraph. The original CL description was copied from here > https://codereview.chromium.org/2329323002#ps120001 > > On 2017/03/09 21:13:43, Daniel Erat wrote: > > you're not supposed to draw on a window before it's mapped, i believe (which > > dana mentioned below). i'm concerned that changes around whether we wait for > > windows to be mapped or not could make it easy to introduce bugs here. > > > > We redraw on an Expose > https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win... > > Is this sufficient, or do we need to redraw on a MapNotify too? (I > can't see any bad drawing atm) thanks for the pointer! i think that redrawing only on expose is correct. > https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... > File chrome/browser/extensions/extension_commands_global_registry_apitest.cc > (right): > > https://codereview.chromium.org/2630773002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/extension_commands_global_registry_apitest.cc:186: > auto observer = base::MakeUnique<GlobalCommandTreeHostObserver>(); > On 2017/03/09 20:45:34, danakj wrote: > > nit: can u pass a lambda (or closure) to the observer that has the actions u > > want it to do (the key events) so that code stays down here? > > Done (sort of, I couldn't figure out how to use the lambda). > > https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): > > https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1216: > XWithdrawWindow(xdisplay_, xwindow_, 0); > On 2017/03/09 15:07:22, Daniel Erat wrote: > > this also unmaps the window, right? is there a reason why we don't set > > window_mapped_ to false here and instead wait for UnmapNotify? > > Before, we had wait_for_unmap_ that sort of did this. But, now there's no point > in adding a BlockUntilWindowUnmapped > > https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right): > > https://codereview.chromium.org/2630773002/diff/60001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:301: bool > is_visible_; > On 2017/03/09 20:45:34, danakj wrote: > > I would name these more literally as the state they represent. > > > > window_mapped_in_client_ > > window_mapped_in_server_ > > > > or something like that. > > Done.
lgtm
LGTM
Rubber stamp LGTM
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2630773002/#ps80001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489168047808650,
"parent_rev": "6fd7349089bc194ac6a08af6f94298987629dd9b", "commit_rev":
"662c24fc6debb33ee34a927d21dc26154b8973d6"}
Message was sent while issue was closed.
Description was changed from ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers immediately map the window after a MapRequest. For example, the i3 window manager will not map the created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669,670959 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) ========== to ========== Avoid blocking while mapping an X11 window For most window managers, calling XMapWindow causes a corresponding MapNotify event to fire, indicating it is ready for drawing. Since this usually happens immediately, but asynchronously, chromium previously blocked the UI thread until it got this event, to avoid calling any further X11 commands on the window until it was ready. Unfortunately, not all window managers immediately map the window after a MapRequest. For example, the i3 window manager will not map the created window while you're in fullscreen mode (i.e. youtube video). This means chromium blocks indefinitely until the user manually exits out of fullscreen mode. The fix here is to avoid blocking, since we cannot make the assumption that we'll get a MapNotify event quickly. By simply not blocking on window map, we create two issues. The first issue occurs when dragging tabs outside a window to create a new one. The newly created window would fail to size itself properly according to the space given to it by the window manager. This was because the delayed_resize_task_ would get cancelled by SetBounds during the time the thread would have otherwise been blocked. This issue was fixed by only cancelling the task when the bounds changed in that function, since that's the only time it will call OnHostResized itself. The second issue occurs during TooltipControllerTests, where a tooltip is shown/hidden, and then immediately calls IsVisible() on the widget (which eventually calls it on the underlying native window). Since the mapping is now async, IsVisible() was false until MapNotify was called. The fix for this was to keep track of an additional variable for when we are waiting for a MapNotify event, and using that to make IsVisible() true during this time (after calling MapWindow, and before getting MapNotify). BUG=505669,670959 patch from issue 2329323002 at patchset 120001 (http://crrev.com/2329323002#ps120001) Review-Url: https://codereview.chromium.org/2630773002 Cr-Commit-Position: refs/heads/master@{#456098} Committed: https://chromium.googlesource.com/chromium/src/+/662c24fc6debb33ee34a927d21dc... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/662c24fc6debb33ee34a927d21dc...
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
This made the msan bot pretty unhappy: https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes... Example: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory.full%2FL... [18754:18754:0310/105850.976775:16618164712:ERROR:desktop_window_tree_host_x11.cc(1147)] Not implemented reached in virtual void views::DesktopWindowTreeHostX11::InitModalType(ui::ModalType) ==18754==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x7f5ddb3dc5cc in ?? /home/tom/chromium/src/third_party/instrumented_libraries/scripts/out/Instrumented-msan-chained-origins-trusty/gen/third_party/instrumented_libraries/libx11-6/libx11-1.6.2/src/SetNrmHint.c:79:9 #1 0x124ddf4f in UpdateMinAndMaxSize ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1701:3 #2 0x124df791 in SetBoundsInPixels ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1252:5 #3 0x124d1ad3 in CenterWindow ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:672:3 #4 0x12477d55 in SetInitialBounds ui/views/widget/widget.cc:1508:25 #5 0x12475f35 in Init ui/views/widget/widget.cc:358:5 #6 0x1249cf52 in CreateDialogWidget ui/views/window/dialog_delegate.cc:45:11 #7 0x1ba0e5ad in CreateBrowserModalDialogViews components/constrained_window/constrained_window_views.cc:223:7 #8 0x84d62ac in TestBody components/constrained_window/constrained_window_views_unittest.cc:229:27 #9 0x133dd84c in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2458:12 #10 0x133dd84c in Run testing/gtest/src/gtest.cc:2474:0 #11 0x133e01d1 in Run testing/gtest/src/gtest.cc:2656:11 #12 0x133e1739 in Run testing/gtest/src/gtest.cc:2774:28 #13 0x13401c4d in RunAllTests testing/gtest/src/gtest.cc:4647:43 #14 0x13400abe in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 #15 0x13400abe in Run testing/gtest/src/gtest.cc:4255:0 #16 0x1331ae00 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46 #17 0x1331ae00 in Run base/test/test_suite.cc:271:0 #18 0x13324c1a in Run base/callback.h:85:12 #19 0x13324c1a in LaunchUnitTestsInternal base/test/launcher/unit_test_launcher.cc:211:0 #20 0x1332443d in LaunchUnitTests base/test/launcher/unit_test_launcher.cc:453:10 #21 0x9731e3 in ?? components/test/run_all_unittests.cc:8:10 #22 0x7f5dd6e11f44 in __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287:0 #23 0x902d58 in _start ??:? Uninitialized value was stored to memory at #0 0x124de1a4 in UpdateMinAndMaxSize ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1694:17 #1 0x124df791 in SetBoundsInPixels ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1252:5 #2 0x124d1ad3 in CenterWindow ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:672:3 #3 0x12477d55 in SetInitialBounds ui/views/widget/widget.cc:1508:25 #4 0x12475f35 in Init ui/views/widget/widget.cc:358:5 #5 0x1249cf52 in CreateDialogWidget ui/views/window/dialog_delegate.cc:45:11 #6 0x1ba0e5ad in CreateBrowserModalDialogViews components/constrained_window/constrained_window_views.cc:223:7 #7 0x84d62ac in TestBody components/constrained_window/constrained_window_views_unittest.cc:229:27 #8 0x133dd84c in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2458:12 #9 0x133dd84c in Run testing/gtest/src/gtest.cc:2474:0 #10 0x133e01d1 in Run testing/gtest/src/gtest.cc:2656:11 #11 0x133e1739 in Run testing/gtest/src/gtest.cc:2774:28 #12 0x13401c4d in RunAllTests testing/gtest/src/gtest.cc:4647:43 #13 0x13400abe in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 #14 0x13400abe in Run testing/gtest/src/gtest.cc:4255:0 #15 0x1331ae00 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46 #16 0x1331ae00 in Run base/test/test_suite.cc:271:0 #17 0x13324c1a in Run base/callback.h:85:12 #18 0x13324c1a in LaunchUnitTestsInternal base/test/launcher/unit_test_launcher.cc:211:0 #19 0x1332443d in LaunchUnitTests base/test/launcher/unit_test_launcher.cc:453:10 #20 0x9731e3 in ?? components/test/run_all_unittests.cc:8:10 #21 0x7f5dd6e11f44 in __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287:0 Uninitialized value was created by an allocation of 'hints' in the stack frame of function '_ZN5views24DesktopWindowTreeHostX1119UpdateMinAndMaxSizeEv' #0 0x124dd590 in UpdateMinAndMaxSize ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1669:0 Can you take a look?
Message was sent while issue was closed.
On 2017/03/10 23:41:43, Nico wrote: > This made the msan bot pretty unhappy: > > Can you take a look? Ok, investigating now
Message was sent while issue was closed.
On 2017/03/10 23:57:08, Tom Anderson wrote: > On 2017/03/10 23:41:43, Nico wrote: > > This made the msan bot pretty unhappy: > > > > Can you take a look? > > Ok, investigating now Fix created here https://codereview.chromium.org/2740933005/
Message was sent while issue was closed.
Makes sense. On Mar 10, 2017 7:18 PM, <thomasanderson@google.com> wrote: > On 2017/03/10 23:57:08, Tom Anderson wrote: > > On 2017/03/10 23:41:43, Nico wrote: > > > This made the msan bot pretty unhappy: > > > > > > Can you take a look? > > > > Ok, investigating now > > Fix created here > https://codereview.chromium.org/2740933005/ > > https://codereview.chromium.org/2630773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
faisalsalipada7@gmail.com changed reviewers: + faisalsalipada7@gmail.com
Message was sent while issue was closed.
faisalsalipada7@gmail.com |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
