#1604 Memory leak when adding a frame to a page by JS

[Joerg Werner]

When adding a frame to a page by some JS code, HtmlUnit may leak memory. See the attached test case to reproduce the issue. The test case executes a loop with these steps:

1. Opens a fresh web client.
2. Loads a page (which has the JS code to add the frame).
3. Closes the web client.

A short heap dump analysis revealed that the thread local holding the postponed actions in JavaScriptEngine appears to grow over time, ie. contains entries for more and more threads. The only postponed action stored per thread is the one created in BaseFrameElement, but it (indirectly) references the web client and therefore hogs a good share of memory.

Could you please have a look and check why the postponed action is not executed? And what do you think about always clearing/removing the thread locals for the current JS thread when the web client quits? Just to be on the safe side ...

Thanks!

[Joerg Werner]

To be more precise: The thread local map for the current thread, the main thread, gets bigger over time, as more and more ThreadLocal instances are added to it, a couple of them for each new web client instance. So it is indeed necessary to remove all these ThreadLocal instances from the map again when the web client shuts down.

[Ahmed Ashour]

Thanks for reporting, the below change was done, please provide your feedback:

JavaScriptEngine: removes all postponed actions on webClient.closeAllWindows();

[Joerg Werner]

Thanks, Ahmed. I had a quick look at your changes. Some questions come to mind:

• Why is there a postponed action left at all? Shouldn't it have been executed properly?
• The "javaScriptRunning_" thread local should probably be removed within an own if-branch, independently of the presence of "postponedActions_".
• There are more ThreadLocals in the code, in XPathUtils and in History. While XPathUtils looks pretty safe (the ThreadLocal is static), we might leaking memory in History. I know it is just a map entry with a Boolean object, but it might add up over time ...

[Ahmed Ashour]

• I guess: the postponed actios are tehere becuase mostly there is a background process continuously adding new ones
• Ronald has just done done
• com.gargoylesoftware.htmlunit.History: ThreadLocal is with a Boolean value, not a map.

[Joerg Werner]

Regarding the left-over postponed action: in the test case no background JavaScript is involved, so the reason must be something else. When looking at the code, it seems that loading a frame in onload will be done as a postponed action. When it is time to execute the postponed actions, the current list of actions is detached from the thread-local and processed. So far, so good. The problem is that the load-the-frame postponed action appears to create another postponed action, which is enqueued in a new list at the thread-local. This action is the one that will not be processed and is left over.

Regarding the thread-local in History: to avoid an (admittedly slight) memory leak, it should be removed as well, probably when the window to which the history belongs is closed.

[Joerg Werner]

Since this issue is marked as closed already, I wanted to raise your attention to a specific detail that is still open. I am not quite sure if the handling of postponed actions is correct in all cases. It appears that postponed actions created by postponed actions are not executed (or executed only when processing postponed actions the next time). Could you please have a look and comment? Thanks.