312342 - xf:repeat is leaking

Status: RESOLVED WONTFIX

(Core Graveyard :: XForms, defect)

Description

[Aurelian Penciu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

If a model instance is replaced with external data due to application context
changes and xforms:repeat is used to display data the Firefox memory usage
increases each time the instance data is replaced. If repeat is not used there
still seems to be a drift but it's almost negligeable.

Unfortunately memory is not released when I navigate out of the XForms page so
heavy usage of such a page over a few hours requires restarting the browser.

Reproducible: Always

Steps to Reproduce:
I will provide a sample attachment.
1.Define a model with a request instance and an external response instance.
2.Define a submission that sends the request instance data and replaces the
response instance with the result. 
3.Add a trigger that "sends" the submission.
4.Use a xf:repeat element to iterate through the response data and display a
simple element or attribute for each item.
5. Keep clicking on the toggle and check the Firefox memory usage numbers.
6. Go to a different URL and check ifthe memory usage decreases.

Actual Results:  
Results will be provided as an attachment. I used 54045 bytes of XML instance
data. The initial memory usage figure for Mozilla was roughly 21MB. Repeating
the test about 80 times I ended up with 50MB. If I removed the repeat element
the final number was 22MB. The average increase per step was 360KB. Displaying a
different URL did not "clear" the numbers as expected.

Expected Results:  
Keep a "flat" memory profile. I think the behavior of the same test if the
repeat was removed can be deemed acceptable but a plus of 360KB per refresh is
rather steep.

I also noticed that if I increase the XML size over 64K the submission fails.

Comment 1

[Aurelian Penciu]

Attachment: testcase

I tested this WAR on Tomcat 5.5 with J2SE 1.5.

The URL for the test page is:
http://localhost:8080/XFIssues/xmlSizeLimit.xhtml.

The servlet generates a random "schedule" and the page retrieves 4 consecutive
days or roughly under 64KB of XML data. Adding another day and increasing the
size  over 64KB caused submission errors.

Comment 2

[Aurelian Penciu]

Attachment: Memory usage numbers.

Comment 3

[Allan Beaufour]

(In reply to comment #1)
> Created an attachment (id=199432) [edit]
> testcase
> 
> I tested this WAR on Tomcat 5.5 with J2SE 1.5.
> 
> The URL for the test page is:
> http://localhost:8080/XFIssues/xmlSizeLimit.xhtml.

Not the simplest test in the world :) And not really easy to confirm when you do
not supply XFSchMgr...

I'd rather see a pure XForms file and a pure XML instance.

Comment 4

[Allan Beaufour]

Repeat is definately leaking. It is not tied to submission. Reloading the same
page (at least with an external instance), will keep the memory usage increasing.

Comment 5

[Aurelian Penciu]

Attachment: Simpler testcase

Sorry... I'm too stuck in my servlet world :(. Here is a simplified XForms file
and some instance data. It was set to run on C:/XFIssues. Also provided the
larger (bad!) instance data to ilustrate the XML size limit I was suspecting.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> Created an attachment (id=199551) [edit]
> Simpler testcase
> 

Even simpler testcase would be great.

Comment 7

[Aurelian Penciu]

Attachment: Same testcase; XML data in file (no servlet)

Provied test data as standalone XML file. Roughly the same size as the previous test to produce similar memory usage increase.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Fix for the warnings

This doesn't fix the leak(s?),
only the warnings:
WARNING: NS_ENSURE_TRUE(doc) failed, file nsXFormsUtils.cpp, line 396

The problem is that in some controls the mBoundNode points to 
the old instance data while binding other controls. And because the old 
document has gone, the nodes don't have ownerDocument.

The patch has also some coding style fixes.

Comment 9

[Allan Beaufour]

Comment on attachment 202829 [details] [diff] [review]
Fix for the warnings

>+nsXFormsControlStubBase::Reset()
>+{
>+  RemoveIndexListeners();
>+
>+  if (mModel) {
>+    mModel->RemoveFormControl(this);
>+    mModel = nsnull;
>+  }
>+  mBoundNode = nsnull;
>+  mModel = nsnull;

Just in case it did not understand the nsnull the first time? ;-)

>@@ -634,41 +638,51 @@ nsXFormsModelElement::Rebuild()
>     NS_ENSURE_TRUE(oldFormList, NS_ERROR_OUT_OF_MEMORY);
>     *oldFormList = mFormControls;
>   
>     // Clear out mFormControls so that we can rebuild the list.  We'll go control 
>     // by control over the old list and rebind the controls.
>     mFormControls.Clear(); // if this happens on a documentchange
> 
>     PRInt32 controlCount = oldFormList->Count();
>+
>     for (PRInt32 i = 0; i < controlCount; ++i) {
>-      nsIXFormsControl* control = NS_STATIC_CAST(nsIXFormsControl*, 
>+      nsIXFormsControl* control = NS_STATIC_CAST(nsIXFormsControl*,
>+                                                 (*oldFormList)[i]);
>+      if (control) {
>+        control->Reset();
>+      }
>+    }
>+
>+    for (PRInt32 i = 0; i < controlCount; ++i) {
>+      nsIXFormsControl* control = NS_STATIC_CAST(nsIXFormsControl*,
>                                                  (*oldFormList)[i]);
>       /// @todo If a control is removed because of previous control has been
>       /// refreshed, we do, obviously, not need to refresh it. So mFormControls
>       /// should have weak bindings to the controls I guess? (XXX)
>       ///
>       /// This could happen for \<repeatitem\>s for example.
>       if (!control) {
>         continue;
>       }
> 
>       // run bind to reset mBoundNode for all of these controls and also, in the
>       // process, they will be added to the model that they should be bound to.
>       control->Bind();
>     }

What's the rationale behind this double-run?

Comment 10

[Allan Beaufour]

I'll go leak-hunting in general, it's not only repeat I can tell you.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 202829 [details] [diff] [review]
Fix for the warnings

Removing review request. We will fix the leaks and in the same time we should fix the problems with nodes which have null ownerDocument.

Comment 12

[Allan Beaufour]

It seems to me that bug 316569 is to blame for most non-repeat leaks. I haven't looked specifically on repeat yet, except for confirming that it sure leaks.

Comment 13

[Allan Beaufour]

The code has changed quite a bit since we looked at this. The whole RRR-routine have changed, we've changed repeat refresh and moved to XBL. Sombody needs to re-confirm/investigate this.

Comment 14

[Wayne Mery (:wsmwk)]

FWIW reporter is gone (email address bad)

Comment 15

[John L. Clark]

The test form needs to be modified slightly to use a relative reference in the submission, but after doing that I believe I can verify this bug using the following procedure.  Running `top`, I repeatedly press the "Get Data" button, and the "RES" column shows increasing memory usage, even after navigating away from, and then back to, the test form.  I believe (at least) this leak is affecting my users at the moment.  Time to dig in!

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

RIP xforms