Bugzilla – Full Text Bug Listing
| Summary: | [GTK] Shell.setBounds() with specific values crashes JVM or makes the shell non functional | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Andrey Loskutov <loskutov> | ||||
| Component: | SWT | Assignee: | Andrey Loskutov <loskutov> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | akurtakov, lufimtse, platform-swt-inbox | ||||
| Version: | 3.8.2 | ||||||
| Target Milestone: | 4.6 M2 | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| See Also: |
https://git.eclipse.org/r/51985 https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=84570dc1b48c0ba9e7f3220fae8e28aef46e7b21 |
||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
New Gerrit change created: https://git.eclipse.org/r/51985 What does non usable shell means? (In reply to Alexander Kurtakov from comment #2) > What does non usable shell means? The background is not painted, the shell can not be resized, closed, it is just a rectangle sitting on the screen until the JVM is closed or shell.dispose() is called. Hello Andrey,
Thank you for spotting this issue and submitting the patch.
As you correctly pointed out, GdkWindow has a limitation :
"The X and Y coordinates for a window are specified in pixels, relative to the parent window's origin. The origin of each window is its top left ("northwest") corner. Notice that a 16-bit signed integer is used; X windows have a maximum size of 32768 pixels. Negative values are allowed, but the window will be clipped by its parent window (only the portion inside the parent window will be visible)"
Source: http://www.linuxtopia.org/online_books/gui_toolkit_guides/gtk+_gnome_application_development/sec-gdkwindow_1.html
This is roughly 32 monitors in size assuming each monitor has a height of ~1000 pixels.
With that said, I'm not sure if users would really want windows that are 32 monitors wide. Imagine the 'Ok/Cancle' buttons are on the far right and is not easily accessible by keyboard.
May I suggest that we further build upon your patch and limit the dimensions to the maximum of Display.getBounds()?
This get's the maximum pixel area for the whole screen not including start-bars
(for multiple monitors this retrieves the span of all monitors combined, e.g on my dual-monitor system this is 3840 by 1080).
The reason why we found the issue was that we had a custom tooltip shell which size was calculated from given text. Reducing the size to display width can be too restrictive. So I've only limited the size to something we can handle without such side effects like crash. I'm not sure if there are controls which size exceed display size because they are nested in some scroll composite, so they could be damaged by the too strict limits. (In reply to comment #5) > The reason why we found the issue was that we had a custom tooltip shell which > size was calculated from given text. Reducing the size to display width can be > too restrictive. So I've only limited the size to something we can handle > without such side effects like crash. I'm not sure if there are controls which > size exceed display size because they are nested in some scroll composite, so > they could be damaged by the too strict limits. I see, that's a good point. In that case, may I suggest we only limit Shell.java:setBounds to be that of monitor size and leave Control.java as is? I don't see a use case for having a shell bigger than the size of all monitors (?), but as you mentioned Control.java can have the scrolled items to be larger. Thoughts? (In reply to Leo Ufimtsev from comment #6) > (In reply to comment #5) > > The reason why we found the issue was that we had a custom tooltip shell which > > size was calculated from given text. Reducing the size to display width can be > > too restrictive. So I've only limited the size to something we can handle > > without such side effects like crash. I'm not sure if there are controls which > > size exceed display size because they are nested in some scroll composite, so > > they could be damaged by the too strict limits. > > I see, that's a good point. > > In that case, may I suggest we only limit Shell.java:setBounds to be that > of monitor size and leave Control.java as is? > I don't see a use case for having a shell bigger than the size of all > monitors (?), but as you mentioned Control.java can have the scrolled items > to be larger. > > Thoughts? Few things. 1) I've changed Control not because it is needed for the crash fix but because shell doesn't override any of public setBounds() and setSize() methods. Restricting the checks to Shell only would mean we either have to override those 5 methods in the Shell in order to fix javadoc or to hack if(me instanceof Shell) into Control.int setBounds (int x, int y, int width, int height, boolean move, boolean resize) method just before the fix, and update javadocs to point out that "... note that shell bounds might not exceed ...". This is possible for sure - I don't know which way you like it more. 2) Restricting the Shell to Monitor size is kind of randomizer added to the application. Since users have all different monitor numbers/resolutions, we might see random and not reproducible issues because shells created by one user will be different in size as shells created by another user, both running exact same code and with exact same Eclipse window size. Investigating that kind of issues will add another layer of unpredictability to the SWT. I absolutely agree that there is no sense to create shells with the size 32767, but obviously our code hit this border. Do you know if there are similar limits exists already somewhere in SWT and how they are handled? We should be consistent here too. (In reply to comment #7) > (In reply to Leo Ufimtsev from comment #6) > > (In reply to comment #5) > > > The reason why we found the issue was that we had a custom tooltip shell > which > > > size was calculated from given text. Reducing the size to display width can > be > > > too restrictive. So I've only limited the size to something we can handle > > > without such side effects like crash. I'm not sure if there are controls > which > > > size exceed display size because they are nested in some scroll composite, > so > > > they could be damaged by the too strict limits. > > > > I see, that's a good point. > > > > In that case, may I suggest we only limit Shell.java:setBounds to be that > > of monitor size and leave Control.java as is? > > I don't see a use case for having a shell bigger than the size of all > > monitors (?), but as you mentioned Control.java can have the scrolled items > > to be larger. > > > > Thoughts? > > Few things. > > 1) I've changed Control not because it is needed for the crash fix but because > shell doesn't override any of public setBounds() and setSize() methods. > > Restricting the checks to Shell only would mean we either have to override those > 5 methods in the Shell in order to fix javadoc or to hack if(me instanceof > Shell) into Control.int setBounds (int x, int y, int width, int height, boolean > move, boolean resize) method just before the fix, and update javadocs to point > out that "... note that shell bounds might not exceed ...". > > This is possible for sure - I don't know which way you like it more. Oh, I see what you mean by the inconsistencies in the public javadoc business. That's a good point. > 2) Restricting the Shell to Monitor size is kind of randomizer added to the > application. Since users have all different monitor numbers/resolutions, we > might see random and not reproducible issues because shells created by one user > will be different in size as shells created by another user, both running exact > same code and with exact same Eclipse window size. Investigating that kind of > issues will add another layer of unpredictability to the SWT. I absolutely agree > that there is no sense to create shells with the size 32767, but obviously our > code hit this border. I did some more research. There seems to be already a mechanism in place somewhere that restricts the size of a 'shell' to the client area of the primary working area (excluding toolbars etc..). So I guess it would make it redundant to check twice. > Do you know if there are similar limits exists already > somewhere in SWT and how they are handled? We should be consistent here too. Well, there is a mechanism in place that caches and re-sizes a shell if it is moved while it is hidden/reparented: 42930: Bug 455671 - [GTK] Location of Shell gets reset after reparenting #2 [I0f6a0d1b] https://git.eclipse.org/r/#/c/42930/ This involves moving the shell which eventually perculates down to setBounds(). But it shouldn't conflict with your patch. And as above, the size of the shell is already limited to the size of the primary monitor client area. With the above, it seems like we're good to go. I reproduced the issue and verified that the patch has fixed the crash (gtk2) /pixel tearing(gtk3) issue. I think it's good for merging. Thank you very much for this patch. Gerrit change https://git.eclipse.org/r/51985 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=84570dc1b48c0ba9e7f3220fae8e28aef46e7b21 Nice catch. Pushed to master. |
Created attachment 255220 [details] Project with code to reproduce the crash We found this occasionally because the application tried to show a very long text in a tooltip, and this immediately crashed *entire* Eclipse JVM: (SWT:24589): Gdk-ERROR **: The program 'SWT' received an X Window System error. This probably reflects a bug in the program. The error was 'BadAlloc (insufficient resources for operation)'. (Details: serial 821 error_code 11 request_code 53 minor_code 0) (Note to programmers: normally, X errors are reported asynchronously; that is, you will receive the error a while after causing it. To debug your program, run it with the --sync command line option to change this behavior. You can then get a meaningful backtrace from your debugger if you break on the gdk_x_error() function.) I think this is a GTK bug, and there is no way to fix it on SWT side, but SWT should at least make sure we do no crash. The point is: if the requested shell width or height value is bigger 2^14, or more specific: if the (width % (2^15) >= 2^14), GTK crashes. So I can open a shell with bounds 32767 x 50 or 92768 x 50 but I can not open one with bounds 32768 x 50 or 102768 x 50. I guess GTK code uses initially unsigned int values of the size 2^15 but internally does some calculations with signed ints (so the positive part is of the value has size 2^14), and during the downcast they might use unexpected values. A standalone application to reproduce the issue is attached. I can reproduce the JVM crash on both 3.8.2 and 4.5.0 versions of SWT, with GTK2 only. On my GTK3 the *opened* shell seem to be not usable, but it does not crash the entire JVM. rpm -q gtk2 gtk2-2.24.22-5.el7.x86_64 rpm -q gtk3 gtk3-3.8.8-5.el7.x86_64