Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 331562

Summary: [nls tooling] PropertyKeyHyperlinkDetector throws IAE when file contains invalid unicode escape
Product: [Eclipse Project] JDT Reporter: Markus Keller <markus.kell.r>
Component: TextAssignee: Deepak Azad <deepakazad>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: daniel_megert
Version: 3.7   
Target Milestone: 3.7 M6   
Hardware: PC   
OS: Windows 7   
Whiteboard:

Description Markus Keller CLA 2010-12-01 11:40:53 EST 
HEAD, similar problem as bug 328601

- open x.properties (contains an invalid unicode escape):

bad=\u26
hello= world

- hold down Ctrl and hover over "hello" => IAE


!ENTRY org.eclipse.ui 4 0 2010-12-01 17:07:18.715
!MESSAGE Unhandled event loop exception
!STACK 0
java.lang.IllegalArgumentException: Malformed \uxxxx encoding.
	at java.util.Properties.loadConvert(Properties.java:552)
	at java.util.Properties.load0(Properties.java:375)
	at java.util.Properties.load(Properties.java:325)
	at org.eclipse.jdt.internal.ui.propertiesfileeditor.PropertyKeyHyperlinkDetector.detectHyperlinks(PropertyKeyHyperlinkDetector.java:90)
	at org.eclipse.ui.texteditor.HyperlinkDetectorRegistry$HyperlinkDetectorDelegate.detectHyperlinks(HyperlinkDetectorRegistry.java:80)
	at org.eclipse.jface.text.hyperlink.HyperlinkManager.findHyperlinks(HyperlinkManager.java:286)
	at org.eclipse.jface.text.hyperlink.HyperlinkManager.findHyperlinks(HyperlinkManager.java:258)
	at org.eclipse.jface.text.hyperlink.HyperlinkManager.mouseMove(HyperlinkManager.java:462)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:205)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1062)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4084)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3675)
Comment 1 Deepak Azad CLA 2010-12-15 13:26:11 EST 
The call to Properties#load() in PropertyKeyHyperlinkDetector.detectHyperlinks(...) looks unnecessary to me. If the key in question is invalid, PropertyKeyHyperlinkDetector.getUnicodeString(...) would also complain. There is no need to check the whole file by calling Properties#load().
Comment 2 Dani Megert CLA 2010-12-16 07:15:45 EST 
(In reply to comment #1)
> The call to Properties#load() in
> PropertyKeyHyperlinkDetector.detectHyperlinks(...) looks unnecessary to me. If
> the key in question is invalid,
> PropertyKeyHyperlinkDetector.getUnicodeString(...) would also complain. There
> is no need to check the whole file by calling Properties#load().

We need the check to be 100% sure that the key is a real key in the properties list. Also note, that you won't be able to use that file as it will also fail to load when it tries to resolve one of the keys. We should write an error to the editor status line (see PropertyKeyHyperlink.showErrorInStatusLine(String)) and don't write to the .log.
Comment 3 Deepak Azad CLA 2010-12-16 08:16:19 EST 
(In reply to comment #2)
> We need the check to be 100% sure that the key is a real key in the properties
> list. 
Ok... is there a scenario when the key is not a real key ? Honestly, I can not think of one, and hence think that the check is unnecessary. Plus we would save some cycles if we do not load the whole file.

> Also note, that you won't be able to use that file as it will also fail
> to load when it tries to resolve one of the keys. We should write an error to
> the editor status line (see PropertyKeyHyperlink.showErrorInStatusLine(String))
> and don't write to the .log.
Yeah.. that's there. The right thing to do is to mark it as an error in the editor (Bug 328374) and let the user know that about it even if he does not try to invoke a hyperlink. Also I see no point in preventing a user from opening a hyperlink just because there are some errors in the file - we do not do that in the Java editor as well.

Having said that, since currently we do not mark it as an error, I was thinking of leaving the check as it is *for now*.
Comment 4 Dani Megert CLA 2010-12-16 08:28:24 EST 
> Ok... is there a scenario when the key is not a real key ? Honestly, I can not
> think of one, and hence think that the check is unnecessary. Plus we would save
> some cycles if we do not load the whole file.
It's the other way around: if you want to remove the code you need to proof that the key is 100% valid ;-)

> 
> > Also note, that you won't be able to use that file as it will also fail
> > to load when it tries to resolve one of the keys. We should write an error to
> > the editor status line (see PropertyKeyHyperlink.showErrorInStatusLine(String))
> > and don't write to the .log.
> Yeah.. that's there. The right thing to do is to mark it as an error in the
> editor (Bug 328374) and let the user know that about it even if he does not try
> to invoke a hyperlink. 
As you just said: it's another bug.

>Also I see no point in preventing a user from opening a
> hyperlink just because there are some errors in the file - we do not do that 
>in
> the Java editor as well.
The hover also fails (see bug 328601) and the file is completely unusable with the (unseen) error.

> Having said that, since currently we do not mark it as an error, I was thinking
> of leaving the check as it is *for now*.
Just write it to the status bar as we currently have other things to do than working on bug 328374. This would then also be consistent with the fix for the hover problem where we notify the user about the broken properties file.
Comment 5 Deepak Azad CLA 2010-12-16 08:37:18 EST 
(In reply to comment #4)
> Just write it to the status bar as we currently have other things to do than
> working on bug 328374. This would then also be consistent with the fix for the
> hover problem where we notify the user about the broken properties file.
Fair enough.
Comment 6 Deepak Azad CLA 2011-02-13 06:42:06 EST 
(In reply to comment #2)
> We should write an error to
> the editor status line (see PropertyKeyHyperlink.showErrorInStatusLine(String))
> and don't write to the .log.

Fixed in HEAD in PropertyKeyHyperlinkDetector.java
Comment 7 Markus Keller CLA 2011-02-14 05:48:24 EST 
> Fixed in HEAD in PropertyKeyHyperlinkDetector.java

Why do we need the field fTextEditor? Can't you just pass the ITextEditor to showErrorInStatusLine? Avoiding state is always a plus.
Comment 8 Deepak Azad CLA 2011-02-14 06:47:46 EST 
(In reply to comment #7)
> > Fixed in HEAD in PropertyKeyHyperlinkDetector.java
> 
> Why do we need the field fTextEditor? Can't you just pass the ITextEditor to
> showErrorInStatusLine? Avoiding state is always a plus.
Ok, fixed in HEAD.