Filter large tables with tablefilter hangs since v45.0

RESOLVED WORKSFORME

Status

()

Core
DOM: Core & HTML
RESOLVED WORKSFORME
2 years ago
2 years ago

People

(Reporter: Marcel, Unassigned)

Tracking

({hang, perf, regression})

45 Branch
hang, perf, regression
Points:
---

Firefox Tracking Flags

(firefox45 unaffected, firefox46 unaffected, firefox47 unaffected, firefox48 unaffected, firefox-esr45 affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8735266 [details]
bug-report.tar.gz

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160304114936

Steps to reproduce:

1. Open the HTML file
2. type in the second box "TEST" (for example)


Actual results:

Firefox reports that the script is hanging. 
Affected versions are: 
45.0 a1 (build from 2015-11-02)
45.0.0
45.0.1
48.0 a1 (2016-03-25)


Expected results:

The script should have filtered the rows and show an empty table.
Works fine with:
38.7.1
44.0.2
45.0 a1 (build from 2015-11-01)

Comment 1

2 years ago
Created attachment 8735289 [details]
testcase.html (self-contained testcase)

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=798c82c8a96d1099cf712434d9c9036fc48d8297&tochange=9160f08660b8290559e427fd80d617edd86fe2a6

Suspected: bug 264412.

Comment 2

2 years ago
Mats, as roc is away, could you find someone in charge of this part of code, please.
Blocks: 264412
Status: UNCONFIRMED → NEW
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
tracking-firefox-esr45: --- → ?
Component: General → DOM: Core & HTML
Ever confirmed: true
Flags: needinfo?(mats)
Keywords: hang, perf, regression
OS: Unspecified → All
Hardware: Unspecified → All
Here's the problematic script:

function tf_GetNodeText(n)
/*====================================================
	- returns text + text of child nodes of a node
=====================================================*/
{
	var s = n.textContent || n.innerText || n.innerHTML.replace(/\<[^<>]+>/g, '');
	s = s.replace(/^\s+/, '').replace(/\s+$/, '')/*.tf_Trim()*/;
	return s.tf_Trim();
}

So when n.textContent returns "" we will evaluate n.innerText which in this case
also returns "" so we will evaluate n.innerHTML too.  This function is called
once for each <td> it seems, about half of which are empty.

Apart from that, n.innerText flushes layout(*) so if there are DOM / style changes
between the calls to tf_GetNodeText this will be very costly.  And this is what
causes the performance problem in this case.

(*) this is documented here:
https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent

So, I think this bug is invalid.  The workaround to the .innerText flush
problem is to set display:none on the subtree while it's being modified.
In this case I'm guessing it's the <table> element. Like so:
tableElement.style.display="none"
// sort the table, i.e. all the tf_GetNodeText calls happens here
tableElement.style.display=""
tracking-firefox46: ? → ---
tracking-firefox47: ? → ---
tracking-firefox48: ? → ---
tracking-firefox-esr45: ? → ---
Flags: needinfo?(mats)

Comment 4

2 years ago
.innerText is called in tablefilter_all.js, so is it a bug in this JS lib?
Yes.

Comment 6

2 years ago
Is it possible for you to file a bug report as you know the subject better than me?
https://github.com/koalyptus/TableFilter/issues
I'll leave that for someone else.  Comment 3 have all the info needed about the problem.

Comment 8

2 years ago
done https://github.com/koalyptus/TableFilter/issues/162
(Reporter)

Comment 9

2 years ago
The issue was fixed by the JS lib by not executing .innerText if .textContent is supported.
Thanks for the fast response! Closing this bug as it works with the current version now.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
status-firefox45: affected → unaffected
status-firefox46: affected → unaffected
status-firefox47: affected → unaffected
status-firefox48: affected → unaffected
You need to log in before you can comment on or make changes to this bug.