Closed
Bug 749448
Opened 12 years ago
Closed 12 years ago
Remove XTF
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: billm, Assigned: smaug)
References
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(3 files)
143.95 KB,
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
114.35 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
114.34 KB,
patch
|
Details | Diff | Splinter Review |
As far as I know this is unused. The xpcshell test for it has memory leaks.
Attachment #618852 -
Flags: review?(bugs)
Comment 1•12 years ago
|
||
This was never used by Gecko core, just extensions. Does xforms still use it?
Reporter | ||
Comment 2•12 years ago
|
||
This blog post suggests not: http://philipp.wagner.name/blog/2011/07/the-future-of-mozilla-xforms/
Reporter | ||
Comment 3•12 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?
Comment 5•12 years ago
|
||
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•12 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•12 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•12 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•12 years ago
|
||
Or, at least this should be reviewed by aaronr and surkov and philipp
Assignee | ||
Comment 10•12 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•12 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•12 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.
Comment 14•12 years ago
|
||
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•12 years ago
|
||
It was used also by X+V, but that is long gone. And I think now XForms has finally died.
Comment 16•12 years ago
|
||
I'm also in favor of removing this.
Assignee | ||
Comment 17•12 years ago
|
||
Yeah, I think we can remove XTF (and XMLEvents). ESR17 will still have XTF for some time.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bugs
Comment 18•12 years ago
|
||
(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•12 years ago
|
||
Updated billm's patch. Compiling....
Comment 20•12 years ago
|
||
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•12 years ago
|
||
ted> no, that doesn't matter ted> we only ship one combined .xpt file
Assignee | ||
Comment 22•12 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 23•12 years ago
|
||
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 | ||
Comment 24•12 years ago
|
||
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/df2f15a17798
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: addon-compat,
dev-doc-needed
Resolution: --- → FIXED
Comment 26•12 years ago
|
||
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•12 years ago
|
||
Any information about FF version? 3.6 perhaps?
Comment 28•12 years ago
|
||
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•12 years ago
|
||
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.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•