767410 - Weaken WeakMethodClosure's reference to its MethodEnv

Status: RESOLVED FIXED

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Spawned off from Bug 500548, comment 9:

(In reply to Steven Johnson from Bug 500548, comment #2)
> Note that this differs from the one in Flash in a significant way: it only
> holds "savedThis" via WeakRef, but not the MethodEnv. I contend that this is
> an improvement, as the intent of WeakMethodClosure is to avoid holding the
> receiver in memory, and that the MethodEnv is irrelevant to our purposes.
> Since WeakRefs are cheap, but not free, we shouldn't use them when we don't
> need to, and I think we don't need to here. (Werner, I'm asking you for
> feedback as someone who knows the details of EventDispatcher better than I.)

I think this change has introduced a memory leak.

Steven's statement above makes it sound like he has removed the reference to the MethodEnv entirely from instances of WeakMethodClosure ("the MethodEnv is irrelevant to our purposes").

But there *is* still a reference to the MethodEnv: The one that is being passed up the super-class chain, and retained in the parent class of MethodEnv.

I believe the old WeakMethodClosure (the one that was not part of the tamarin) held the env weakly, while the new one holds it strongly.  Getting rid of the weak-ref in the name of blind removal of "cheap but not free" WeakRef support is wrong-headed: You need to verify that the referenced object will be retained in the usual-case in order to justify changing the weak-ref to a strong one.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: pdf F v1: Diagram of WeakMethodClosure prior to "fixing" Bug 500548

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: pdf G v1: Diagram of WeakMethodClosure post "fixing" Bug 500548

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: dot F' v1: graphviz source for pdf F

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: dot G' v1: graphviz source for pdf G

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch R v1: refactor, pull out common FunctionObjectBase class (work-in-progress)

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch S v1: reparent WeakMethodClosure to FunctionObjectBase (work-in-progress)

patch S is meant to be applied atop patch R.

patch S is the actual bug fix (as opposed to the mere preparatory refactoring in patch R).

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #5)
> Created attachment 636478 [details] [diff] [review]
> patch R v1: refactor, pull out common FunctionObjectBase class

(I think the introduction of FunctionObjectBase is without merit; IIUC adds no state or methods, and so I am going to try to remove it, which hopefully will simplify all of this.)

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #7)
> (In reply to Felix S Klock II from comment #5)
> > Created attachment 636478 [details] [diff] [review]
> > patch R v1: refactor, pull out common FunctionObjectBase class
> 
> (I think the introduction of FunctionObjectBase is without merit; IIUC adds
> no state or methods, and so I am going to try to remove it, which hopefully
> will simplify all of this.)

(oh wait, patch S did add some state to FunctionObjectBase.  So maybe it has merit after all.  Still working through my options.)

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch V r1: simpler refactor; add get_callEnv() virtual method

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch W v1: "replace" strong ref to callEnv with a weak one

This is meant to be applied atop patch V.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

patch V (attachment 637638 [details] [diff] [review]) and patch W (attachment 736639 [details] [diff] [review]) represent a new less-aggressive strategy for fixing the bug.  Instead of drastically revising the inheritance hierarchy, I kept the existing non-ideal inheritance structure, and instead abstract the callEnv accesses into a getter in FunctionObject, and allow m_callEnv to be set to null within WeakMethodClosure, which provides its own weakly-held reference to the MethodEnv.

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #11)
> patch V (attachment 637638 [details] [diff] [review]) and patch W (attachment 736639 [details] [diff] [review])

Should have said "patch V (attachment 637638 [details] [diff] [review]) and patch W (attachment 637639 [details] [diff] [review])"

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch W v2: "replace" strong ref to callEnv with a weak one

(fixing dumb mistake where I forgot to initialize the weak ref in the ctor.)

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 637638 [details] [diff] [review]
patch V r1: simpler refactor; add get_callEnv() virtual method

I want to get the review process started.  I've already sent this patch on its own through the shelf-builder last night; note that patch V is intended to be a pure-refactoring in preparation for the next change in patch W where the actual change in structure will be located.

(I'm putting patch W through a set of tests now; assuming that goes okay, I'll be putting a review request on it shortly.)

Ruchi: Feel free to delegate to someone else, e.g. Bill, if you won't get to it (but check with the new reviewer first).  I want to land this today.  If no one is going to be able to do the review today, let me know and I'll determine whether to push with review pending.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

(shelf test on patch V+W passed.  same notes as comment 14 apply here.)

Comment 16

[Ruchi Lohani]

Comment on attachment 637930 [details] [diff] [review]
patch W v2: "replace" strong ref to callEnv with a weak one

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

::: core/MethodClosure.cpp
@@ +149,5 @@
> +    }
> +
> +    /*virtual*/ GCRef<MethodEnv> WeakMethodClosure::get_callEnv() const
> +    {
> +        return _get_callEnv();

I am missing something here. Why a different method and why not just get the weakref here?

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 637930 [details] [diff] [review]
patch W v2: "replace" strong ref to callEnv with a weak one

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

::: core/MethodClosure.cpp
@@ +149,5 @@
> +    }
> +
> +    /*virtual*/ GCRef<MethodEnv> WeakMethodClosure::get_callEnv() const
> +    {
> +        return _get_callEnv();

Mostly to be analogous to the existing structure for WeakMethodClosure::_get_savedThis(): put the "complicated" logic into an inlined non-virtual routine, and then call out to that routine from the virtual method.

But, your question points out that the analogy breaks down, in part because the logic of _get_callEnv() is so much simpler than that of _get_savedThis().

So we're left with a question: Do we want a separate inlined non-virtual _get_callEnv() routine?  I suspect not, I really don't want to think that the efficiency matters at all here.

So I'll probably take your implicit suggestion, and remove _get_callEnv(), folding its body into get_callEnv().

Comment 18

[Tamarin Bot]

changeset: 7455:1c835894e362
user:      Felix Klock II <[e-mail]>
summary:   Bug 767410: Add get_callEnv virtual method to Function and switch to using it everywhere (r=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/1c835894e362

Comment 19

[Tamarin Bot]

changeset: 7456:1e434150cd16
user:      Felix Klock II <[e-mail]>
summary:   Bug 767410: "replace" strong ref to callEnv with a weak ref in WeakMethodClosure, the way it used to be (r=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/1e434150cd16

Comment 20

[Felix S. Klock II [:pnkfelix, :fklock]]

(should have marked as fixed long ago.)