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.

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

Fix some crashes #74

Merged
merged 2 commits into from
Feb 19, 2016
Merged

Fix some crashes #74

merged 2 commits into from
Feb 19, 2016

Conversation

timholy
Copy link
Contributor

@timholy timholy commented Feb 14, 2016

This fixes two problems (observed on Linux w/ NVIDIA drivers):

  • Bool(x) produces an InexactError if x != 0 && x != 1
  • Addition of yield() before destroying the window allows any already-scheduled drawing operations to occur, preventing an error like:
X Error of failed request:  GLXBadDrawable
  Major opcode of failed request:  154 (GLX)
  Minor opcode of failed request:  11 (X_GLXSwapBuffers)
  Serial number of failed request:  171
  Current serial number in output stream:  172

@jayschwa
Copy link
Member

Thanks. I'm willing to merge both changes, but a bit of due diligence first.

How are you encountering the InexactError? What value is the ccall returning? Since SetWindowShouldClose only accepts a Bool, I can't envision how WindowShouldClose can get other values.

Do you have a small test case for the segfault? If it's originating from GLFW, then I'd like to report it upstream.

Are there possible side-effects to the yield for people who aren't using co-routines, or perhaps using them differently than your scenario? Why do you believe the yield should live here and not be the responsibility of the caller?

@timholy
Copy link
Contributor Author

timholy commented Feb 14, 2016

Both errors cropped up in response to me closing the window---by me clicking on the close button with the mouse, not by issuing a SetWindowShouldClose command.

Like I said in the commit message (I should have copied it into the text above), I've seen glfwWindowShouldClose return the value 189. As you surely know, in C, if (x) evaluates to true if x != 0, but julia is more strict, and in most places in Base such ccalls are written the way I did here. Note that glfwSetWindowShouldClose itself takes an int as its input, http://www.glfw.org/docs/latest/group__window.html#ga49c449dde2a6f87d996f4daaa09d6708, so it doesn't seem out of the question that it would return values different from 0 and 1 if something else (not julia) is setting the close flag.

As far as the segfault goes, I could get that to happen relatively reliably with some of the examples in GLVisualize (run them once, close the window, then run them again). My suspicion is that the use of Reactive, which schedules operations for the future, is what's leading to the segfault. While one might say that it would be better to chase down the logic in the rendering functions and make sure that it never tries to render to an invalid surface, to me it seemed there was real possibility of a race condition. Consequently I wondered it might be safer to make sure that scheduled events finish before closing the window.

CC @SimonDanisch.

@SimonDanisch
Copy link
Member

I can confirm both errors ;) I must admit, that I fixed the `InexactError locally and forgot about it :D
The other one has bugged me as well and this seems to be a valid temporary solution! I'd like to do better book keeping of the OpenGL context at some point, but this will rquire quite some restructuring!

At least on Linux, this ccall can return numbers that are not 0 or 1 (I've seen 189).
@timholy
Copy link
Contributor Author

timholy commented Feb 17, 2016

OK, @jayschwa I added tests for both issues. The first commit should be completely uncontroversial.

The second is basically a way of circumventing a bug in a user's render function. I'm not sure it should hurt anything, but if you prefer I can drop the second commit.

@SimonDanisch, the bug that triggers the segfault in many of the GLVisualize examples has the direct fix in JuliaGL/GLWindow.jl#26; that should be applied no matter what happens to the 2nd commit here.

@jayschwa
Copy link
Member

👍 on first commit. I'll defer to @SimonDanisch on the second since I guess it depends on what happens in his other packages.

@SimonDanisch
Copy link
Member

I think this is a fair temporary fix! I'll look deeper into the issue when I clean up context handling and event management!

@timholy
Copy link
Contributor Author

timholy commented Feb 19, 2016

@SimonDanisch, since JuliaGL/GLWindow.jl#26 is merged, should I drop the second?

@timholy
Copy link
Contributor Author

timholy commented Feb 19, 2016

Wow, quite a coincidence of timing. If I understand correctly, we should keep both, so I guess this could be merged.

jayschwa added a commit that referenced this pull request Feb 19, 2016
@jayschwa jayschwa merged commit 00531f3 into JuliaGL:master Feb 19, 2016
@jayschwa
Copy link
Member

Thanks for the contribution @timholy. Thanks for weighing in @SimonDanisch. I'll tag a point release from master this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants