Last Comment Bug 749448 - Remove XTF
: Remove XTF
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla19
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 464909 605871 811832 812375
  Show dependency treegraph
 
Reported: 2012-04-26 16:39 PDT by Bill McCloskey (:billm)
Modified: 2013-09-14 11:02 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (143.95 KB, patch)
2012-04-26 16:39 PDT, Bill McCloskey (:billm)
bugs: review-
Details | Diff | Splinter Review
WIP (114.35 KB, patch)
2012-11-14 07:16 PST, Olli Pettay [:smaug]
bzbarsky: review+
Details | Diff | Splinter Review
up-to-date (114.34 KB, patch)
2012-11-15 04:27 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-04-26 16:39:12 PDT
Created attachment 618852 [details] [diff] [review]
patch

As far as I know this is unused. The xpcshell test for it has memory leaks.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-04-26 16:50:07 PDT
This was never used by Gecko core, just extensions.  Does xforms still use it?
Comment 2 Bill McCloskey (:billm) 2012-04-26 16:53:11 PDT
This blog post suggests not:
http://philipp.wagner.name/blog/2011/07/the-future-of-mozilla-xforms/
Comment 3 Bill McCloskey (:billm) 2012-04-26 16:58:39 PDT
Also, I should note that the alternative to removing XTF is to patch the cycle collector code that traces through JS objects to handle a case that only XTF uses. This is a fairly hot path, so we'd be slowing down the common case to make XTF not leak.
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-26 16:59:10 PDT
Why does it matter if XTF leaks?
Comment 5 Alex Vincent [:WeirdAl] 2012-04-26 17:00:29 PDT
I used to use it in JS land, but it was never really stable enough and didn't quite meet my needs.  +1 for killing this off.

I did have someone ask for a copy of my XTF JS-based library a couple months ago...
Comment 6 Bill McCloskey (:billm) 2012-04-26 17:01:34 PDT
Rafael and I spent about two days tracking down this leak in the hope that it was related to the incremental GC leak. If we leave this in, someone else is liable to waste more time on it.
Comment 7 Bill McCloskey (:billm) 2012-04-26 17:04:30 PDT
Also, it would be nice if we check for leaks in xpcshell tests. Right now they're just ignored. That seems bad to me.
Comment 8 Olli Pettay [:smaug] 2012-04-26 17:12:26 PDT
Comment on attachment 618852 [details] [diff] [review]
patch

XTF is still used by extensions.
We could just fix XTF's classinfo.
I can do that
Comment 9 Olli Pettay [:smaug] 2012-04-26 17:13:41 PDT
Or, at least this should be reviewed by aaronr and surkov and philipp
Comment 10 Olli Pettay [:smaug] 2012-04-26 17:15:26 PDT
But still, I think fixing the leak should be trivial.
Better to do that first, and remove XTF after telling everyone that it is being
removed.
XForms isn't the only addon which has used XTF.
Comment 11 Bill McCloskey (:billm) 2012-04-26 17:30:20 PDT
OK. Are you going to somehow make it so we don't use a JS object for classinfo, or have the cycle collector traverse through the classinfo?
Comment 12 Olli Pettay [:smaug] 2012-04-27 03:19:45 PDT
I was thinking to create a new class for the classinfo and have
a weak pointer from classinfo to the element class and vise versa and manually disconnect them
from each other when needed.
Comment 13 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-04-27 05:21:51 PDT
(In reply to Bill McCloskey (:billm) from comment #6)
> Rafael and I spent about two days tracking down this leak in the hope that
> it was related to the incremental GC leak. If we leave this in, someone else
> is liable to waste more time on it.

This also blocks enabling useful assertions that some global objects are not leaked.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-11-13 17:59:41 PST
smaug, what extensions other than XForms use this?  It's being an annoyance as we're changing DOM code... and forcing some things to be virtual that can be non-virtual otherwise.
Comment 15 Olli Pettay [:smaug] 2012-11-13 18:02:15 PST
It was used also by X+V, but that is long gone. And I think now XForms has finally died.
Comment 16 Peter Van der Beken [:peterv] 2012-11-14 01:45:57 PST
I'm also in favor of removing this.
Comment 17 Olli Pettay [:smaug] 2012-11-14 02:28:28 PST
Yeah, I think we can remove XTF (and XMLEvents). ESR17 will still have XTF for some time.
Comment 18 Philipp Wagner [:imphil] 2012-11-14 07:07:36 PST
(In reply to Olli Pettay [:smaug] from comment #15)
> And I think now XForms has finally died.

Yes, I would agree with you on that. Let's remove XTF if it helps.
Comment 19 Olli Pettay [:smaug] 2012-11-14 07:16:38 PST
Created attachment 681485 [details] [diff] [review]
WIP

Updated billm's patch. Compiling....
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2012-11-14 07:46:46 PST
Comment on attachment 681485 [details] [diff] [review]
WIP

Review of attachment 681485 [details] [diff] [review]:
-----------------------------------------------------------------

Do we need to add the xpt file to some removed-files file?
Comment 21 Olli Pettay [:smaug] 2012-11-14 07:50:43 PST
ted>	no, that doesn't matter
ted>	we only ship one combined .xpt file
Comment 22 Olli Pettay [:smaug] 2012-11-14 07:52:56 PST
Comment on attachment 681485 [details] [diff] [review]
WIP

Compiles and runs locally. Will push to try with the XML events removal
patch.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2012-11-14 11:37:48 PST
Comment on attachment 681485 [details] [diff] [review]
WIP

r=me, but I believe you can now make nsGenericElement::HasAttribute/GetAttribute/RemoveAttribute non-virtual.  Followup patch for that is fine.
Comment 24 Olli Pettay [:smaug] 2012-11-15 04:27:33 PST
Created attachment 681943 [details] [diff] [review]
up-to-date
Comment 25 Olli Pettay [:smaug] 2012-11-15 05:28:41 PST
https://hg.mozilla.org/mozilla-central/rev/df2f15a17798
Comment 26 Lawrence Mandel [:lmandel] (use needinfo) 2012-11-15 17:29:46 PST
FWIW, I pulled add-on ping data for XForms from the past 7 days. Here is the XForms install base for active Firefox instances that were able to submit a ping over that period (note that this data says nothing about whether the add-on is enabled):

Nov 14: 2278
Nov 13: 2302
Nov 12: 2224
Nov 11: 1758
Nov 10: 1300
Nov 9 : 1980
Nov 8 : 2338
Nov 7 : 2413
Comment 27 Olli Pettay [:smaug] 2012-11-15 17:32:24 PST
Any information about FF version? 3.6 perhaps?
Comment 28 Lawrence Mandel [:lmandel] (use needinfo) 2012-11-15 17:55:35 PST
Here's the breakdown of install by FF version for 20121114:

16.0.2	1656
12.0	118
15.0.1	103
16.0.1	75
14.0.1	56
10.0.10	29
13.0.1	29
11.0	22
15.0	19
10.0.2	16
17.0	16
4.0.1	14
9.0.1	12
10.0	11
7.0.1	11
8.0.1	10
10.0.6	8
16.0	7
10.0.7	6
8.0	5
10.0.1	5
10.0.4	5
13.0	5
6.0.2	4
5.0	4
6.0	4
10.0.5	3
10.0.9	3
10.0.8	3
18.0a2	3
4.0	3
10.0.3	2
5.0.1	2
17.0a2	2
18.0a1	2
19.0a1	1
15.2.1-x64	1
4.0b12	1
6.0.1	1
17.0a1	1
Comment 29 Philipp Wagner [:imphil] 2012-11-16 00:04:30 PST
The XForms extension only ever worked up to Firefox 7 IIRC, and already the 4 version was quite crashy. But those latest code changes were never released.
The last version published on AMO was supporting up to Firefox 3.6, the last version I released as preview on my homepage was for 4.0. 
So we can be pretty sure that that install data is meaningless, as the extension will almost certainly be disabled for everything above Firefox 4.0.

Note You need to log in before you can comment on or make changes to this bug.