The default bug view has changed. See this FAQ.

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: billm, Assigned: smaug)

Tracking

({addon-compat, dev-doc-needed})

unspecified
mozilla19
x86_64
Linux
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 618852 [details] [diff] [review]
patch

As far as I know this is unused. The xpcshell test for it has memory leaks.
Attachment #618852 - Flags: review?(bugs)
This was never used by Gecko core, just extensions.  Does xforms still use it?
(Reporter)

Comment 2

5 years ago
This blog post suggests not:
http://philipp.wagner.name/blog/2011/07/the-future-of-mozilla-xforms/
(Reporter)

Comment 3

5 years ago
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.
Why does it matter if XTF leaks?
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...
(Reporter)

Comment 6

5 years ago
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.
(Reporter)

Comment 7

5 years ago
Also, it would be nice if we check for leaks in xpcshell tests. Right now they're just ignored. That seems bad to me.
(Assignee)

Comment 8

5 years ago
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
Attachment #618852 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

5 years ago
Or, at least this should be reviewed by aaronr and surkov and philipp
(Assignee)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
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?
(Assignee)

Comment 12

5 years ago
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.
(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.
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.
(Assignee)

Comment 15

4 years ago
It was used also by X+V, but that is long gone. And I think now XForms has finally died.
I'm also in favor of removing this.
(Assignee)

Comment 17

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

Updated

4 years ago
Assignee: nobody → bugs
(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.
(Assignee)

Comment 19

4 years ago
Created attachment 681485 [details] [diff] [review]
WIP

Updated billm's patch. Compiling....
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?
(Assignee)

Comment 21

4 years ago
ted>	no, that doesn't matter
ted>	we only ship one combined .xpt file
(Assignee)

Comment 22

4 years ago
Comment on attachment 681485 [details] [diff] [review]
WIP

Compiles and runs locally. Will push to try with the XML events removal
patch.
Attachment #681485 - Flags: review?(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.
Attachment #681485 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Blocks: 811832
(Assignee)

Comment 24

4 years ago
Created attachment 681943 [details] [diff] [review]
up-to-date
(Assignee)

Comment 25

4 years ago
https://hg.mozilla.org/mozilla-central/rev/df2f15a17798
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: addon-compat, dev-doc-needed
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 812375
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
(Assignee)

Comment 27

4 years ago
Any information about FF version? 3.6 perhaps?
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
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.
Target Milestone: --- → mozilla19
Blocks: 464909
Blocks: 605871
You need to log in before you can comment on or make changes to this bug.