codenameone / CodenameOne

Cross-platform framework for building truly native mobile apps with Java or Kotlin. Write Once Run Anywhere support for iOS, Android, Desktop & Web.
https://www.codenameone.com/
Other
1.7k stars 403 forks source link

crash painting text area #1730

Open ddyer0 opened 8 years ago

ddyer0 commented 8 years ago

This crash occurred on IOS, painting a text area. I'm pretty sure that in this case, getRowStrings was returning null as a legitimate consequence of initial setWidth or setText in the edt thread. It's just a matter of the exact timing of the initial repaint.

Something should be done to protect against this null exeption, but I'm unsure what. However, I am pretty sure this is not caused by out-of-edt activity.

[EDT] 0:0:0,0 - Exception: java.lang.NullPointerException - null java.lang.NullPointerException at com_codename1_ui_TextArea.getLines:745 at com_codename1_ui_plaf_DefaultLookAndFeel.drawTextArea:558 at com_codename1_ui_TextArea.paint:1056 at bridge_TextArea.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at bridge_Component.paint:230 at bridge_Container.paint:1896 at online_common_commonChatApplet.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at bridge_Component.paint:744 at bridge_ComponentProxy.actualPaint:42 at lib_ProxyWindow.paint:1178 at online_common_exCanvas.actualPaint:604 at online_common_RepaintManager.paint:230 at online_common_exCanvas.paint:1896 at bridge_ComponentProxy.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at bridge_Component.paint:1896 at bridge_Container.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at bridge_Component.paint:1896 at bridge_Container.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at bridge_Component.paint:1896 at bridge_Container.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at bridge_Component.paint:320 at bridge_Container.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at bridge_Component.paint:320 at bridge_Container.paint:549 at bridge_MasterPanel.paint:1708 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Container.paint:1313 at com_codename1_ui_Form.paint:3099 at bridge_MasterForm.paint:3124 at com_codename1_ui_Component.internalPaintImpl:1308 at com_codename1_ui_Form.internalPaintImpl:3109 at com_codename1_ui_Component.paintInternalImpl:1282 at com_codename1_ui_Component.paintInternal:1257 at com_codename1_ui_Component.paintInternal:1225 at com_codename1_ui_Component.paintComponent:1510 at com_codename1_ui_Component.paintComponent:1457 at com_codename1_impl_CodenameOneImplementation.paintDirty:527 at com_codename1_ui_Display.edtLoopImpl:1071 at com_codename1_ui_Display.mainEDTLoop:993 at com_codename1_ui_RunnableWrapper.run:120 at com_codename1_impl_CodenameOneThread.run:176 at java_lang_Thread.runImpl:153

shannah commented 8 years ago

Still most likely that there is an access off the EDT. The only place that sets rowStrings to null is here https://github.com/codenameone/CodenameOne/blob/master/CodenameOne/src/com/codename1/ui/TextArea.java#L401

I'd be inclined to do a test just before this line to check if it is off the EDT, and throw an exception in that case. That way it would at least fail fast and be easier to track down.

@codenameone What do you think?

ddyer0 commented 8 years ago

I don't do setWidth or setText outside of EDT, at least not directly; but they are done as part of window construction when the window is not yet attached to the hierarchy. So I don't think a check/exception can work without breaking a lot of things. I think this should be fixed by making the painting code more defensive. This was a one-time exception during construction, so I assume it was dependent on the exact timing of the initial repaint of the window.

shannah commented 8 years ago

If there were no accesses off the EDT this should never happen. Throwing an exception would at least reveal where it is happening so that you (or we) can fix the code to run on the EDT

ddyer0 commented 8 years ago

The code that generated this exception has no paths off-edt. My reading is that if a setWidth is excecuted on-edt, immediately before a paint is triggered, you can get to the exception.

shannah commented 8 years ago

No. getRowStrings() initializes the rowStrings property just before trying to use it. The only way I can see that rowStrings would end up null at this point in the code, is if setWidth() were called off the EDT.

ddyer0 commented 8 years ago

setting the line table to null in setWidth is a bad idea anyway. Better to set a flag saying the line table is invalid rather than using null as that flag.

ddyer0 commented 8 years ago

This is the relevant version of setWidth:

public void setWidth(int w) 
{ final int fw = w;
    G.runInEdt(new Runnable() { public void run() { actualSetWidth(fw); }});
}
public void actualSetWidth(int w) 
{ super.setWidth(w); 
}

my G.runInEdt calls run or runInEdt as appropriate.

shannah commented 8 years ago

Just don't do UI off the EDT. The more you try to kludge guards to accommodate off-EDT access, the harder it will be to actually find the cause of problems.

ddyer0 commented 8 years ago

Quoting stand on zanzibar "I tell you three times" I don't call setwidth off edt.