749448 - Remove XTF

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

As far as I know this is unused. The xpcshell test for it has memory leaks.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This was never used by Gecko core, just extensions.  Does xforms still use it?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This blog post suggests not:
http://philipp.wagner.name/blog/2011/07/the-future-of-mozilla-xforms/

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 (Exited; not receiving bugmail, old account, do not use)]

Why does it matter if XTF leaks?

Comment 5

[Alex Vincent [:WeirdAl]]

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 [inactive unless it's an emergency] (:billm)]

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 [inactive unless it's an emergency] (:billm)]

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][[e-mail]]]

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][[e-mail]]]

Or, at least this should be reviewed by aaronr and surkov and philipp

Comment 10

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

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 [inactive unless it's an emergency] (:billm)]

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][[e-mail]]]

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)]

(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 [:bzbarsky]]

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][[e-mail]]]

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]]

I'm also in favor of removing this.

Comment 17

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

Yeah, I think we can remove XTF (and XMLEvents). ESR17 will still have XTF for some time.

Comment 18

[Philipp Wagner [:imphil]]

(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][[e-mail]]]

Attachment: WIP

Updated billm's patch. Compiling....

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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][[e-mail]]]

ted>	no, that doesn't matter
ted>	we only ship one combined .xpt file

Comment 22

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

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 [:bzbarsky]]

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][[e-mail]]]

Attachment: up-to-date

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/df2f15a17798

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

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][[e-mail]]]

Any information about FF version? 3.6 perhaps?

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

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]]

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.