Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0007119 [Squeak Packages] OmniBrowser minor always 07-10-08 15:06 07-16-08 13:22
Reporter sittiri View Status public  
Assigned To dr
Priority normal Resolution fixed  
Status resolved  
Summary 0007119: SystemWindow instances hanging in squeak-dev image
Description Fire up a fresh squeak-dev or -web image (we tried with the latest 3.9
and 3.10).
Print it:

"(A)" Smalltalk garbageCollect. (SystemWindow allInstances difference: World submorphs) size " 0 "

Open a browser (OmniBrowser) create a class or edit a method, accept
it, then close the window.
Print (A) again, it answers 1 instead of 0.
Additional Information
Attached Files  MorphicUIBugTe...tforZombies.st [^] (502 bytes) 07-11-08 18:40
 OBGroupingMorph(2625).jpeg [^] (28,036 bytes) 07-11-08 23:25
 OBBrowser-zombies-Patch-M7119-nice.1.cs [^] (453 bytes) 07-15-08 20:46

- Relationships

- Notes
(0012353 - 120 - 144 - 144 - 144 - 144 - 144)
matthewf
07-10-08 17:48

Here's a shot in the dark about how to work around this:

Utilities cleanseOtherworldySteppers

just a random guess.
 
(0012354 - 82 - 88 - 88 - 88 - 88 - 88)
sittiri
07-10-08 18:24

No, it doesn't help. Nice method name, though :)
(#cleanseOtherworld_l_ySteppers)
 
(0012355 - 875 - 1017 - 1017 - 1017 - 1017 - 1017)
wiz
07-11-08 01:01

Hi sittiri,

Nice bug find. It's always good to see another reporter who can describe how to easily reproduce a problem.

More: data.

I checked preferences to assure that the trash was not suppose to hold on to things. It was not.

I did an explore on the result instead of a print.
You get a list with one item.
selecting that Item and evaluating "self openInHand")
confirms the instance is the deleted browser.
(with the brower in the world the count goes back to 0}.
deleting it brings it back to 1.


Also doing sittiri,'s procedure twice (separate browser each time) increases the count to 2.

Exploring pointers seems to indicate that #accept: and how the model handles it is the source of the problem.

Apparently OB is not using a weak array or there are some very clingy weak arrays out there.

Yours in curiosity and service, --Jerome Peace
 
(0012357 - 288 - 306 - 306 - 306 - 306 - 306)
dr
07-11-08 08:44

right, I can confirm this problem, it also happens to me.
I couldn't yet find out what exactly happens during #accept: that triggers this problem though...
I tried to introduce weak arrays at some places, but this didn't help so far.
If anyone has some other ideas, please let me know.
 
(0012364 - 894 - 995 - 995 - 995 - 995 - 995)
wiz
07-11-08 18:51

Uploaded a goodie test to check for unworldly OBGroupingMorphs
MorphicUIBugTe...tforZombies.st

the pointer chain (expolring pointers is new to me. I am not an expert at interpreting results)
shows the hanging on morphs to be pointed to by a '' which looks like an empty text selection
This intern is pointed to by a compiled method which seems to correspond to the symbol #selection: in someones method dictionary.
I got lost when trying to figure out which class the method dictionary belonged to.
 
Though I ran into algernon stuff along the way.
Would removing Algernon cure the problem.

Caveat on the test. It is not a probe and not a true test.
It will catch the memory leak morphs but it also finds morphs that belong to windows collapse to the docking bar.

When it fails, the analysis starts by exploring the rejects.

Yours in curiosity and service, --Jerome Peace
 
(0012365 - 948 - 1092 - 1092 - 1092 - 1092 - 1092)
wiz
07-11-08 23:37

OBGroupingMorph(2625).jpeg

In a fresh sq7179dev08.07.01 I
I installed the zombie test.
Ran it (it passed)
Got a browser.
opened a method and selected some text in the method but did not
do an accept.

Then deleted the browser and ran the test again.

(The test did pass)
Then I did the last two steps again.
And the test failed this time.

The picture is an inspector on one of one of the rejects from the test.

The submorphs are still listed in the dependents dictionaries. And I suspect this is relevant to the issue.
Submorphs point to their owners so if the submorphs are keys in a dependents dictionary then everything up the chain is held on to as well including the system window.

My though on the cure is that deletion may have to be recursively applied to all submorphs to remove their dependents.

The next question is how do the legacy browsers avoid this?

Yours in curiosity and service, --Jerome Peace
 
(0012367 - 1342 - 1581 - 1581 - 1581 - 1581 - 1581)
wiz
07-12-08 01:55

Ok.
In a fresh july dev image.

Install the zombie catcher.
Get a test-runner
select Test-Bugs>>MorphicUiBugTest
Run selected (you get 3 green passes)

World>open>system browser (You get a system browser)
Select a method ( I used Installer>mantis>bug:)
cause an edit then erase it (I just hilighted String and did a printit then deleted the printed class name)
Delete the browser. When it asks say discard edits.

Use the test runner to run selected again.
Now you get two passes and the zombie catcher has caught something.
Run the failure to get the debugger.
Select the zombie test and Highlight the rejects temp var.
Ask to explore the contents.
You get an array with two Grouping morphs.

Highlight the second one (it corresponds to the text pane)
in the evaluation area do :
self submorphsDo: [:each | each delete ] .

Now delete the debugger and the explorer (to remove extraneous refs)
 and run the selected tests again.
Back to all three tests passing.

So the problem is not from accepting edits just from making them.

When deleting the browser care needs to be taken that components with dependents delete their dependent arrays and themselves from the keys.

And maybe discarding edits needs to be a little more thorough for Omnibrowsers.

Hth,

Yours in service and curiosity, --Jerome Peace
 
(0012376 - 503 - 515 - 515 - 515 - 515 - 515)
dr
07-15-08 09:01

I agree, the presence of OBGroupingMorph instances in the submorphs of system windows is the cause of the problem. Or at least the problem is solvable when deleting this instances.
But are you sure this is related to the dependents array? As this is a weak array, I was thinking it shouldn't hamper garbage collection?
Also it's strange to me why this only happens when editing takes place, because the grouping morphs are always present, they are used to layout the various components in the browser.
 
(0012377 - 698 - 921 - 921 - 1016 - 1016 - 1016)
nicolas cellier
07-15-08 15:26

I see some DependentsArray are "broken":
    Smalltalk garbageCollect.
    (SystemWindow allInstances difference: World submorphs) size.
    ((Object classPool at: #DependentsFields) select: [:e | e basicSize > 1])
        inspect.
You see some dependents pretends they have nil as first element...
Maybe 0002701 can cure this, but this is not the real problem...


Observe this:
    (EventManager classPool at: #ActionMaps) inspect.
If you remove the (an OBEnhancementDefinitionPanel) keys, then the garbage is recycled...

Yes, the key is weak, but the value isn't.
If the value is pointing on the key, then the key will NEVER get removed.
I didn't check, but i bet this is the case.
 
(0012378 - 1042 - 1174 - 1174 - 1269 - 1269 - 1269)
wiz
07-15-08 18:21

Hi Nicholas,

Thanks for your insights and reference to 0002701.
Great clues.


Hi dr,

OBGroupMorph is not necessarily the key culprit.
I settled on testing for it because in was closer to the problem than systemwindow. The owner chain is what keeps both the group morphs and the system window in the zombie chain.

The closest culprit is probably the OBPluggableTextMorph.

I had no exposure to OB code but the suspicion about dependency came from looking at what the non OB code for the multiselection browser did when deleting.

All component model stuff does recursive dependency breaking. So this bug probably occured earlier and was handled.

OB seems to have been writen w/o regard of the previous browser code.
So OB runs into the integration bugs. The code review that would locate them would be to look at the code for the browsers it is trying to replace and notice the difference.

Of course tests help tremendously. The simple zombie checker served me well.

Yours in curiosity and service, --Jeorme Peace
 
(0012379 - 584 - 662 - 662 - 662 - 662 - 662)
nicolas cellier
07-15-08 19:06

Hi Jerome,
not sure you read carefully my comments.

The problem lies in
OBDefinitionPanel>>#initializeCompletionController

This one associates an action to itself.
This is a problem, because the action also point to self,
(a MessageSend to an ECController whose model is self).
Thus, a weak reference to self is added in (EventManager classPool at: #ActionMaps).
Since an associated action points to self too, and the the value is not weak, a strong reference to self will persist, and the weak key will never be reclaimed.

Reread my previous post and try by yourself.
 
(0012380 - 657 - 768 - 768 - 768 - 768 - 768)
nicolas cellier
07-15-08 20:45

Note that SHWorkspace are also registered in (EventManager classPool at: #ActionMaps), but they are reclaimed.

This is because a SHWorkspace is the model of associated SystemWindow.
When the SystemWindow is closed, it sends a (model release), which sends a (self releaseActionMap) ... et voila pourquoi ca marche.

Possible solutions would be:
a) to arrange to send a release to the OBDefinitionPanel upon window close...
   I did it in OBBrowser>>#release.
b) to add a slot (inst var?) to OBDefinitionPanel...
   I understand ecompletion authors rejected this solution though
   (to keep it lightweight and minimize preinstall scripts I presume)
 
(0012382 - 1250 - 1364 - 1364 - 1364 - 1364 - 1364)
wiz
07-16-08 05:05

Hi Nicolas,

I've had a chance since last commenting to look further into your remarks.

I found a ActionMap key that was the model of the target of its message send. So you were right on about the cicularity.

My curiosity is paying only slight attention to this specific problem. It's got me thinking about the bigger issue of how do we look for and find these things in the future.

I would classify this bug a very serious integration bug.
New authors writing code w/o understanding how the code they are replacing worked. And missing entirely what is needed to do dependents correctly.

So how deep does it go? I can get zombies from a dev image with a system browser (OB) no need for a multiple selection browser.

So what I need to know next is what is a good testable invariant for finding these kind of memory leaks. What kind of integrity test would catch them and prove that all the places that need your patch have gotten them.

One more complication is that Gaza Guru has added a morphic property to hold dendents for morphs. So in light of these problems, I would like to see that tested to know if it is adding to the difficulties or not.

Anyway, I very much appreciate your knowledge and efforts.

Cheers -Jer
 
(0012383 - 913 - 989 - 989 - 989 - 989 - 989)
nicolas cellier
07-16-08 07:32

Jerome,
you ask the very right question, how to avoid such bugs in the future?
The roots of the problem are:

1) The event framework has a "weak"ness, the case when the value will reference same object as weak key seems very common to me
2) e-completion hackers totally hi-jacked this framework to simply store an information related to an instance and used only in that instance, without creating an instance variable...

Obviously, something has to be changed, or the bug will reproduce in one form or another.
For 1), there are plenty of workaround like WeakMessageSend trying to avoid the problem. IMO this is just another sign that the framework is not robust. Maybe we should look if Announcement framework has stronger bases.
For 2), I don't have a solution in mind. Submit the problem to e-completion authors and/or to squeak-dev.

In the interim, consider the patch I provided as a workaround.
 
(0012384 - 141 - 153 - 153 - 153 - 153 - 153)
dr
07-16-08 13:22

Thanks a lot for your efforts, Nicolas and Jerome!

I integrated the provided patch as a workaround for this problem in OB-Standard-dr.344.
 

- Issue History
Date Modified Username Field Change
07-10-08 15:06 sittiri New Issue
07-10-08 15:06 sittiri Status new => assigned
07-10-08 15:06 sittiri Assigned To  => Damien Cassou
07-10-08 17:21 Damien Cassou Assigned To Damien Cassou => dr
07-10-08 17:48 matthewf Note Added: 0012353
07-10-08 18:24 sittiri Note Added: 0012354
07-11-08 01:01 wiz Note Added: 0012355
07-11-08 08:44 dr Note Added: 0012357
07-11-08 18:40 wiz File Added: MorphicUIBugTe...tforZombies.st
07-11-08 18:51 wiz Note Added: 0012364
07-11-08 23:25 wiz File Added: OBGroupingMorph(2625).jpeg
07-11-08 23:37 wiz Note Added: 0012365
07-12-08 01:55 wiz Note Added: 0012367
07-15-08 09:01 dr Note Added: 0012376
07-15-08 15:26 nicolas cellier Note Added: 0012377
07-15-08 18:21 wiz Note Added: 0012378
07-15-08 19:06 nicolas cellier Note Added: 0012379
07-15-08 20:45 nicolas cellier Note Added: 0012380
07-15-08 20:46 nicolas cellier File Added: OBBrowser-zombies-Patch-M7119-nice.1.cs
07-16-08 05:05 wiz Note Added: 0012382
07-16-08 07:32 nicolas cellier Note Added: 0012383
07-16-08 13:22 dr Status assigned => resolved
07-16-08 13:22 dr Resolution open => fixed
07-16-08 13:22 dr Note Added: 0012384


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
104 total queries executed.
63 unique queries executed.
Powered by Mantis Bugtracker