Closed Bug 1382185 Opened 4 years ago Closed 3 years ago

Crash if MakeCurrent fails during the creation of a GLContext.

Categories

(Core :: Graphics, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox56 --- wontfix
firefox57 --- fix-optional

People

(Reporter: nical, Assigned: nical)

Details

(Whiteboard: [gfx-noted])

Attachments

(8 files)

This call to make current https://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/gfx/gl/GLContext.cpp#538 can fail and when this happens we crash right after in fGetString as we check that the context is current.
For method that return a boolean, just return false. Readback doesn't return anything at the moment, so I just added a gfxCriticalError there but we could return a bool instead and go through users of this method to make sure they handle the failure.
Assignee: nobody → nical.bugzilla
Attachment #8887898 - Flags: review?(jgilbert)
I changed my mind about the way Readback was handled in the previous patch. This patch make Readback return false in case of error and fixes the callers.
Attachment #8887903 - Flags: review?(jgilbert)
Using LOCAL_GL_CONTEXT_LOST as the error is a bit of a shot in the dark here, let me know if there is a better one.
Attachment #8887909 - Flags: review?(jgilbert)
The amount of places where we ignore the result of MakeCurrent is a bit discouraging but I think that we should get into the habit of doing this properly. At the very least in anything related to creating and destroying contexts or objects because of the various ugly shutdown scenarios where the underlying context dies before our wrappers.
Attachment #8887917 - Flags: review?(jgilbert)
Comment on attachment 8887908 [details] [diff] [review]
Check the status of MakeCurrent in CanvasRenderingContext2D

Review of attachment 8887908 [details] [diff] [review]:
-----------------------------------------------------------------

Should we report a gfxWarning or something of the likes in this case?
Attachment #8887908 - Flags: review?(bas) → review+
Comment on attachment 8887898 [details] [diff] [review]
Check the return value of MakeCurrent in various places in GLContext.cpp

Review of attachment 8887898 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.cpp
@@ +2451,5 @@
>      if (!mHeavyGLCallsSinceLastFlush) {
>          return;
>      }
> +    if (MakeCurrent()) {
> +        fFlush();

if (!MakeCurrent())
    return;

fFlush();

...is better

@@ +2905,5 @@
>          return false;
>  
> +    if (!MakeCurrent()) {
> +        return false;
> +    }

Don't bother with {} around one-line control flow statements. (match the code above)
Attachment #8887898 - Flags: review?(jgilbert) → review+
Attachment #8887903 - Flags: review?(jgilbert) → review+
Comment on attachment 8887908 [details] [diff] [review]
Check the status of MakeCurrent in CanvasRenderingContext2D

Review of attachment 8887908 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/TexUnpackBlob.cpp
@@ -886,5 @@
>  
>      ////
>  
>      const auto& gl = webgl->gl;
> -    MOZ_ALWAYS_TRUE( gl->MakeCurrent() );

Revert this spurious change.
Attachment #8887909 - Flags: review?(jgilbert) → review+
(In reply to Nicolas Silva [:nical] from comment #6)
> Created attachment 8887917 [details] [diff] [review]
> Check the status of MakeCurrent in WebGLBuffer
> 
> The amount of places where we ignore the result of MakeCurrent is a bit
> discouraging but I think that we should get into the habit of doing this
> properly. At the very least in anything related to creating and destroying
> contexts or objects because of the various ugly shutdown scenarios where the
> underlying context dies before our wrappers.

Fortunately, to some extent, we have to support our context collapsing under us at any time, given how context loss/device reset works. We just need to make sure the landing is soft.

Since we can't actually guarantee that one MakeCurrent success means our call will succeed, we don't need to make all our logic fallible. Instead, we should ensure that a MakeCurrent failure is harmless, and correctly precipitates WebGL's context loss.
Comment on attachment 8888995 [details]
Bug 1382185 - Clear MakeCurrent when MakeCurrent fails. -

https://reviewboard.mozilla.org/r/160022/#review165652
Attachment #8888995 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8888997 [details]
Bug 1382185 - Consider un-forced MakeCurrent failures context-loss. -

https://reviewboard.mozilla.org/r/160028/#review165656
Attachment #8888997 - Flags: review?(nical.bugzilla) → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a698a65f759
Check the status of MakeCurrent in GLContext.cpp. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7764f3432bac
Check the status of GLContext::MakeCurrent in TexUnpackBlob.cpp. r=jgilber
Darnit, I got the bug number wrong in two of the patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b746e314d3d1
Return false in GLContext::Readback if MakeCurrent failed. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/7928b9c4f4c3
Check the status of GLContext::MakeCurrent in CanvasRenderingContext2D. r=Bas
Keywords: leave-open
Do we expect the two unreviewed patches to eventually land?
I believe they ended up not being necessary. :nical?
Flags: needinfo?(nical.bugzilla)
I think that they should land. We may have not run into crashes because of not checking make-current there (so it's not an emergency), but it's just a matter of timing/ordering so I believe these cases are potential source of crash and more generally I would like to establish the convention of always being sure that the context was successfully made current before using it.
Flags: needinfo?(nical.bugzilla)
The leave-open keyword is there and there is no activity for 6 months.
:nical, maybe it's time to close this bug?
Flags: needinfo?(nical.bugzilla)
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(nical.bugzilla)
Resolution: --- → FIXED
Comment on attachment 8887915 [details] [diff] [review]
Check the status of MakeCurrent in WebGLContext

Review of attachment 8887915 [details] [diff] [review]:
-----------------------------------------------------------------

WebGL is implicitly made-current, so we don't need these.
Attachment #8887915 - Flags: review?(jgilbert) → review-
Comment on attachment 8887917 [details] [diff] [review]
Check the status of MakeCurrent in WebGLBuffer

Review of attachment 8887917 [details] [diff] [review]:
-----------------------------------------------------------------

WebGL is implicitly made-current, so we don't need these.
Attachment #8887917 - Flags: review?(jgilbert) → review-
You need to log in before you can comment on or make changes to this bug.