1228687 - "Assertion failure: gl->IsCurrent()"

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1228687: ScopedResolveTexturesForDraw needs the context to be current, so make those calls earlier. r?jrmuizel

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

Comment 4

[Kelsey Gilbert [:jgilbert]]

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.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

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/

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

tracking, figuring we may want to uplift this after it lands.

Comment 7

[Kelsey Gilbert [:jgilbert]]

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.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(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 :)

Comment 9

[Johnny Stenback (:jst)]

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...

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yup, will do.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a2342c8f5d

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4a2342c8f5d

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Upliftable? or not worth the risk?

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 15

[Kelsey Gilbert [:jgilbert]]

(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 16

[Kelsey Gilbert [:jgilbert]]

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

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jesse, can you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Also see bug 1247977 comment 14 and bug 1247977 comment 15 for indications that this is fixed.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

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]:

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cdacdc1e3247

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

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 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/711c5b868eda