[squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Juan Vuletich-4
Hi Folks,

I found a bug in WeakMessageSend that generates a DNU because of the
message receiver being collected and disappearing. The culprits are
#ensureReceiver and #ensureReceiverAndArguments . These methods ensure
nothing. It might happen (although not often) that the receiver or some
argument is collected after this method is called, but before it is
actually used.

The problem happens in Squeak 3.10, Pharo-10243, and Cuis.

Please take a look. Also take a look at my proposed solution. I'm not
sure, maybe some will object keeping an artificial reference to an
object that could have been collected otherwise, eve if it is just for
the time of a message send... Anyway, comments welcome.

Cheers,
Juan Vuletich

'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May 2009 at 11:02:25 am'!

!WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'!
value
        ^ arguments isNil
                ifTrue: [
                        self withEnsuredReceiverDo: [ :r | r perform: selector]]
                ifFalse: [
                        self withEnsuredReceiverAndArgumentsDo: [ :r :a |
                                r
                                        perform: selector
                                        withArguments: a]]! !

!WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'!
valueWithArguments: anArray
        | argsToUse |
       
        "Safe to use, because they are built before ensureing receiver and args..."
        argsToUse _ self collectArguments: anArray.
        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
                                r
                                        perform: selector
                                        withArguments: argsToUse ]! !

!WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'!
valueWithEnoughArguments: anArray

        "call the selector with enough arguments from arguments and anArray"
        | args |
        args _ Array new: selector numArgs.
        args replaceFrom: 1
                to: ( arguments size min: args size)
                with: arguments
                startingAt: 1.
        args size > arguments size ifTrue: [
                args replaceFrom: arguments size + 1
                        to: (arguments size + anArray size min: args size)
                        with: anArray
                        startingAt: 1.
        ].

        "args is safe to use, because they are built before ensureing receiver and args..."
        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
                                r
                                        perform: selector
                                        withArguments: args ]! !

!WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'!
withEnsuredReceiverAndArgumentsDo: aBlock
        "Grab real references to receiver and arguments. If they still exist, evaluate aBlock."

        "Return if my receiver has gone away"
        | r a |
        r _ self receiver.
        r ifNil: [ ^self ].

       
        "Make sure that my arguments haven't gone away"
        "arguments can not become nil. The extra care in #ensureReceiverAndArguments is wrong
        (BTW, the whole idea is wrong, that's why we do these new methods...)"
        a _ Array withAll: arguments.
        a with: shouldBeNil do: [ :arg :flag |
                arg ifNil: [ flag ifFalse: [ ^self ]]
        ].

        aBlock value: r value: a! !

!WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'!
withEnsuredReceiverDo: aBlock
        "Grab a real reference to receive. If still there, evaluate aBlock."

        "Return if my receiver has gone away"
        | r |
        r _ self receiver.
        r ifNil: [ ^self ].

        aBlock value: r! !

WeakMessageSend removeSelector: #ensureReceiver!
WeakMessageSend removeSelector: #ensureReceiverAndArguments!


Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Nicolas Cellier
Yes sure, that was stupid!

But:

1) #value & al used to return the value of evaluating the message, or
nil if any receiver/argument garbaged.
Now #value & al will always return self

2) #valueWithArguments: should not care if arguments were garbage
collected, because some replacement arguments were provided

3) #valueWithEnoughArguments: should care of garbage collected
arguments from: anArray size to: arguments size.

Please publish a mantis report, then upload your updated code :)

Cheers

Nicolas

2009/5/12 Juan Vuletich <[hidden email]>:

> Hi Folks,
>
> I found a bug in WeakMessageSend that generates a DNU because of the message
> receiver being collected and disappearing. The culprits are #ensureReceiver
> and #ensureReceiverAndArguments . These methods ensure nothing. It might
> happen (although not often) that the receiver or some argument is collected
> after this method is called, but before it is actually used.
>
> The problem happens in Squeak 3.10, Pharo-10243, and Cuis.
>
> Please take a look. Also take a look at my proposed solution. I'm not sure,
> maybe some will object keeping an artificial reference to an object that
> could have been collected otherwise, eve if it is just for the time of a
> message send... Anyway, comments welcome.
>
> Cheers,
> Juan Vuletich
>
> 'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May
> 2009 at 11:02:25 am'!
>
> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'!
> value
>        ^ arguments isNil
>                ifTrue: [
>                        self withEnsuredReceiverDo: [ :r | r perform:
> selector]]
>                ifFalse: [
>                        self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>                                r
>                                        perform: selector
>                                        withArguments: a]]! !
>
> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'!
> valueWithArguments: anArray
>        | argsToUse |
>
>        "Safe to use, because they are built before ensureing receiver and
> args..."
>        argsToUse _ self collectArguments: anArray.
>        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>                                r
>                                        perform: selector
>                                        withArguments: argsToUse ]! !
>
> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'!
> valueWithEnoughArguments: anArray
>
>        "call the selector with enough arguments from arguments and anArray"
>        | args |
>        args _ Array new: selector numArgs.
>        args replaceFrom: 1
>                to: ( arguments size min: args size)
>                with: arguments
>                startingAt: 1.
>        args size > arguments size ifTrue: [
>                args replaceFrom: arguments size + 1
>                        to: (arguments size + anArray size min: args size)
>                        with: anArray
>                        startingAt: 1.
>        ].
>
>        "args is safe to use, because they are built before ensureing
> receiver and args..."
>        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>                                r
>                                        perform: selector
>                                        withArguments: args ]! !
>
> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'!
> withEnsuredReceiverAndArgumentsDo: aBlock
>        "Grab real references to receiver and arguments. If they still exist,
> evaluate aBlock."
>
>        "Return if my receiver has gone away"
>        | r a |
>        r _ self receiver.
>        r ifNil: [ ^self ].
>
>
>        "Make sure that my arguments haven't gone away"
>        "arguments can not become nil. The extra care in
> #ensureReceiverAndArguments is wrong
>        (BTW, the whole idea is wrong, that's why we do these new
> methods...)"
>        a _ Array withAll: arguments.
>        a with: shouldBeNil do: [ :arg :flag |
>                arg ifNil: [ flag ifFalse: [ ^self ]]
>        ].
>
>        aBlock value: r value: a! !
>
> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'!
> withEnsuredReceiverDo: aBlock
>        "Grab a real reference to receive. If still there, evaluate aBlock."
>
>        "Return if my receiver has gone away"
>        | r |
>        r _ self receiver.
>        r ifNil: [ ^self ].
>
>        aBlock value: r! !
>
> WeakMessageSend removeSelector: #ensureReceiver!
> WeakMessageSend removeSelector: #ensureReceiverAndArguments!
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Nicolas Cellier
Oops I did not understand the API at first glance...
3) was wrong because arguments are added first, and anArray last -

One more thing: WeakActionSequence using isValid test is fragile too...


2009/5/12 Nicolas Cellier <[hidden email]>:

> Yes sure, that was stupid!
>
> But:
>
> 1) #value & al used to return the value of evaluating the message, or
> nil if any receiver/argument garbaged.
> Now #value & al will always return self
>
> 2) #valueWithArguments: should not care if arguments were garbage
> collected, because some replacement arguments were provided
>
> 3) #valueWithEnoughArguments: should care of garbage collected
> arguments from: anArray size to: arguments size.
>
> Please publish a mantis report, then upload your updated code :)
>
> Cheers
>
> Nicolas
>
> 2009/5/12 Juan Vuletich <[hidden email]>:
>> Hi Folks,
>>
>> I found a bug in WeakMessageSend that generates a DNU because of the message
>> receiver being collected and disappearing. The culprits are #ensureReceiver
>> and #ensureReceiverAndArguments . These methods ensure nothing. It might
>> happen (although not often) that the receiver or some argument is collected
>> after this method is called, but before it is actually used.
>>
>> The problem happens in Squeak 3.10, Pharo-10243, and Cuis.
>>
>> Please take a look. Also take a look at my proposed solution. I'm not sure,
>> maybe some will object keeping an artificial reference to an object that
>> could have been collected otherwise, eve if it is just for the time of a
>> message send... Anyway, comments welcome.
>>
>> Cheers,
>> Juan Vuletich
>>
>> 'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May
>> 2009 at 11:02:25 am'!
>>
>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'!
>> value
>>        ^ arguments isNil
>>                ifTrue: [
>>                        self withEnsuredReceiverDo: [ :r | r perform:
>> selector]]
>>                ifFalse: [
>>                        self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>>                                r
>>                                        perform: selector
>>                                        withArguments: a]]! !
>>
>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'!
>> valueWithArguments: anArray
>>        | argsToUse |
>>
>>        "Safe to use, because they are built before ensureing receiver and
>> args..."
>>        argsToUse _ self collectArguments: anArray.
>>        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>>                                r
>>                                        perform: selector
>>                                        withArguments: argsToUse ]! !
>>
>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'!
>> valueWithEnoughArguments: anArray
>>
>>        "call the selector with enough arguments from arguments and anArray"
>>        | args |
>>        args _ Array new: selector numArgs.
>>        args replaceFrom: 1
>>                to: ( arguments size min: args size)
>>                with: arguments
>>                startingAt: 1.
>>        args size > arguments size ifTrue: [
>>                args replaceFrom: arguments size + 1
>>                        to: (arguments size + anArray size min: args size)
>>                        with: anArray
>>                        startingAt: 1.
>>        ].
>>
>>        "args is safe to use, because they are built before ensureing
>> receiver and args..."
>>        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>>                                r
>>                                        perform: selector
>>                                        withArguments: args ]! !
>>
>> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'!
>> withEnsuredReceiverAndArgumentsDo: aBlock
>>        "Grab real references to receiver and arguments. If they still exist,
>> evaluate aBlock."
>>
>>        "Return if my receiver has gone away"
>>        | r a |
>>        r _ self receiver.
>>        r ifNil: [ ^self ].
>>
>>
>>        "Make sure that my arguments haven't gone away"
>>        "arguments can not become nil. The extra care in
>> #ensureReceiverAndArguments is wrong
>>        (BTW, the whole idea is wrong, that's why we do these new
>> methods...)"
>>        a _ Array withAll: arguments.
>>        a with: shouldBeNil do: [ :arg :flag |
>>                arg ifNil: [ flag ifFalse: [ ^self ]]
>>        ].
>>
>>        aBlock value: r value: a! !
>>
>> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'!
>> withEnsuredReceiverDo: aBlock
>>        "Grab a real reference to receive. If still there, evaluate aBlock."
>>
>>        "Return if my receiver has gone away"
>>        | r |
>>        r _ self receiver.
>>        r ifNil: [ ^self ].
>>
>>        aBlock value: r! !
>>
>> WeakMessageSend removeSelector: #ensureReceiver!
>> WeakMessageSend removeSelector: #ensureReceiverAndArguments!
>>
>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Nicolas Cellier
Oh, and I forgot, correct class comment:

 shouldBeNil Array of: Boolean --  used to distinguish nil arguments
from garbage collected arguments

2009/5/12 Nicolas Cellier <[hidden email]>:

> Oops I did not understand the API at first glance...
> 3) was wrong because arguments are added first, and anArray last -
>
> One more thing: WeakActionSequence using isValid test is fragile too...
>
>
> 2009/5/12 Nicolas Cellier <[hidden email]>:
>> Yes sure, that was stupid!
>>
>> But:
>>
>> 1) #value & al used to return the value of evaluating the message, or
>> nil if any receiver/argument garbaged.
>> Now #value & al will always return self
>>
>> 2) #valueWithArguments: should not care if arguments were garbage
>> collected, because some replacement arguments were provided
>>
>> 3) #valueWithEnoughArguments: should care of garbage collected
>> arguments from: anArray size to: arguments size.
>>
>> Please publish a mantis report, then upload your updated code :)
>>
>> Cheers
>>
>> Nicolas
>>
>> 2009/5/12 Juan Vuletich <[hidden email]>:
>>> Hi Folks,
>>>
>>> I found a bug in WeakMessageSend that generates a DNU because of the message
>>> receiver being collected and disappearing. The culprits are #ensureReceiver
>>> and #ensureReceiverAndArguments . These methods ensure nothing. It might
>>> happen (although not often) that the receiver or some argument is collected
>>> after this method is called, but before it is actually used.
>>>
>>> The problem happens in Squeak 3.10, Pharo-10243, and Cuis.
>>>
>>> Please take a look. Also take a look at my proposed solution. I'm not sure,
>>> maybe some will object keeping an artificial reference to an object that
>>> could have been collected otherwise, eve if it is just for the time of a
>>> message send... Anyway, comments welcome.
>>>
>>> Cheers,
>>> Juan Vuletich
>>>
>>> 'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May
>>> 2009 at 11:02:25 am'!
>>>
>>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'!
>>> value
>>>        ^ arguments isNil
>>>                ifTrue: [
>>>                        self withEnsuredReceiverDo: [ :r | r perform:
>>> selector]]
>>>                ifFalse: [
>>>                        self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>>>                                r
>>>                                        perform: selector
>>>                                        withArguments: a]]! !
>>>
>>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'!
>>> valueWithArguments: anArray
>>>        | argsToUse |
>>>
>>>        "Safe to use, because they are built before ensureing receiver and
>>> args..."
>>>        argsToUse _ self collectArguments: anArray.
>>>        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>>>                                r
>>>                                        perform: selector
>>>                                        withArguments: argsToUse ]! !
>>>
>>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'!
>>> valueWithEnoughArguments: anArray
>>>
>>>        "call the selector with enough arguments from arguments and anArray"
>>>        | args |
>>>        args _ Array new: selector numArgs.
>>>        args replaceFrom: 1
>>>                to: ( arguments size min: args size)
>>>                with: arguments
>>>                startingAt: 1.
>>>        args size > arguments size ifTrue: [
>>>                args replaceFrom: arguments size + 1
>>>                        to: (arguments size + anArray size min: args size)
>>>                        with: anArray
>>>                        startingAt: 1.
>>>        ].
>>>
>>>        "args is safe to use, because they are built before ensureing
>>> receiver and args..."
>>>        ^self withEnsuredReceiverAndArgumentsDo: [ :r :a |
>>>                                r
>>>                                        perform: selector
>>>                                        withArguments: args ]! !
>>>
>>> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'!
>>> withEnsuredReceiverAndArgumentsDo: aBlock
>>>        "Grab real references to receiver and arguments. If they still exist,
>>> evaluate aBlock."
>>>
>>>        "Return if my receiver has gone away"
>>>        | r a |
>>>        r _ self receiver.
>>>        r ifNil: [ ^self ].
>>>
>>>
>>>        "Make sure that my arguments haven't gone away"
>>>        "arguments can not become nil. The extra care in
>>> #ensureReceiverAndArguments is wrong
>>>        (BTW, the whole idea is wrong, that's why we do these new
>>> methods...)"
>>>        a _ Array withAll: arguments.
>>>        a with: shouldBeNil do: [ :arg :flag |
>>>                arg ifNil: [ flag ifFalse: [ ^self ]]
>>>        ].
>>>
>>>        aBlock value: r value: a! !
>>>
>>> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'!
>>> withEnsuredReceiverDo: aBlock
>>>        "Grab a real reference to receive. If still there, evaluate aBlock."
>>>
>>>        "Return if my receiver has gone away"
>>>        | r |
>>>        r _ self receiver.
>>>        r ifNil: [ ^self ].
>>>
>>>        aBlock value: r! !
>>>
>>> WeakMessageSend removeSelector: #ensureReceiver!
>>> WeakMessageSend removeSelector: #ensureReceiverAndArguments!
>>>
>>>
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Juan Vuletich-4
Done. Mantis issue 7352. Please take a look at updated code.

Comments interleaved...

Nicolas Cellier wrote:
> Oh, and I forgot, correct class comment:
>
>  shouldBeNil Array of: Boolean --  used to distinguish nil arguments
> from garbage collected arguments
>
>  
added

> 2009/5/12 Nicolas Cellier <[hidden email]>:
>  
>> Oops I did not understand the API at first glance...
>> 3) was wrong because arguments are added first, and anArray last -
>>
>> One more thing: WeakActionSequence using isValid test is fragile too...
>>
>>    

It actually was not fragile, becuse it used the new safe methods in
WeakMessageSend. What I did is to remove the #isValid tests.

>> 2009/5/12 Nicolas Cellier <[hidden email]>:
>>    
>>> Yes sure, that was stupid!
>>>
>>> But:
>>>
>>> 1) #value & al used to return the value of evaluating the message, or
>>> nil if any receiver/argument garbaged.
>>> Now #value & al will always return self
>>>      

fixed. thanks.

>>> 2) #valueWithArguments: should not care if arguments were garbage
>>> collected, because some replacement arguments were provided
>>>      

What I did with this is to honor the old behavior. The (unwritten)
specification is: "If the receiver or some argument is collected, then
do nothing". Sounds reasonable, and I saw no reason to change this.

>>> 3) #valueWithEnoughArguments: should care of garbage collected
>>> arguments from: anArray size to: arguments size.
>>>
>>> Please publish a mantis report, then upload your updated code :)
>>>
>>> Cheers
>>>
>>>      

I also changed all _ to := , to ease integration in Croquet.

Cheers,
Juan Vuletich

Reply | Threaded
Open this post in threaded view
|

Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Nicolas Cellier
2009/5/13 Juan Vuletich <[hidden email]>:

> Done. Mantis issue 7352. Please take a look at updated code.
>
> Comments interleaved...
>
> Nicolas Cellier wrote:
>>
>> Oh, and I forgot, correct class comment:
>>
>>  shouldBeNil            Array of: Boolean --  used to distinguish nil
>> arguments
>> from garbage collected arguments
>>
>>
>
> added
>
>> 2009/5/12 Nicolas Cellier <[hidden email]>:
>>
>>>
>>> Oops I did not understand the API at first glance...
>>> 3) was wrong because arguments are added first, and anArray last -
>>>
>>> One more thing: WeakActionSequence using isValid test is fragile too...
>>>
>>>
>
> It actually was not fragile, becuse it used the new safe methods in
> WeakMessageSend. What I did is to remove the #isValid tests.
>
>>> 2009/5/12 Nicolas Cellier <[hidden email]>:
>>>
>>>>
>>>> Yes sure, that was stupid!
>>>>
>>>> But:
>>>>
>>>> 1) #value & al used to return the value of evaluating the message, or
>>>> nil if any receiver/argument garbaged.
>>>> Now #value & al will always return self
>>>>
>
> fixed. thanks.
>
>>>> 2) #valueWithArguments: should not care if arguments were garbage
>>>> collected, because some replacement arguments were provided
>>>>
>
> What I did with this is to honor the old behavior. The (unwritten)
> specification is: "If the receiver or some argument is collected, then do
> nothing". Sounds reasonable, and I saw no reason to change this.
>

Sure, this is a very odd undocumented API, so it's hard to decide
whether a bug or not. Usage should be analyzed first.
My guess is that intention was to provide nil arguments where we don't
want Weakness, and replace these nils further with provided
replacement anArray... But the implementation is limited to these two
cases:
1) Weak arguments are all at the beginning and we use #valueWithEnoughArguments:
2) Weak arguments are all at the end and the shouldBeNil are all at
the beginning
The case of a Weak argument superseded by an element of anArray should
be a side effect hardly ever used, so I guess that does not much
matter to keep or change API in this special case.

In Pharo, I would recommend to deprecate and sweep this stuff (provide
a better API if necessary).
In distributions claiming maximum compatibility, I would recommend
keeping the API. But it's not ideal to be stucked on such kind of
code.

Nicolas

>>>> 3) #valueWithEnoughArguments: should care of garbage collected
>>>> arguments from: anArray size to: arguments size.
>>>>
>>>> Please publish a mantis report, then upload your updated code :)
>>>>
>>>> Cheers
>>>>
>>>>
>
> I also changed all _ to := , to ease integration in Croquet.
>
> Cheers,
> Juan Vuletich
>
>

Reply | Threaded
Open this post in threaded view
|

Attention Polymorph package maintainers - Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend

Juan Vuletich-4
In reply to this post by Juan Vuletich-4
Hi Nicolas,

I saw your notes to http://bugs.squeak.org/view.php?id=7352 . Thanks for
your careful and detailed review! I addressed all the issues you raise
with new change sets, except those about WeakMessageSend subclasses. It
seems that NonReentrantWeakMessageSend and ExclusiveWeakMessageSend are
part of the Polymorph package. I believe they should modify them as needed.

Cheers,
Juan Vuletich