Closed Bug 1228687 Opened 4 years ago Closed 4 years ago

"Assertion failure: gl->IsCurrent()"

Categories

(Core :: Canvas: WebGL, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed

People

(Reporter: jruderman, Assigned: milan, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: gfx-noted)

Attachments

(3 files)

Attached file testcase
This assertion is part of code added at:
https://hg.mozilla.org/mozilla-central/diff/7d1c223f397c/dom/canvas/WebGLContextDraw.cpp#l1.502

changeset:   https://hg.mozilla.org/mozilla-central/rev/7d1c223f397c
user:        Jeff Gilbert
date:        Tue Nov 24 20:15:29 2015 -0800
summary:     Bug 1221822 - Finish the WebGL texture refactor. r=kamidphish,mattwoodrow,bz
Attached file stack
Flags: needinfo?(jgilbert)
Whiteboard: gfx-noted
DrawElements_check is responsible for making the context current, but in two places in WebGLContextDraw.cpp, we create a ScopedResolveTexturesForDraw before we make that call to DrawElements_check.  On the other hand, ScopedResolveTexturesForDraw creates and binds a FakeBlackTexture, which wants the context to be current.
Assignee: nobody → milan
https://reviewboard.mozilla.org/r/38221/#review34779

::: dom/canvas/WebGLContextDraw.cpp:52
(Diff revision 1)
> +    mWebGL->MakeContextCurrent();

Don't MakeCurrent here. MakeCurrent should be in the calling code.
This should just assert that it's current.

::: dom/canvas/WebGLContextDraw.cpp:224
(Diff revision 1)
> -    MakeContextCurrent();
> +    // Right now, the creation of ScopedResolveTexturesForDraw make the context current

Don't hide the MakeCurrent like this.
Attachment #8726744 - Flags: review?(jmuizelaar)
Comment on attachment 8726744 [details]
MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38221/diff/1-2/
Attachment #8726744 - Attachment description: MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current. r?jrmuizel → MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel
Attachment #8726744 - Flags: review?(jmuizelaar)
tracking, figuring we may want to uplift this after it lands.
Comment on attachment 8726744 [details]
MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel

https://reviewboard.mozilla.org/r/38221/#review35435

This LGTM, unless jrmuizel is on the hook for a particular reason.
Attachment #8726744 - Flags: review+
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> Comment on attachment 8726744 [details]
> MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the
> context to be current, so make those calls earlier. r?jrmuizel
> 
> https://reviewboard.mozilla.org/r/38221/#review35435
> 
> This LGTM, unless jrmuizel is on the hook for a particular reason.

I was trying to deal with time difference and get things taken care of earlier, but that backfired :)
David, any chance you could land this, or get it landed, for Milan given that he's still out for a few more days? Both Jeff's are at GDC IIRC, otherwise I would ask them...
Flags: needinfo?(dvander)
Yup, will do.
Flags: needinfo?(dvander)
https://hg.mozilla.org/mozilla-central/rev/b4a2342c8f5d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Attachment #8726744 - Flags: review?(jmuizelaar)
Upliftable? or not worth the risk?
Flags: needinfo?(milan)
Low risk, but I don't know what this looks like in the wild, outside the fuzzing, so it may be better to just let it ride 48?
Flags: needinfo?(milan)
(In reply to Milan Sreckovic [:milan] from comment #14)
> Low risk, but I don't know what this looks like in the wild, outside the
> fuzzing, so it may be better to just let it ride 48?

We should take this at least in Aurora47.
Comment on attachment 8726744 [details]
MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel

Approval Request Comment
[Feature/regressing bug #]: -
[User impact if declined]: Bad behavior, possible crashiness.
[Describe test coverage new/current, TreeHerder]: no specific tests
[Risks and why]: Very low. (well-understood)
[String/UUID change made/needed]: none
Attachment #8726744 - Flags: approval-mozilla-aurora?
Hi Jesse, can you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(jruderman)
Duplicate of this bug: 1247977
Also see bug 1247977 comment 14 and bug 1247977 comment 15 for indications that this is fixed.
Comment on attachment 8726744 [details]
MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel

Approval Request Comment
[Feature/regressing bug #]: 1221822.  This first shipped with 45.
[User impact if declined]: High frequency crash (see duplicated bug 1247977)
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low.  See aurora request comments as well.
[String/UUID change made/needed]:
Attachment #8726744 - Flags: approval-mozilla-beta?
Comment on attachment 8726744 [details]
MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel

(Potential) Crash fix, Aurora47+
Attachment #8726744 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Regarding the conversation in bug 1247977 that was originally marked as a duplicate of this, but has since one solitary crash since the fix - the steps to reproduce from this bug still pass, the one reproducible example we had locally for bug 1247977 still passes, and the configuration of the machine that did exhibit the crash is an unsupported one.  So, I'm still comfortable with the original user impact and risk claims for the uplifts.
Comment on attachment 8726744 [details]
MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel

Crash fix cleanup, ok to uplift for beta 9.
Attachment #8726744 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.