336791 - land leak fixes making wrapped JS closures work with nsIDOMGCParticipant on 1.8 branch

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is a tracking bug for landing the collection of leak fixes (and regression fixes following up on them) related to nsIDOMEventListener and XMLHttpRequest leaks on the 1.8 branch.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: merged patch

This is the necessary patches merged to the branch (except for bug 324865, which I landed separately this afternoon).

The next step will be reducing the interface changes; this is simply what landed on the trunk merged to the branch.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch with only interface change being nsIContent

This patch has all the interface changes taken out (at a 1-word cost for a bunch of objects, most significantly nsImageLoadingContent) except for the changes to nsIContent.  (Taking those out would be pretty trivial, but it's a 1-word cost for all elements.)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: interdiff showing changes made to preserve API compatibility

This is an interdiff showing the changes I made to the merge result in order to preserve API compatibility.  It excludes some fixes to incorrect merging shown in attachment 225107 [details] [diff] [review] (conflicts in nsHTMLSharedObjectElement.cpp and a missed piece that needed to be merged into nsGenericDOMDataNode.cpp as well).  Note when reading these interdiffs that a bunch of the patch segments begin with "reverted:", which I presume means that all of the changes in the file in the first patch are not present in the second.

My notes on the merging are as follows (fixed up since previous attachment):

Bugs merged:
bug 241518
bug 319293
bug 319642
bug 322985
bug 323807
bug 323534
bug 325279
bug 326646
bug 339412
bug 324865 (in separate tree; already landed)
bug 206520
bug 321054
bug 339488

I could fix nsDOMAttribute for branch/trunk differences, but having too much reachability shouldn't be a problem, so I may as well just leave it matching the trunk even though nsDOMAttribute ownership is (IIRC) different on the branch.

I could remove nsIDOMGCParticipant from nsIContent to preserve nsIContent API compatibility too, at a 1-word cost for each object.  If so, I'd need to deal with both nsGenericElement and nsGenericDOMDataNode.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_8_BRANCH.

Comment 5

[Ivan Khlistov]

Is it possible not to modify nsIContent?
> Checked in to MOZILLA_1_8_BRANCH.
> 

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> Is it possible not to modify nsIContent?

Not without increasing the size of all content nodes by 4 bytes or something a good bit more complicated.  This was discussed a good bit on mozilla-dev-planning before I landed.

Comment 7

[Ivan Khlistov]

So make it by increasing size of all content nodes by 4 bytes (no interface change) in mozilla 1_8 branch. 
And make it by the right way (changing interface) in mozilla 1_9 branch.
Because FF 1.5 and FF 2.0 APIs must be compatible as maximum as possible.

For me, i dont' wish to rebuild my extension dll because just one mozilla's interface has being changed.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Dbaron: Would it make sense to add this interface as a tearoff and make sure to have a pretty generous cache for the tearoffs? I guess you'd have to make separate tearoffs for nsGenericElement, nsDocument and nsGenericDOMDataNode, which would kind'a suck (and cost some time).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No point having a tearoff from nsDocument, and no point having a tearoff if there isn't a rarely-allocated slots object in which the pointer to it can be stored, but that would work.

I don't plan to work on it, though.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

On elements we do have a rarly allocated slots object where we can stick a pointer (nsGenericElement::nsDOMSlots), but i'm not sure if sticking the pointer in there will cause us to create lots and lots more of those slots? I.e. how often we QI to  nsIGCParticipant

Comment 11

[chris hofmann]

could this change be related to problems in https://bugzilla.mozilla.org/show_bug.cgi?id=344699