Community
Participate
Working Groups
Steps to reproduce: 1. Open 5 form-based Mylyn task editor 2. Use Close Others Result on Windows: Eclipse sometimes fails to repaint the gradients of sections or the SharedHeaderFormEditor that is still open. When the editor is resized or Eclipse is minimized/maximized the problem fixes itself. This symptoms are same as the ones described on bug 218412. Result on Linux: The program 'Eclipse' received an X Window System error. This probably reflects a bug in the program. The error was 'BadPixmap (invalid Pixmap parameter)'. (Details: serial 187174 error_code 4 request_code 56 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.) Result on Mac OS X: The open SharedHeaderFormEditor does not refresh anymore and stays blank until closed. Eclipse version: I20080530-1730 (3.4RC3). I can not reproduce this problem with Eclipse 3.3. I am unsure whether this problem is caused by Mylyn or if this is a bug in platform but I noticed that the way resources are shared between form-based editors has changed in 3.4 (bug 200690, bug 202474) which might be related. This bug is a blocker for the Mylyn 3.4RC4 release. I'd be happy to provide more details or do further debugging if anyone can provide pointers where to look next.
(In reply to comment #0) > 1. Open 5 form-based Mylyn task editor Does it happen with 5 (or more) PDE editors? Since Eclipse crashed, were any VM logs generated?
(In reply to comment #0) > The program 'Eclipse' received an X Window System error. When I say crash I'm referring to this. I imagine Eclipse crashed here.
Yes, Eclipse just exits. No hs_err_pid file is written. I could not reproduce the problem with manifest editors but PDE extends FormEditor rather than SharedHeaderFormEditor.
One theory which I would like to test is that this is a regression in org.eclipse.ui.forms. My first guess would be that this is not the case since very little has changed which would affect this case. The API for org.eclipse.ui.forms has not changes since Eclipse 3.3, so I would appreciate it if you could perform the folowing test: Sgtrat by downloading Eclipse 3.3.2. In your Eclipse 3.4 + Mylyn installation import the deployable plugin org.eclipse.ui.forms from the Eclipse 3.3.2 installation. Start a new run time workbench and see if the problem is still reproducible. If the problem goes away close the org.eclipse.ui.forms plugin you imported and see if the problem comes back. If this is the case then this would clearly point to a regression in org.eclipse.ui.forms. If the problem is unaffected by the version of org.eclipse.ui.forms then it is less clear where the problem lies, it may still be an old bug in org.eclipse.ui.forms, or it could be a bug in Mylyn or even SWT.
I imported org.eclipse.ui.forms version 3.3.0.v20070511 from Eclipse 3.3.2 into my workspace. Now I get this stack trace when trying to open a task editor: java.lang.IllegalArgumentException: Index out of bounds at org.eclipse.swt.SWT.error(SWT.java:3759) at org.eclipse.swt.SWT.error(SWT.java:3693) at org.eclipse.swt.SWT.error(SWT.java:3664) at org.eclipse.swt.custom.CTabFolder.getItem(CTabFolder.java:1187) at org.eclipse.ui.part.MultiPageEditorPart.getItem(MultiPageEditorPart.java:596) at org.eclipse.ui.part.MultiPageEditorPart.getEditor(MultiPageEditorPart.java:507) at org.eclipse.ui.part.MultiPageEditorPart.activateSite(MultiPageEditorPart.java:859) at org.eclipse.ui.part.MultiPageEditorPart.pageChange(MultiPageEditorPart.java:775) at org.eclipse.ui.forms.editor.FormEditor.pageChange(FormEditor.java:509) at org.eclipse.ui.forms.editor.FormEditor.setActivePage(FormEditor.java:623) at org.eclipse.ui.forms.editor.SharedHeaderFormEditor.createPages(SharedHeaderFormEditor.java:105) at org.eclipse.ui.part.MultiPageEditorPart.createPartControl(MultiPageEditorPart.java:310) at org.eclipse.ui.internal.EditorReference.createPartHelper(EditorReference.java:661) at org.eclipse.ui.internal.EditorReference.createPart(EditorReference.java:428) at org.eclipse.ui.internal.WorkbenchPartReference.getPart(WorkbenchPartReference.java:594) at org.eclipse.ui.internal.EditorReference.getEditor(EditorReference.java:266) [...]
I have overridden FormEditor.pageChanged() with a modified copy of the super class to work around the error. I haven't been able to reproduce the crash with the Eclipse 3.3.2 version of org.eclipse.ui.forms so far. Running the 3.4 version with the same modifications of pageChanged() still crashes Eclipse.
Thanks Steffen, this is very useful information. If you are able to narrow the point at which it started failing down any further that would help even more. According to Bug 218412 the problem (assuming that it is the same problem) was present in 3.4M5. That bug report does not state which milestone was being used before the problem showed up but I'm guessing that you would not have to go back too many milestones to get to the point before this regressed. In the meantime we are going to see if we can reproduce the bug at our end.
Here is what I found: 1. Open two task editors 2. Close the editor opened first 3. Close the editor opened second Now FormImages.ids contains disposed images (which doesn't seem right). Here is what I think is happening: When the second editor is closed FormImages.markFinished() disposes it's gradients but fails to lookup the id in images. As a fall back it disposes the passed image but since the image is still referenced in images it now contains disposed images. The reason the lookup fails is that although id is the right object reference that was obtained from ids its hash code has changed and therefore the lookup in images fails. The hashCode() implementation of FormImages.ImageIdentifier takes colors into account which change their hash code when disposed. It seems that when I close the first editor the colors referenced by FormImages.ImageIdentifier get disposed which changes the hash code of the image identifier. If I comment out the code in FormImages.ImageIdentifier.hashCode() and FormImages.ImageIdentifier.equals() that loops through the fColors array everything is happy (this is not a proper fix as it will fail if gradients are the size but use different colors, it might be better for instance to hold on to RGB objects instead of Color objects in FormImages.ImageIdentifier to get a stable hash code).
The reason that bug does not surface in PDE seems to be that PDEFormEditor overrides createToolkit(): protected FormToolkit createToolkit(Display display) { // Create a toolkit that shares colors between editors. return new FormToolkit(PDEPlugin.getDefault().getFormColors(display)); } This prevents FormToolkit from disposing the colors since they are marked as shared. For now I will implement this the same way in Mylyn to prevent crashes but this bug potentially affects anyone who uses SharedHeaderFormEditor.
It might be too late to address in 3.4, but in that case in 3.5 you should look at using org.eclipse.jface.resource.ResourceManager and org.eclipse.jface.resource.LocalResourceManager to manage shared fonts and images. localManager = new LocalResourceManager(JFaceResources.getResources()); The local resource manager can be created with each toolkit, used to create your fonts and images, and can be disposed with your toolkit. It keeps track of all the references to a shared resource, and can be used to create colors, fonts, and images. PW
I've opened Bug 236418 to investigate using a LocalResourceManager PW
FormImages is new in 3.4. Thanks for finding and diagnosing this problem Steffen! For 3.4.1 I will look into fixing the FormImages.ImageIdentifier.hashCode() and FormImages.ImageIdentifier.equals() as this is the lowest risk fix to the problem. I will also investigate the possibility of using a LocalResourceManager in 3.5.
Um... Isn't this a stop ship?
(In reply to comment #6) > I have overridden FormEditor.pageChanged() with a modified copy of the super > class to work around the error. Steffen has worked around the problem for Mylyn, so this is not a stop ship for them. However, the general problem in forms is a bad one so it might be worth considering this for 3.4. I am working on a fix now.
Created attachment 104334 [details] proposed fix The problem was that upon closing the editor, SharedHeaderFormEditor#dispose disposes of Color objects that later are used in ImageIdentifier#equals and ImageIdentifier#hashCode when a dispose listener calls FormImages#markFinished. This then leads to the corruption described by Steffen in comment 8. This patch uses RGB objects instead of Color objects in ImageIdentifier and ComplexImageIdentifier, as suggested by Steffen. With the patch, the logic in FormImages.java seems to work correctly.
The fix looks good. It is basically the exact same fix I was just testing myself. Thanks Boris.
Fix needs to be *extremely* carefully reviewed. Do we understand the performance impact?
I will continue testing it today. The performance impact is negligible. We are storing and using RGBs instead of Colors now. The hashcode and equality methods in RGBs are very simple and I highly doubt they are slower than the same methods from Color. This is the only difference with this fix.
Created attachment 104355 [details] fix with new junit I'm very confident in Boris' fix. I have attached a new patch which includes the fix (unchanged) as well as a new junit test to cover this case.
Patch looks very safe.
(In reply to comment #19) > Created an attachment (id=104355) [details] > fix with new junit > ImageIdentifier: If this guard is needed fRGBs[i] = color == null ? null : color.getRGB(); then equals(*) and hashCode() also need NPE protection. If this is addressed, then the fix is +1 minor: The copyright should include 2008 in both files. PW
(In reply to comment #21) > (In reply to comment #19) > > Created an attachment (id=104355) [details] [details] > > fix with new junit > > > > ImageIdentifier: If this guard is needed > fRGBs[i] = color == null ? null : color.getRGB(); > then equals(*) and hashCode() also need NPE protection. > > If this is addressed, then the fix is +1 I thought of the same too when looking at the patch but did not decide to raise the issue since the original code also just throws an NPE if someone were to just pass in a Color array filled with nulls. Perhaps some null checks should be done in the getGradient methods.
I have looked at the code with Boris and Silenio. Can we just take the code out, seeing as it is a cache or is that more dangerous?
The patch does not use the length that is passed in the construct to iterate over the array of colors, but it does check for a null color. There are other places in the original code that used colors.length instead fLength as well, so it is not a new problem in the patch. If the colors array is referenced outside of the class, the behavior will be changed by the patch. Should the junit test be part of the patch? On the plus the new code is tested. On the minus side, RC4 patches are supposed to be minimum.
PMC review requested. I have sent the folloing mail to eclipse-pmc. PMC bug fix request: for Bug 236324 – [Forms] Eclipse 3.4 crashes / failes to repaint when "Close Others" is used with SharedHeaderFormEditor BUG URL: https://bugs.eclipse.org/bugs/show_bug.cgi?id=236324 GOALS/BENEFITS: This bug is a regression which was introduced early in Eclipse 3.4. It caused serious problems for the Mylyn project and was marked as a blocker because it caused a crash on Linux. While Mylyn has been able to work around this bug it could impact any user of SharedHeaderFormEditor. To create the symptoms it is necessary to have more than one editor open and then close them in a particular order, this could easily get missed in product testing but will show up in the real world. Because this is a regression which can affect existing code and because the consequences are severe we are requesting approval to fix this in Eclipse 3,4, RISKS: We have taken every possible step to minimize risk. A new JUnit test has been added . The patch has been reviewed by 4 different reviewers and we can see no way that this will cause regressions. We have also tested editors which use SharedHeaderFormEditor including the PDE editors and the Forms examples and found no problems there. PERFORMANCE IMPACT: None.
(In reply to comment #21) > ImageIdentifier: If this guard is needed > fRGBs[i] = color == null ? null : color.getRGB(); > then equals(*) and hashCode() also need NPE protection. Since, as Remy said, we did not have any null guards before I think we should leave them out for now and keep the fix as simple and trivial as possible. I have opened bug 236484 to track the addition of null checking at the getGradient level. This would ensure that null values never make it into the ImageIdentifier. The only way someone could hit this using public APIs (FormImages is internal) is if they called FormHeading.setTextBackground(...) and passed an array of Colors that had nulls. This doesn't make sense anyway and I don't actually think it's unreasonable to throw NPEs in this case. (In reply to comment #24) > The patch does not use the length that is passed in the construct to iterate > over the array of colors, but it does check for a null color. There are other > places in the original code that used colors.length instead fLength as well, so > it is not a new problem in the patch. fLength does not denote the length of the colors array. It gives the height (or width) of the desired gradient. Thus it is only used to generate a hashcode and test equality. > If the colors array is referenced outside of the class, the behavior will be > changed by the patch. This is true, but FormImages is an internal class and no legitimate uses of it from within Forms will be affected by this. Also, the behavioural change will reflect the original intent of the class. (In reply to comment #23) > I have looked at the code with Boris and Silenio. Can we just take the code > out, seeing as it is a cache or is that more dangerous? It would be possible to take the code out and revert the 3.3 behaviour, but the gain involved in sharing forms gradient images is substantial and the fix is very low risk.
In terms of benefits if this gets fixed in 3.4: My understanding is that any FormEditor that does not override createToolkit() to use a toolkit with shared colors can cause corruption of the shared gradients cache when it is closed. That means even though Mylyn and PDE are not affected directly they will still use the shared cache and Mylyn's automatic closing of task editors is likely to trigger this bug once the cache has been corrupted by any other plug-in.
Created attachment 104383 [details] patch with updated copyrights This patch is the same as the previous, but with updated copyrights as suggested in comment 21.
I also feel that pulling this feature out would be more disruptive than patching the code to fix this specific bug and has its own risks - a rollback would involve a lot more lines of code changes, some in classes that have been modified since this feature was introduced in 3.4M2. While I do not happy about making this change post RC4 I think that the alternatives are slightly worse (back out the entire class which performs caching) or a lot worse (leave the bug in).
Jazz has several FormEditor implementations and is affected by this problem. If the fix doesn't go into 3.4 we would have to work around this in several editors. A fix in eclipse itself would be much better. Not only for Jazz but for all clients of FormEditors.
The patch with updated copyrights has been applied. To respond to two of the previous comments. I did not add extra null pointer checks because we are post RC4 and the lack of null pointer checks is a very minor issue - I would prefer not to add minor cleanup changes at this stage. A null pointer would never get passed in under any useful scenario and no-one has complained via a bug report about getting a null pointer exception in this routine. I have committed the JUnit test change because it does not add any risk. Adding the extra test acts as a way of verifying that the fix has made it into the build and is working correctly so I would claim that it reduces risk.
It took us since RC4 to get to the root of this slippery problem--started with UA bug 218412, which should be fixed now--so it's *great* to see this fixed in time for 3.4.
*** Bug 218412 has been marked as a duplicate of this bug. ***
*** Bug 214832 has been marked as a duplicate of this bug. ***
