CellTable Memory Leak in IE8

250 views
Skip to first unread message

Harvard Pan

unread,
Jul 23, 2015, 5:05:10 PM7/23/15
to Google Web Toolkit
Hello,

Our company uses GWT 2.5.1-rc1 and many of our customers (healthcare) use IE8. We were hopeful that the memory leak in CellTable would have been addressed by the memory leak fix for FlexTable. That leak (Issue 6938 - https://code.google.com/p/google-web-toolkit/issues/detail?id=6938) was fixed in 2.6. After grabbing the fix and merging it into the 2.5.1-rc1 code, we can confirm that FlexTable indeed is fixed and no longer leaking. However, we still have leaking resources in CellTable. I've written a small sample application to demonstrate the code that leaks. It's available on BitBucket for anyone to pull.
The root cause of 6938 was described in a Microsoft Connect page: http://connect.microsoft.com/IE/feedback/details/790340/memory-leak-in-ie9-ie10-tables
In it, it describes multiple reasons for the leak, including:
  • rows with ids
  • cells with ids
  • code that references a row.cells expression, even if it does not store or use the result (that's the fix in 6938)
  • code that does not set row.innerHTML to null after invoking table.deleteRow() for the row.
I imagine that the CellTable leak is related to one of the conditions above. I suspect the last one as I never do actually see any setting to null of innerHTML in the javascript. Wanted to check in to see if anyone on this forum had any ideas on where we could investigate next.

Thanks!
Harvard

DaveC

unread,
Aug 4, 2015, 7:11:24 AM8/4/15
to Google Web Toolkit
Hi,

I think you'll have to dig into AbstractCellTable in order to fix this. I did a quick test of a fix I implemented and it "appears" to work for IE11 running IE8/IE9 mode but not IE10 mode, I've not tested it in a real IE8 or IE9.

Basically what I did was exaactly what you said - set row.innerHTML to null.

I took a copy of AbstractCellTable (including the package structure) and placed it in my test project. In the ImplTrident inner class I've tweaked the methods replaceTableSection and replaceAllRowsImplLegacy to set the innerHTML to null for each row e.g. 

    private native final void setRowInnerHtmlToNull(Element row)/*-{
    row.innerHTML = null;
    }-*/;

private void replaceTableSection.....

 TableElement tableElement = table.getElement().cast();
      
      Element child = section.getFirstChildElement();
      while (child != null) {
        setRowInnerHtmlToNull(child);
        child = child.getNextSiblingElement();
      }
      
      tableElement.replaceChild(newSection, section);

.....}

 protected void replaceAllRowsImplLegacy(AbstractCellTable<?> table, TableSectionElement section,
        SafeHtml html) {
      // Remove all children.
      Element child = section.getFirstChildElement();
      while (child != null) {
        Element next = child.getNextSiblingElement();
        section.removeChild(child);
        setRowInnerHtmlToNull(child);
        child = next;
      }

....}

Hope this helps.

Cheers,
Dave

Harvard Pan

unread,
Aug 5, 2015, 8:24:27 AM8/5/15
to Google Web Toolkit
Dave,

Thank you SO much for this. I tested this using my test project and it seems to have solved the memory leak completely! I've attached my version of the file (modified from 2.5.1-rc1 branch) with your changes.

What's the best way to get this fix to the GWT code base so that future versions of GWT will have this fix as well? I'd previously logged Issue #9164 (https://github.com/gwtproject/gwt/issues/9164) on the gwtproject page.

Thanks again!
Harvard
AbstractCellTable.java

DaveC

unread,
Aug 5, 2015, 9:07:42 AM8/5/15
to Google Web Toolkit
No worries, glad it worked! ;)

I've not personally committed any code to the core gwt project but you can find out more here http://www.gwtproject.org/makinggwtbetter.html#contributingcode

Bear in mind that GWT 3.0 is going to be a very different animal (for instance, from what I understand there will be no widgets or gwt-rpc...), so perhaps there isn't such a need to get these things fixed... I might be wrong though.

Cheers,
Dave

Harvard Pan

unread,
Aug 12, 2015, 11:30:29 PM8/12/15
to google-we...@googlegroups.com
Turns out that my test program no longer leaked because the rows in the test program's CellTable were no longer being replaced. After I changed the test program to output different data for each iteration, it became clear that the rows in the table just never changed. I tracked down the problem to setting the innerHTML for each row before the call to tableElement.replaceChild(newSection, section). Once I moved the setting of innerHTML to null to after this call, the table updates properly, but the leak still remains.

Upon further testing, the memory leak seems to be largely from the replaceChild call. I guess if the entire TableSection is replaced in the DOM, IE8 doesn't properly clean this up, even if you set the innerHTML to null for the individual rows. I changed the implementation of replaceTableSection so that it ends up calling replaceAllRowsImplLegacy (which replaces one row at a time) and this yielded the correct result of updating the table while also not leaking memory.

Here's the final code that I ended up with in ImplTrident:
    private native final void setRowInnerHtmlToNull(TableRowElement row) /*-{
      row.innerHTML = null;
    }-*/;
      
    /**
     * This method is used for legacy AbstractCellTable that's not a
     * {@link TableSectionChangeHandler}.
     */
    protected void replaceAllRowsImplLegacy(AbstractCellTable<?> table, TableSectionElement section,
        SafeHtml html) {
      // Remove all children.
      Element child = section.getFirstChildElement();
      while (child != null) {
        Element next = child.getNextSiblingElement();
        section.removeChild(child);
        // GWT Issue #9164 - https://github.com/gwtproject/gwt/issues/9164
        if (TableRowElement.is(child)) {
            setRowInnerHtmlToNull(TableRowElement.as(child));
        }
        child = next;
      }

      // Add new child elements.
      TableSectionElement newSection = convertToSectionElement(table, section.getTagName(), html);
      child = newSection.getFirstChildElement();
      while (child != null) {
        Element next = child.getNextSiblingElement();
        section.appendChild(child);
        child = next;
      }
    }
    
    /**
     * Render html into a table section. This is achieved by first setting the html in a DIV
     * element, and then swap the table section with the corresponding element in the DIV. This
     * method is used in IE since the normal optimizations are not feasible.
     * 
     * @param table the {@link AbstractCellTable}
     * @param section the {@link TableSectionElement} to replace
     * @param html the html of a table section element containing the rows
     */
    private void replaceTableSection(AbstractCellTable<?> table, TableSectionElement section,
        SafeHtml html) {
      String sectionName = section.getTagName().toLowerCase();
      replaceAllRowsImplLegacy(table, section, html);
      if ("tbody".equals(sectionName)) {
        ((TableSectionChangeHandler) table).onTableBodyChange(section);
      } else if ("thead".equals(sectionName)) {
        ((TableSectionChangeHandler) table).onTableHeadChange(section);
      } else if ("tfoot".equals(sectionName)) {
        ((TableSectionChangeHandler) table).onTableFootChange(section);
      }
    }

Again, many thanks for all your help. We couldn't have solved this otherwise.

Harvard


--
You received this message because you are subscribed to a topic in the Google Groups "Google Web Toolkit" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/google-web-toolkit/lNOKG2dgAzs/unsubscribe.
To unsubscribe from this group and all its topics, send an email to google-web-tool...@googlegroups.com.
To post to this group, send email to google-we...@googlegroups.com.
Visit this group at http://groups.google.com/group/google-web-toolkit.
For more options, visit https://groups.google.com/d/optout.

Dave Chapman

unread,
Aug 13, 2015, 8:28:13 AM8/13/15
to google-we...@googlegroups.com
Hi Harvard,

Thanks for the update, I did think it was "lucky" my quick hack worked, but glad I could help (in a small way!).

Cheers,
Dave
Reply all
Reply to author
Forward
0 new messages