Mantis - Squeak
Viewing Issue Advanced Details
7352 Kernel major random 05-13-09 13:53 02-06-11 23:48
jmvuletich  
nicolas cellier  
normal  
closed 3.10.2bc  
fixed  
none    
none trunk  
0007352: WeakMessageSend generates a DNU because of the message receiver being collected and disappearing
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.
 WeakMessageSendFixPart1-jmv-M7352.cs [^] (865 bytes) 05-13-09 14:43
 WeakMessageSendFixPart2-jmv-M7352.cs [^] (5,502 bytes) 05-13-09 14:44
 WeakMessageSendFixPart1-jmv-M7352.9.cs [^] (1,055 bytes) 05-14-09 15:11
 WeakMessageSendFixPart2-jmv-M7352.8.cs [^] (6,380 bytes) 05-14-09 15:12

Notes
(0013126)
nicolas cellier   
05-13-09 22:11   
Hi Juan, I approve the essential of the fix, however there are some details to be reviewed, so here are my code critics.
Changing this piece of art is not that straight forward!

1) #isReceiverOrAnyArgumentGarbage is a message only in WeakMessageSend
#isValid was also understood by MessageSend.

Now, with new definition of WeakActionSequence>>#asMinimalRepresentation, a WeakActionSequence cannot include a mixture of MessageSend and WeakMessageSend.
In old time, it could have. Is it on purpose?

2) Catching Exception (WeakActionSequenceTrappingError) is considered bad and dangerous, so watch your steps. You should maybe consider catching Error instead.

3) You have broken WeakActionSequence>>#value, because it will now return nil when last action is garbaged. You can't rely on using #asMinimalRepresentation previously because garbage collection can occur in between as you noticed. I don't know if this has any implication, but the API is clearly changed.

4) Beware, #isValid has another definition in WeakMessageSend subclass: #NonReentrantWeakMessageSend.
With your new usage of #asMinimalRepresentation, it would have been wrong to use NonReentrantWeakMessageSend.>>#isValid because it would exclude definitely a temporarily unavailable action... Unfortunately that also excludes MessageSend - see 1)

Beware if you correct 3), then a NonReentrantWeakMessageSend that is not valid but is not garbaged will return nil when evaluated, and this will be assigned to answer when it should not...
(0013127)
jmvuletich   
05-14-09 15:17   
Hi Nicolas, thanks for reviewing!
1) Fixed. Added #isReceiverOrAnyArgumentGarbage to MessageSend. Did not add back #isValid because it is "false polymorphism", a message implemented by objects that are not polymorphic at all. This is bad in general, it adds confusion and fills 'implementors' and 'senders' with irrelevant stuff.

2) That problem was already there before. I fixed it.

3) Fixed

4) WeakMessageSend does not have any subclass in stock 3.10. It seems that those are part of the PolyMorph package. I guess its mainainers need to update this. I did not fix this.
(0013142)
GazzaGuru   
07-02-09 12:42   
In terms of the semantics of NonReentrantWeakMessageSend, #isValid is not really necessary... the rentrancy is protected upon the call to #valueWithArguments:.

I'll remove in next update.

As for #valueWithEnoughArguments: it *could* be sent in our 3.9 images, just happens not to be used (only used with MessageSend in that image).
I can remove those from Polymorph too, with the caveat that they shouldn't be called in older images.

Of course, when a block is used as an action there's a problem (defines #isValid but not #isReceiverOrAnyArgumentGarbage). Then there's BlockClosures...
(0013843)
nicolas cellier   
08-22-10 19:43   
Fixed in:
http://source.squeak.org/trunk/Kernel-nice.482.mcz [^]
http://source.squeak.org/trunk/System-nice.360.mcz [^]
http://source.squeak.org/trunk/Kernel-nice.483.mcz [^]

Note I did not remove WeakMessageSend>>valueWithEnoughArguments: though it is not used with such receiver.
Who knows, a package might need it...