Closed
Bug 1382185
Opened 7 years ago
Closed 5 years ago
Crash if MakeCurrent fails during the creation of a GLContext.
Categories
(Core :: Graphics, enhancement, P3)
Core
Graphics
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox56 | --- | wontfix |
firefox57 | --- | fix-optional |
People
(Reporter: nical, Assigned: nical)
Details
(Whiteboard: [gfx-noted])
Attachments
(8 files)
2.40 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
4.90 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
1.80 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
6.30 KB,
patch
|
jgilbert
:
review-
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
jgilbert
:
review-
|
Details | Diff | Splinter Review |
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nical
:
review+
|
Details |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8887908 -
Flags: review?(bas)
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8887915 -
Flags: review?(jgilbert)
Assignee | ||
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
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 8•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8887903 -
Flags: review?(jgilbert) → review+
Comment 9•7 years ago
|
||
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.
Updated•7 years ago
|
Attachment #8887909 -
Flags: review?(jgilbert) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-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+
Comment 16•7 years ago
|
||
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
Assignee | ||
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2a698a65f759 https://hg.mozilla.org/mozilla-central/rev/7764f3432bac
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b746e314d3d1 https://hg.mozilla.org/mozilla-central/rev/7928b9c4f4c3
Updated•7 years ago
|
Priority: -- → P3
Do we expect the two unreviewed patches to eventually land?
status-firefox56:
--- → fix-optional
status-firefox57:
--- → fix-optional
Comment 21•7 years ago
|
||
I believe they ended up not being necessary. :nical?
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 22•7 years ago
|
||
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)
Updated•7 years ago
|
Comment 23•5 years ago
|
||
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)
Assignee | ||
Updated•5 years ago
|
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(nical.bugzilla)
Resolution: --- → FIXED
Updated•5 years ago
|
Keywords: leave-open
Comment 24•5 years ago
|
||
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 25•5 years ago
|
||
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.
Description
•