1286459 - Crash in mozilla::WebGLContext::MakeContextCurrent

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-82fee369-435f-47ff-9446-99c2c2160713.
=============================================================

#2 on 0710 Windows nightly, 42 crashes from 14 installations.

Bug 1275866 was resolved at 0606, and it seemed to be helpful. But the crash number has been high since 0703 [1], probably a regression.

[1] https://crash-stats.mozilla.com/signature/?product=Firefox&date=%3E%3D2016-06-01&signature=mozilla%3A%3AWebGLContext%3A%3AMakeContextCurrent&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=date&_sort=&page=1#graphs

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

Review commit: https://reviewboard.mozilla.org/r/65080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65080/

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1286459 - Use WeakPtr for deferred references. -

Review commit: https://reviewboard.mozilla.org/r/65082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65082/

Comment 3

[Jeff Muizelaar [:jrmuizel]]

https://reviewboard.mozilla.org/r/65082/#review62128

I don't see how this solves the crash.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> https://reviewboard.mozilla.org/r/65082/#review62128
> 
> I don't see how this solves the crash.

We've had couple crashes similar to this, where an object is kept alive longer than the context stays non-lost. In these cases, the context has already torn down its GLContext, resulting in a crash during the WebGLObject's CC collection.

The full solution involves changing how we do context loss for WebGLObjects. I would like to defer this to coincide with a larger redo of the context loss system to match the spec. (which I already have patches for) We'll be nailing down the spec in the WG meeting this week Thursday.

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8772232 [details]
Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

https://reviewboard.mozilla.org/r/65080/#review62150

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8772233 [details]
Bug 1286459 - Use WeakPtr for deferred references. -

https://reviewboard.mozilla.org/r/65082/#review62238

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Most of crashes are from WebGLVertexArrayGL::~WebGLVertexArrayGL()[1].

We will call DestroyResourcesAndContext() in webglContext_lost and ~WebglContext which call WebGLVertexArrayGL::DeleteOnce(). So I have no idea why the gl context is not valid here.

In DestroyResourcesAndContext(), there is an early return for checking gl ptr. But I still can't find a path that the list of VertexArray is not empty and the gl ptr is nullptr.

[1]
https://crash-stats.mozilla.com/report/index/04ad3233-d3ae-4c59-ac1c-fba4f2160720
[2]
https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContext.cpp?q=%2Bfunction%3Amozilla%3A%3AWebGLContext%3A%3ADestroyResourcesAndContext%28%29&redirect_type=single#229

Comment 8

[Kelsey Gilbert [:jgilbert]]

These don't call DeleteOnce:
https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/canvas/WebGLContext.cpp#255

These are the likely culprit for ~WebGLVertexArrayGL crashes.

Comment 9

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/mozilla-central/annotate/cde56ead650f/dom/canvas/WebGLContext.cpp#l986
https://hg.mozilla.org/mozilla-central/annotate/cde56ead650f/dom/canvas/WebGLContext.cpp#l1000

These don't call the teardown function, though they are after creating most of the webgl guts in InitAndValidateGL.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> https://hg.mozilla.org/mozilla-central/annotate/cde56ead650f/dom/canvas/
> WebGLContext.cpp#l986
> https://hg.mozilla.org/mozilla-central/annotate/cde56ead650f/dom/canvas/
> WebGLContext.cpp#l1000
> 
> These don't call the teardown function, though they are after creating most
> of the webgl guts in InitAndValidateGL.

I had checked with this search[1] before, they are all in glContext creation. So I think these "gl=nullptr" doesn't result in this crash.

[1]
https://dxr.mozilla.org/mozilla-central/search?q=%22gl+%3D+nullptr%22&redirect=false

Comment 11

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8772232 [details]
Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65080/diff/1-2/

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

https://reviewboard.mozilla.org/r/65080/#review63136

::: dom/canvas/WebGLContextUnchecked.h:47
(Diff revision 2)
>      void SamplerParameteriv(WebGLSampler* sampler, GLenum pname, const GLint* param);
>      void SamplerParameterf(WebGLSampler* sampler, GLenum pname, GLfloat param);
>      void SamplerParameterfv(WebGLSampler* sampler, GLenum pname, const GLfloat* param);
>  
>  protected: // data
> -    RefPtr<gl::GLContext> gl;
> +    RefPtr<gl::GLContext> mGL_NeverTouchDirectly;

I'm still confused with this "never touch" refptr<gl>. Let me think more about it.

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8772232 [details]
Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

https://reviewboard.mozilla.org/r/65080/#review63460

::: dom/canvas/WebGLContextUnchecked.cpp:16
(Diff revision 2)
>  #include "WebGLSampler.h"
>  
>  namespace mozilla {
>  
> -WebGLContextUnchecked::WebGLContextUnchecked(gl::GLContext* gl)
> -    : gl(gl)
> +WebGLContextUnchecked::WebGLContextUnchecked(gl::GLContext* _gl)
> +    : mGL_NeverTouchDirectly(_gl)

Why don't we just call DestroyResourcesAndContext() before every "gl=nullptr"?

Does it have different between the original single RefPtr<GLContext> and this two "neverTouch refptr and raw gl ptr"?

Comment 14

[Nicholas Nethercote [inactive]]

#4 topcrash on Windows in the Nightly 20160722030235.

Comment 15

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #13)
> Comment on attachment 8772232 [details]
> Bug 1286459 - Be very careful about nulling WebGLContext::gl. -
> 
> https://reviewboard.mozilla.org/r/65080/#review63460
> 
> ::: dom/canvas/WebGLContextUnchecked.cpp:16
> (Diff revision 2)
> >  #include "WebGLSampler.h"
> >  
> >  namespace mozilla {
> >  
> > -WebGLContextUnchecked::WebGLContextUnchecked(gl::GLContext* gl)
> > -    : gl(gl)
> > +WebGLContextUnchecked::WebGLContextUnchecked(gl::GLContext* _gl)
> > +    : mGL_NeverTouchDirectly(_gl)
> 
> Why don't we just call DestroyResourcesAndContext() before every
> "gl=nullptr"?
> 
> Does it have different between the original single RefPtr<GLContext> and
> this two "neverTouch refptr and raw gl ptr"?

That's what we should uplift, but we've already shown that we can't be trusted to do this. (Otherwise we wouldn't have this bug :) )

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8772232 [details]
Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65080/diff/2-3/

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: (for uplift) 0001-Bug-1286459-Call-DestroyResourcesAndContext-instead-.patch

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8772232 [details]
Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

https://reviewboard.mozilla.org/r/65080/#review63882

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d231bdcb67a
Be very careful about nulling WebGLContext::gl. - r=jerry

Comment 20

[Kelsey Gilbert [:jgilbert]]

Oh, lucky us! Only reports seem to be in 50. We shouldn't need to uplift.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa5261093ae
Hotfix. CLOSED TREE

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d231bdcb67a
https://hg.mozilla.org/mozilla-central/rev/1aa5261093ae