Bug 32845 – [Chromium] WebGL crashes intermittently on Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

Testing of the WebGL demos at http://khronos.org/webgl/wiki/Demo_Repository with Chromium on Linux results in intermittent crashes and displays of the "Aw, snap!" page.

[Kenneth Russell]

Created attachment 45362 [details]
Patch

The dlclose'ing of libGL.so.1 and dlopen'ing of it each time a GraphicsContext3D was created was occasionally causing it to be re-mapped at a different base address. Since GLEW is not re-initialized every time, primarily for performance reasons, its cached function pointers were pointing to garbage. Stopped closing and re-opening libGL.so.1 each time; now it is loaded lazily, when the first 3D context is created. Also reused the X display connection since the GLX routines' correctness might hinge upon it not resulting in a change of GL implementation.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45362 [details] without any errors.

[Eric Seidel (no email)]

Commentary from one of the linux folks would make this review easier. :)

[Adam Langley]

LGTM. I didn't know that WebGL was even supposed to work on Linux, nor how it's getting a connection to the X server.

So the patch looks fine, but if it actually runs then there's something terribly wrong with our sandbox.

[Kenneth Russell]

(In reply to comment #4)
> LGTM. I didn't know that WebGL was even supposed to work on Linux, nor how it's
> getting a connection to the X server.
> 
> So the patch looks fine, but if it actually runs then there's something
> terribly wrong with our sandbox.

Thanks for your review.

WebGL does work in Chromium on Linux and its other supported platforms (see http://khronos.org/webgl/wiki/Getting_a_WebGL_Implementation), but yes, it currently requires disabling the sandbox which is very undesirable. We're working to remove this requirement.

Can you r+ and cq+ the patch?

[Evan Martin]

Did you consider making a class out of all the statics and then having a static instance of that class?  (It might also make the diff smaller.)

[Adam Langley]

(Note: I am not a WebKit reviewer, so I cannot r+ your patch.)

[Kenneth Russell]

(In reply to comment #6)
> Did you consider making a class out of all the statics and then having a static
> instance of that class?  (It might also make the diff smaller.)

I think that would end up being more complicated.

Note that all of the code in this file is essentially temporary until we move the 3D calls into Chromium's upcoming GPU process, so I didn't want to do extensive refactorings.

[Kenneth Russell]

(In reply to comment #7)
> (Note: I am not a WebKit reviewer, so I cannot r+ your patch.)

No problem. How can I get this committed? A co-worker needs to do work in the same portions of this file, so I would really like to get it checked in now so we don't collide.

[Eric Seidel (no email)]

I'll take another look.

[Eric Seidel (no email)]

Looking again, I agree with Evan, this would probably all be cleaner held behind one static pointer instead of adding a whole bunch of static pointers (with unclear lifetimes) to the main class.  Putting them all in one place leaves them as m_ vars and makes their lifetime clear.  You could even make them a struct instead if that would be cleaner.  Then the calls become: s_libGL->glXChooseFBConfig()

Can you explain more about how this is temporary?  If we're about to delete this code, do we need to fix it?

[Kenneth Russell]

(In reply to comment #11)
> Looking again, I agree with Evan, this would probably all be cleaner held
> behind one static pointer instead of adding a whole bunch of static pointers
> (with unclear lifetimes) to the main class.  Putting them all in one place
> leaves them as m_ vars and makes their lifetime clear.  You could even make
> them a struct instead if that would be cleaner.  Then the calls become:
> s_libGL->glXChooseFBConfig()

OK, I'll make these changes and upload a new patch.

> Can you explain more about how this is temporary?  If we're about to delete
> this code, do we need to fix it?

We anticipate moving these calls out of the renderer process next quarter. In the interim we must fix the issue as the crashes are very noticeable and poor behavior.

[Kenneth Russell]

Created attachment 45403 [details]
Revised patch

Factored new statics into a separate GLConnection class, of which there is a static instance.

[WebKit Review Bot]

Attachment 45403 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/GraphicsContext3D.cpp:201:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebKit/chromium/src/GraphicsContext3D.cpp:212:  render_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/chromium/src/GraphicsContext3D.cpp:212:  share_list is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3

[Kenneth Russell]

Created attachment 45405 [details]
Revised patch

Fixed errors reported by style-queue.

[Eric Seidel (no email)]

Comment on attachment 45405 [details]
Revised patch

You beat me!  Now my comments are probably mostly obsolete:

A few naming issues.  Some of which the stylebot caught.

We generally try to encouage full names like "display" instead of "dpy"

It's unclear to me if s_initializedGLEW is still needed or if a s_gl check would be enough now?

I think some folks use sizeof(libnames[0]) in this case:
 319         for (int i = 0; i < sizeof(libNames) / sizeof(const char*); i++)
Sadly we don't have a template function for that, ojan recently filed a bug about htat.

I think we should fix the style-bot errors, but otherwise this is ready to land.  I can't remember if you're a committer yet or not.

[Kenneth Russell]

(In reply to comment #16)
> (From update of attachment 45405 [details])
> You beat me!  Now my comments are probably mostly obsolete:
> 
> A few naming issues.  Some of which the stylebot caught.
> 
> We generally try to encouage full names like "display" instead of "dpy"

I just copied these names out of the GLX headers.

> It's unclear to me if s_initializedGLEW is still needed or if a s_gl check
> would be enough now?

s_initializedGLEW is still needed on all platforms.

> I think some folks use sizeof(libnames[0]) in this case:
>  319         for (int i = 0; i < sizeof(libNames) / sizeof(const char*); i++)
> Sadly we don't have a template function for that, ojan recently filed a bug
> about htat.

Thanks for the suggestion. I'll certainly keep this in mind and if you want me to change it here now I will.

> I think we should fix the style-bot errors, but otherwise this is ready to
> land.  I can't remember if you're a committer yet or not.

Style fixes done as you've seen. I'm not a committer yet.

[Eric Seidel (no email)]

Comment on attachment 45405 [details]
Revised patch

Looks plenty good enough for now.

[WebKit Commit Bot]

Comment on attachment 45405 [details]
Revised patch

Rejecting patch 45405 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Last 500 characters of output:
outTests
Testing 11820 test cases.
http/tests/loading/bad-server-subframe.html -> timed out
Sampling process 7092 for 10 seconds with 10 milliseconds of run time between samples
Sampling completed, processing symbols...
Sample analysis of process 7092 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt

Exiting early after 1 failures. 8746 tests run.
210.59s total testing time

8745 test cases (99%) succeeded
1 test case (<1%) timed out
4 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/138507

[Eric Seidel (no email)]

Comment on attachment 45405 [details]
Revised patch

Your patch was another victim of bug 30392.

[WebKit Commit Bot]

Comment on attachment 45405 [details]
Revised patch

Clearing flags on attachment: 45405

Committed r52504: <http://trac.webkit.org/changeset/52504>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Evan Martin]

BTW, I'm surprised you need the libNames list; dlopen knows to search the library path if you give it a plain filename without slashes (which will also make this code work for people who have libGL in /usr/local/lib or whatever)

[Evan Martin]

For those who care, I filed
  https://bugs.webkit.org/show_bug.cgi?id=33032
about the dlopen thing