Closed Bug 1286459 Opened 4 years ago Closed 4 years ago

Crash in mozilla::WebGLContext::MakeContextCurrent

Categories

(Core :: Canvas: WebGL, defect)

Unspecified
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ting, Assigned: jgilbert)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(2 files, 1 obsolete file)

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
Flags: needinfo?(jgilbert)
Assignee: nobody → jgilbert
Flags: needinfo?(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 on attachment 8772232 [details]
Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

https://reviewboard.mozilla.org/r/65080/#review62150
Attachment #8772232 - Flags: review?(mtseng) → review+
Attachment #8772233 - Flags: review?(jmuizelaar) → review+
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
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.
(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 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/
Attachment #8772232 - Attachment description: Bug 1286459 - Reformat header. - → Bug 1286459 - Be very careful about nulling WebGLContext::gl. -
Attachment #8772232 - Flags: review+ → review?(hshih)
Attachment #8772233 - Attachment is obsolete: true
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 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"?
Attachment #8772232 - Flags: review?(hshih) → review-
#4 topcrash on Windows in the Nightly 20160722030235.
Keywords: topcrash
(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 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/
Attachment #8772232 - Flags: review- → review?(hshih)
Comment on attachment 8772232 [details]
Bug 1286459 - Be very careful about nulling WebGLContext::gl. -

https://reviewboard.mozilla.org/r/65080/#review63882
Attachment #8772232 - Flags: review?(hshih) → review+
Attachment #8774582 - Flags: review?(hshih) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d231bdcb67a
Be very careful about nulling WebGLContext::gl. - r=jerry
Oh, lucky us! Only reports seem to be in 50. We shouldn't need to uplift.
https://hg.mozilla.org/mozilla-central/rev/7d231bdcb67a
https://hg.mozilla.org/mozilla-central/rev/1aa5261093ae
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.