Mantis - Squeak
Viewing Issue Advanced Details
2701 Kernel minor always 02-09-06 22:51 04-18-10 21:59
nicolas cellier  
andreas  
normal  
closed 3.8  
fixed  
none    
none trunk  
0002701: DependentsArray might potentially be misused
Problem identified is the risk of having a window closed without saving the changes among others.

I can exhibit a failure test case built on a silly example (see attachment uploaded),
but i do not know if the potential problem will express in a real case,
because this bug was found from bottom up code analysis.

See subject (DependentsArray size how does it work ?) on squeak-dev for details.

I also attached an incomplete unsatisfying patch not robust to future code evolutions. See additional info and attachments.
evaluating
  dep := self dependents.
  1 to: dep size do: [:i | (dep at: i) doSomething]
might not be correct.

if some weak dependent has been reclaimed by garbage collector (see definition of class DependentsArray), then it is replaced by nil in dependents collection.
The above loop might end prematurely because #size will count only the non nil.
The loop might be evaluated with some nilled elements and not evaluated with some non nil dependents.

It happens that such (1 to:self size do:) construction is called indirectly in 3.8 image either via message #do:without: and also with #includes:.

Also, a (self dependents first) is called in TestRunner that could incorrectly answer nil. I presume this cannot be the case while the window is opened.

Check calls on #dependents.

Attached patch is non satisfying regarding lack of robustness to future code evolution: i just redefined afew collection messages in DependentArray so as not to use super #to:do: construct.

Almost each and every SequenceableCollection messages use this to:do: construct... If one ever send an unprotected message to its dependents, some other bug will potentially appear.

Maybe DependentsArray could also redefine #at:, but i do not know if it might break some methods and it would transform loop budget from n to n*n...

More robust solution should be searched for.

I suggest this bug remain open, or stored somewhere until better patch found.
 DependentsPatch.4.cs [^] (3,152 bytes) 02-09-06 22:51
 DependentsTest.1.cs [^] (1,493 bytes) 02-09-06 22:53
 DependentsTest.2.cs [^] (1,921 bytes) 02-21-07 23:08

Notes
(0010072)
nicolas cellier   
02-21-07 23:22   
A new test is added in DependentsTest.2.cs showing how trivially broken is the current implementation of DependentsArray.

I just add twice the same dependents, which should be prevented by the includes: test in #addDependent:
(0010073)
nicolas cellier   
02-21-07 23:28   
Ah, i found a good example of broken dependents consequence in 3.9 7067 image:

Open a regular browser,
Select a class (you should see class comment),
Save and quit the image,
Restart the image,
Select a method:
-> you see the place of class comment pane is not recycled...

Load DependentsPatch4.cs and replay above test
Ah it seems to work better...
Still not convinced about pending nasty bugs?
(0010094)
Keith_Hodges   
02-22-07 06:21   
"fix begin"
Installer mantis bug:2701 fix: 'DependentsPatch.4.cs'.
"fix test"
Installer mantis bug:2701 fix: 'DependentsTest.2.cs'.
"fix end"
(0013383)
nicolas cellier   
11-14-09 21:29   
Fixed in
http://source.squeak.org/trunk/Kernel-nice.299.mcz [^]
http://source.squeak.org/trunk/KernelTests-nice.108.mcz [^]

Note: The fix is different from changeset provided here.
I preferred to change hierarchy of DependentsArray