Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Yet more frame frailty #30

Closed
rjkroege opened this issue Apr 3, 2018 · 16 comments
Closed

Yet more frame frailty #30

rjkroege opened this issue Apr 3, 2018 · 16 comments
Assignees
@rjkroege

Comments

@rjkroege
Copy link
Owner

rjkroege commented Apr 3, 2018

Frame has some bugs. Make it more robust.

Untested: main.plumbthread(): /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/look.go:48 +0x204
Got a message: &{B edit /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood text 0xc4204de2a0 [47 85 115 101 114 115 47 114 106 107 114 111 101 103 101 47 116 111 111 108 115 47 103 111 112 107 103 47 115 114 99 47 103 105 116 104 117 98 46 99 111 109 47 114 106 107 114 111 101 103 101 47 101 100 119 111 111 100 47 46 103 105 116 47 67 79 77 77 73 84 95 69 68 73 84 77 83 71]}
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x1139453]
goroutine 7 [running, locked to thread]:
github.com/rjkroege/edwood/frame.nrune(...)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/util.go:144
github.com/rjkroege/edwood/frame.(*Frame).strlen(0x12d8ac0, 0x1, 0x13e)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/draw.go:278 +0x43
github.com/rjkroege/edwood/frame.(*Frame)._draw(0x12d8ac0, 0x4ed, 0x13e, 0xc4204de300, 0x4ed)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/draw.go:248 +0x1f0
github.com/rjkroege/edwood/frame.(*Frame).bxscan(0xc420338d80, 0xc420200000, 0x5f, 0x5f, 0xc4202b97f0, 0x0, 0xc4202b9740)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/insert.go:104 +0x88e
github.com/rjkroege/edwood/frame.(*Frame).Insert(0xc420338d80, 0xc420200000, 0x5f, 0x5f, 0x0)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/insert.go:165 +0x16c
main.(*Text).Fill(0xc4202c8418)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/text.go:545 +0xc0
main.(*Text).Redraw(0xc4202c8418, 0x4ed, 0x122, 0x800, 0x15a, 0xc420188000, 0xc4200c48c0, 0x334)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/text.go:143 +0x268
main.(*Text).Resize(0xc4202c8418, 0x4ed, 0x122, 0x800, 0x15a, 0x1, 0x5d4)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/text.go:165 +0x1c0
main.(*Window).Resize(0xc4202c8400, 0x4cc, 0x122, 0x800, 0x2e4, 0xc420160000, 0x0)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/wind.go:254 +0x549
main.(*Column).Add(0xc420001800, 0x0, 0x0, 0xffffffffffffffff, 0x1)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/col.go:140 +0x45c
main.makenewwindow(0x0, 0x4e)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/util.go:225 +0x2b6
main.openfile(0x0, 0xc4201628c0, 0x4)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/look.go:765 +0x8b
main.plumblook(0xc4200b2ae0)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/look.go:248 +0x121
main.mousethread(0xc4200ce0c0)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/acme.go:291 +0x310
created by main.main
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/acme.go:168 +0x8a1
@rjkroege rjkroege self-assigned this Apr 3, 2018
rjkroege added a commit that referenced this issue Apr 5, 2018
@rjkroege
[frame] Add an additional findbox test
0b40533 
Add another unit test for findbox. Part of series of changes to help
with #30.
@rjkroege
Copy link
Owner Author

rjkroege commented Apr 6, 2018

Also see #6 for yet more. General issue: Frame assumes quite consistently that the bounds max % line height is 0 and expects the caller to tend to this detail.

Observation: I wonder if the first version of Frame was fixed width / fixed height only and variable width support was retrofitted?

rjkroege added a commit that referenced this issue Apr 8, 2018
@rjkroege
[frame] Remove nalloc, nbox
cbd759e 
There have been several frame bugs related to the fact that Frame.box
is a slice but with manual bounds management via Frame.nalloc and
Frame.nbox. As a result, it was difficult and subtle to use Go
constructs like append on the slice of frbox objects.

Make this code more idiomatic and hopefully less buggy (in pursuit of
great robustness per #30) by making Frame.box a true Go slice.
@paul-lalonde
Copy link
Collaborator

Another one. This is from executing a shell command that made a lot of output. The bad text.go line does t.fr.Delete(0, t.fr.GetFrameFillStatus().Nchars) which I would expect to aways succeed.
So: there might be a race in edwood that is changing Nchars between the GetFrameFillStatus() call the Delete(), unless Nchars is maintained incorrectly.

panic: Frame.ptofchar in Frame.delete

goroutine 4 [running]:
github.com/rjkroege/edwood/frame.(*Frame).Delete(0xc42033a360, 0x0, 0x8ee, 0x1)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/frame/delete.go:111 +0xd90
main.(*Text).setorigin(0xc420206528, 0xa1e3, 0x1)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/text.go:1640 +0x155
main.(*Text).SetOrigin(0xc420206528, 0xa1e3, 0x1)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/text.go:1604 +0x43
main.(*Text).Show(0xc420206528, 0xa33a, 0xa33a, 0x1f01)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/text.go:1264 +0x15c
main.xfidwrite.func1()
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/xfid.go:483 +0x20d
main.xfidwrite(0xc420204000)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/xfid.go:501 +0x438
main.xfidctl(0xc420204000, 0xc4200cc0c0)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/xfid.go:46 +0x87
created by main.xfidallocthread
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/acme.go:628 +0x1e5

@paul-lalonde
Copy link
Collaborator

In an attempt to catch a race, I instrumented Frame.Init, Delete, Insert with a global mutex. Same outcome. More evidence of Nchars getting out of sync :-(

@paul-lalonde
Copy link
Collaborator

And while typing in a frame just now:

panic: endofframe

goroutine 23 [running]:
github.com/rjkroege/edwood/frame.(*Frame).chop(0xc0001ab320, 0x3fd, 0x4a9, 0x6b1, 0x94)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/frame/insert.go:112 +0x16f
github.com/rjkroege/edwood/frame.(*Frame).Insert(0xc0001ab320, 0xc000478dfc, 0x1, 0x1, 0x54d)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/frame/insert.go:238 +0x1069
main.(*Text).Insert(0xc00021ed28, 0x54d, 0xc000478dfc, 0x1, 0x1, 0x0)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/text.go:475 +0x5b2
main.(*Text).Type(0xc00021ed28, 0x66)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/text.go:1011 +0x396
main.(*Window).Type(0xc00021ec00, 0xc00021ed28, 0x66)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/wind.go:419 +0x37
main.(*Row).Type(0x12e6800, 0x66, 0x52c, 0x43b, 0xc00021ed28)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/row.go:285 +0x133
main.keyboardthread(0xc0000aa0c0)
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/acme.go:464 +0x1e7
created by main.main
	/Users/plalonde/dev/src/github.com/rjkroege/edwood/acme.go:172 +0x926

@rjkroege
Copy link
Owner Author

We would appear to be running frame code from multiple go routines. In particular:

goroutine 66 [running]:
github.com/rjkroege/edwood/frame.(*Frame).cklinewrap(0x12f0340, 0x19c0, 0x333, 0x0, 0x638, 0x333)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/util.go:45 +0x26
github.com/rjkroege/edwood/frame.(*Frame).drawtext(0x12f0340, 0x4ed, 0x317, 0xc4200c0770, 0xc42014c230)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/draw.go:13 +0xb4
github.com/rjkroege/edwood/frame.(*Frame).Insert(0xc42036a900, 0xc420432420, 0x57, 0x58, 0x29a)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/insert.go:344 +0xd7f
main.(*Text).Insert(0xc420266528, 0x29a, 0xc420432420, 0x57, 0x58, 0x1)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/text.go:475 +0x591
main.xfidwrite(0xc4203d4000)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/xfid.go:579 +0x8f4
main.xfidctl(0xc4203d4000, 0xc4200de0c0)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/xfid.go:46 +0x87
created by main.xfidallocthread
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/acme.go:628 +0x1e5

and obviously from main.keyboardthread per above.

And... frame.bxscan uses a global Frame object so there's no way it's thread safe.

rjkroege added a commit that referenced this issue Apr 11, 2018
@rjkroege
[frame] Add more extensive testing for frame
4413940 
Add code that permits frame.Delete and frame.Insert to validate the box
model before and after execution. Use the -validateboxes flag to enable
this mode.

Part of ongoing work to address #30.
@paul-lalonde
Copy link
Collaborator

Now, that's interesting.
We can funnel all frame changes through a chan; we could lock Text; or we could lock Frame.
Preferences? I have a small preference for the first option - the frame API is small enough to make this work in an hour of work, I think.

@rjkroege
Copy link
Owner Author

I will remove the global so that every Frame instance can proceed in parallel. And maybe funnel interaction with Text's Frame through a channel?

@rjkroege rjkroege mentioned this issue Apr 12, 2018
Closed
rjkroege added a commit that referenced this issue Apr 12, 2018
@rjkroege
[frame] Replace the frame global
83b4d25 
Frame was using a global Frame object for temporary storage during
box insertion but was being accessed by different Go routines. This
was highly not thread-safe. Make the Frame object different for
each Insert operation.

This change improves #30. It also fixes $PLAN9/bin/E.
@rjkroege
Copy link
Owner Author

$PLAN9/bin/E still crashes sometimes. But at least now doesn't crash all the time.

rjkroege added a commit that referenced this issue Apr 13, 2018
@rjkroege
[frame] Remove nalloc, nbox
5a4eb78 
There have been several frame bugs related to the fact that Frame.box
is a slice but with manual bounds management via Frame.nalloc and
Frame.nbox. As a result, it was difficult and subtle to use Go
constructs like append on the slice of frbox objects.

Make this code more idiomatic and hopefully less buggy (in pursuit of
great robustness per #30) by making Frame.box a true Go slice.
rjkroege added a commit that referenced this issue Apr 13, 2018
@rjkroege
[frame] Additional robustness check
d17cb27 
Add an additional robustness check to Frame for chop failures. Part
of improving #30.
@rjkroege
Copy link
Owner Author

Two additional/new issues:

  • TAB boxes are getting (wrongly) replicated.
  • Frame.chop is running past the end.

@rjkroege
Copy link
Owner Author

Attempting to delete text from an already empty box? Stack trace looks like a race condition between the filesystem thread and the a different thread?

2018/04/16 08:23:18 Frame.Delete Start p0=0 p1=108
2018/04/16 08:23:18 -- runes in boxes != NChars --
2018/04/16 08:23:18 end: -- runes in boxes != NChars -- &{0xc4201080a0 0xc4200b60c0 0xc4201260e0 [0xc420010a10 0xc42016c070 0xc42016c0e0 0xc4200107e0 0xc4200107e0] (1261,62)-(2048,118) (1261,62)-(2048,118) [] 0 0 48 108 2 2 false true 0xc4200100e0 0xc420010150 false false false 2}
panic: -- runes in boxes != NChars --

goroutine 89 [running]:
github.com/rjkroege/edwood/frame.(*Frame).validateboxmodel(0xc42027ea00, 0x11f1c2d, 0x1e, 0xc4201d5c38, 0x2, 0x2)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/box.go:177 +0x41d
github.com/rjkroege/edwood/frame.(*Frame).Delete(0xc42027ea00, 0x0, 0x6c, 0x0)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/frame/delete.go:13 +0x124
main.(*Text).Delete(0xc420072818, 0x0, 0x6c, 0xc420206101)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/text.go:598 +0x3ad
main.(*Window).SetTag1(0xc420072800)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/wind.go:523 +0x438
main.(*Window).SetTag(0xc420072800)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/wind.go:462 +0x8d
main.xfidclose(0xc420120160)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/xfid.go:247 +0x233
main.xfidctl(0xc420120160, 0xc4200b60c0)
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/xfid.go:46 +0x87
created by main.xfidallocthread
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/acme.go:628 +0x1e5

@paul-lalonde
Copy link
Collaborator

Today I was getting actual work done with edwood.
Whenever inserting pushed a line past the current right edge of the frame, or deleting pulled text up from the line before, it would mis-render, mostly replacing text with blanks. 100% reliable. Scroll away and back, and it's fixed.

rjkroege added a commit that referenced this issue Apr 21, 2018
@rjkroege
[frame] Attempt at making chop more robust.
0a6d6c3 
Part of work on #30. Make chop immune to failure when attempting
to chop the very end of the box model.
rjkroege added a commit that referenced this issue Apr 23, 2018
@rjkroege
[frame] Fixed off-by-1 bug in canfit
b92354f 
canfit was mis-measuring boxes and falsely wrapping when it shouldn't
Part of addressing #30. This change makes canfit use the full width
of the window.
@rjkroege
Copy link
Owner Author

rjkroege commented May 1, 2018

Visual damage: type some invalid characters and tab and there will be some pixel turds on the screen.

@rjkroege
Copy link
Owner Author

rjkroege commented May 1, 2018

Unusual crash:

acme: text.type: <nil>                                                                                                                                                                                                             
panic: text.delete                                                                                                                                                                                                                 
                                                                                                                                                                                                                                   
goroutine 7 [running]:                                                                                                                                                                                                             
main.(*Text).Delete(0xc4202e2528, 0x1f0, 0x365, 0x101)                                                                                                                                                                             
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/text.go:553 +0x46b                                                                                                                                              
main.cut(0xc4202e2528, 0xc4202e2528, 0x0, 0x101, 0x0, 0x0)                                                                                                                                                                         
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/exec.go:320 +0x31d                                                                                                                                              
main.(*Text).Type(0xc4202e2528, 0x69)                                                                                                                                                                                              
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/text.go:881 +0x205                                                                                                                                              
main.(*Window).Type(0xc4202e2400, 0xc4202e2528, 0x69)                                                                                                                                                                              
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/wind.go:417 +0x37                                                                                                                                               
main.(*Row).Type(0x12ef600, 0x69, 0xcd, 0x17a, 0xc4202e2528)                                                                                                                                                                       
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/row.go:285 +0x139                                                                                                                                               
main.keyboardthread(0xc4200d00c0)                                                                                                                                                                                                  
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/acme.go:464 +0x1e8                                                                                                                                              
created by main.main                                                                                                                                                                                                               
        /Users/rjkroege/tools/gopkg/src/github.com/rjkroege/edwood/acme.go:172 +0x959

@rjkroege
Copy link
Owner Author

rjkroege commented May 1, 2018

Previous two issues are connected. The crash happens when the state of the page is invalid after the tab/insert.

@rjkroege
Copy link
Owner Author

Thread safety (as of b8089af) seems to improve robustness.

@rjkroege
Copy link
Owner Author

rjkroege commented Jun 9, 2018

I haven't seen any crashes in Frame for weeks now. I declare this fixed

@rjkroege rjkroege closed this as completed Jun 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rjkroege rjkroege

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rjkroege @paul-lalonde