Opened 14 months ago

Last modified 7 weeks ago

#13191 assigned bug

[Flurry] screensaver crashes in __strtod_internal

Reported by: diver Owned by: Alexander von Gluck
Priority: normal Milestone: Unscheduled
Component: Add-Ons/Screen Savers Version: R1/Development
Keywords: Cc: Alexander von Gluck
Blocked By: Blocking:
Has a Patch: no Platform: x86-64

Description

hrev50849 x86_64.

Selecting Flurry screensaver crashes ScreenSaver preflet.

Not reproducible in x86_gcc2.

Attachments (3)

ScreenSaver-9243-debug-10-01-2017-18-16-42.report (20.7 KB) - added by diver 14 months ago.
ScreenSaver-788-debug-16-01-2017-19-04-05.report (19.4 KB) - added by Premislaus 13 months ago.
Another crash report.
0001-Ensure-screen-savers-have-an-active-GL-context.patch (1.9 KB) - added by Andrew Aldridge 8 weeks ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 14 months ago by diver

Owner: changed from Nobody to Alexander von Gluck
Status: newassigned

Changed 13 months ago by Premislaus

Another crash report.

comment:2 Changed 8 weeks ago by Andrew Aldridge

This is because the FlurryView::AttachedToWindow() logic looks like the following:

void FlurryView::AttachedToWindow() {
  LockGL();
  BGLView::AttachedToWindow(); // Calls UnlockGL()
  // Do stuff assuming a context is active
}

When glGetString(GL_VERSION) is called, it returns a null string. The GLife screensaver has the same problem, but instead of crashing it just renders incorrectly.

For both Flurry and GLife, moving the LockGL call *after* the call to BGLView::AttachedToWindow fixes the problem.

I'll try to create a patch tomorrow.

Changed 8 weeks ago by Andrew Aldridge

comment:3 Changed 8 weeks ago by Andrew Aldridge

Has a Patch: set

comment:4 Changed 8 weeks ago by Andrew Aldridge

(Maybe BGLView::AttachedToWindow() shouldn't unconditionally unlock the GL context; I can persue that route instead if it's preferable)

comment:5 Changed 8 weeks ago by waddlesplash

What did BeOS do re. locking? We should try to mimic that.

comment:6 Changed 8 weeks ago by Andrew Aldridge

The BeBook seems to indicate that the existing behavior of the screensavers should be correct, meaning my patch should be rejected. The current BGLView::AttachedToWindow() locking logic was added to Haiku in fafc6e4b776ba7c9eff20696976d9fdb85113682 (2010) without explanation.

There are two options I can see:

  • Revert that part of the change, making Haiku match BeOS more closely based on the BeBook's code samples, but breaking applications written to depend on Haiku's current behavior.
  • Only call UnlockGL() iff the initial state of BGLView::fDisplayLocker::IsLocked() is false. That is, restore the original state right after setting up the glViewport.

I'll pursue the second option as a pull request to the mesa package in haikuports, if that's the right procedure?

comment:7 Changed 8 weeks ago by Andrew Aldridge

Has a Patch: unset

comment:8 Changed 8 weeks ago by waddlesplash

Cc: Alexander von Gluck added

CC'ing kallisti5 as he's in charge of Mesa-related stuff.

Yes, that's probably correct. Note that there are 2 versions of Mesa though (Mesa 7 for GCC2 and current Mesa for GCC5+) and both will need to be patched I think.

comment:9 Changed 7 weeks ago by PulkoMandy

Aren't LockGL() and UnlockGL() recursive locks? (sorry, I'm not familiar with the API)

If they are, then there should be no problem here - AttachedToWindow could safely call LockGL then UnlockGL which would restore the initial state of the lock.

A patch was submitted here: https://github.com/haikuports/haikuports/pull/2070/files Submitting directly to github.com/haiku/mesa (for gcc2) and upstream mesa (for gcc5) would be better than having the patch in haikuports.

comment:10 Changed 7 weeks ago by Andrew Aldridge

LockGL() and UnlockGL() currently don't behave recursively. I think they probably should?

I appreciate everybody taking the time to help me muddle my way through what a proper fix for this looks like :D

comment:11 Changed 7 weeks ago by waddlesplash

The Be Book does not specify whether or not they are recursive. Guess someone needs to fire up BeOS and find out...

comment:12 Changed 7 weeks ago by PulkoMandy

Let's dig out that BeOS machine. Maybe someone has code I could easily test?

Note: See TracTickets for help on using tickets.