912606 - hellorun fails to create WebGL context

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Ralph Giles (:rillian)]

Visiting http://hellorun.helloenjoy.com/ I get a log, wipe to blank and then a blank screen.

Console reports:

[10:49:10.350] TypeError: k is null @ http://hellorun.helloenjoy.com/js/three.min.js:412
[10:49:10.349] "THREE.WebGLRenderer" "58"
[10:49:10.349] "Error creating WebGL context."
[10:49:10.381] TypeError: this.view is undefined @ http://hellorun.helloenjoy.com/js/hellorun.min.js:1

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Windows as well.  Works in release.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Is this a regression? If so, we need regression range.

Comment 3

[Ralph Giles (:rillian)]

Confirmed on Mac. Works in Release, fails in Nightly (26) and Aurora (25.0a2).

Comment 4

[Ralph Giles (:rillian)]

and fails on Beta (24.0) on Mac.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Probably easiest and fastest to just debug context creation and see why it fails than go through finding a regression range.

Comment 6

[Martin Best (:mbest)]

Unreal and Pearl Boy demo are both running fine, in the version where the hellorun fails.

Comment 7

[bhavana bajaj [:bajaj]]

Tracking and Needinfo'ing :tracy to help out here to help with a regression window on this and see if any other common WebGL related sites are also impacted here to understand if this is a common issue or a site specific one ?

Comment 8

[bhavana bajaj [:bajaj]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5)
> Probably easiest and fastest to just debug context creation and see why it
> fails than go through finding a regression range.

:vlad, who can help with this drive from Engineering side ?

We have our second last beta for Fx24 going to build tomorrow and would like to get anything low risk in there if possible to avoid this issue.

Comment 9

[Tracy Walker [:tracy]]

this regressed on Nightly between:

Working - 2013-06-09-03-12-30
Broken - 2013-06-010-03-11-47

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Tracy, do you have changeset IDs from those builds?
about:buildconfig tells them.

Comment 11

[Ralph Giles (:rillian)]

I've been running a bisect in the background this afternoon, but it will be later tomorrow before I have a revision.

Comment 12

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/efbe547a7972
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130609 Firefox/24.0 ID:20130609031230
Bad:
http://hg.mozilla.org/mozilla-central/rev/252b1ac4d537
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130610 Firefox/24.0 ID:20130610020647
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=efbe547a7972&tochange=252b1ac4d537

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/22ed321f3fbe
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130607 Firefox/24.0 ID:20130607153547
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/02e3a57b61c5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130607 Firefox/24.0 ID:20130607174546
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=22ed321f3fbe&tochange=02e3a57b61c5

Suspected:
02e3a57b61c5	Jeff Gilbert — Bug 870232 - Enable 'webgl' requests for Desktop FF. - r=bjacob

Comment 13

[bhavana bajaj [:bajaj]]

Passing this on to Jeff given Bug 870232 is a suspect and also cc'ing :bjacob to get help here.

Comment 14

[Tracy Walker [:tracy]]

alice, thank you

Comment 15

[bhavana bajaj [:bajaj]]

Jeff,:bjacob can we backout Bug 870232 if there is no simple solution that can land for today's beta ?

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Martin pinged me already, but I'm in a complete rush for b2g 1.2 on bug 905227, I would rather have Jeff or someone else look into this bug rather than me. Sorry. If you need quicker action, you can ask :milan to find someone else in the gfx team.

To quickly answer your question though, it's not clear to me how backing this out will fix this bug, but if there is a strong reason to think so (like, a narrow regression window), then sure, it seems acceptable to back this out from beta.

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Benoit Jacob [:bjacob] from comment #16)
> Martin pinged me already, but I'm in a complete rush for b2g 1.2 on bug
> 905227, I would rather have Jeff or someone else look into this bug rather
> than me. Sorry. If you need quicker action, you can ask :milan to find
> someone else in the gfx team.
> 
> To quickly answer your question though, it's not clear to me how backing
> this out will fix this bug, but if there is a strong reason to think so
> (like, a narrow regression window), then sure, it seems acceptable to back
> this out from beta.

Thanks for looking :bjacob. I have an email out to Jeff,Milan to get help here.I suggested a backout of Bug 870232 as comment 12 suspects it might be the cause and might be the best possible solution in terms of timeline ? I might be wrong but may be investigation from :jeff can confirm that or best next steps.

Comment 18

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

I just tried and backing out bug 870232 does "fix" this.

Comment 19

[Tracy Walker [:tracy]]

I'm not able to reproduce this with several other webGL games or demos.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I am guessing all this would take is for someone to set a breakpoint in the WebGL context creation function and spend 5 minutes stepping through it to see what's going on.

Comment 21

[bhavana bajaj [:bajaj]]

Milan, Can we please perform the backout on mozilla-beta here asap given comment #18 or are there any concerns with it not being clean ?

Vlad, this might me a simple fix depending on the investigation but don't think it is possible to get it resolved within a couple of hours for today's beta hence the request for needed backout.If we find a very low risk solution for the problem by tomorrow which can be verified on nightly/aurora and we are confident it will not have fallout's I am happy to consider the forward fix+avoid backout for our final Beta, else this may just have to resolved in Fx25 timeframe.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Do we know this is our fault and not just a buggy web page?

Comment 23

[bhavana bajaj [:bajaj]]

(In reply to Olli Pettay [:smaug] from comment #22)
> Do we know this is our fault and not just a buggy web page?

Given this is not reproduced in release Fx23, not sure if its a website issue. Alternatively, I am not sure if there some web property change starting Fx24 which is causing this.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Well, the web site may have some FF specific code which doesn't expect
getContext("webgl") to return anything.

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

If we back this out, we will have to investigate to the bottom of why this site fails to run with that, because indeed we really do want to expose the "webgl" context ID (and increasingly, other, perfectly legitimate sites are going to fail to work if we do not support it).

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(5 min of stepping in debugger!)

Looks to be a problem with the site.

It first calls getContext with "webgl", and then it calls it later for the same canvas with "experimental-webgl".

But we may need to allow webgl and experimental-webgl to be used interchangeably.

Comment 27

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

Can we reach out to them to fix it? Vlad, how hard is to do what you're suggesting?  Either way, and based on Comment 25, it doesn't sound to me like the backout is the way to go.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #26)
> But we may need to allow webgl and experimental-webgl to be used
> interchangeably.

Checking what the spec actually says on this seems to be a prerequisite before we can decide on a patch and/or reach out to the website's developers.

Comment 29

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Milan Sreckovic [:milan] from comment #27)
> Can we reach out to them to fix it? Vlad, how hard is to do what you're
> suggesting?  Either way, and based on Comment 25, it doesn't sound to me
> like the backout is the way to go.

Trivial to fix in our code, especially if we just do it for webgl.  Backout doesn't make sense.

(In reply to Benoit Jacob [:bjacob] from comment #28)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #26)
> > But we may need to allow webgl and experimental-webgl to be used
> > interchangeably.
> 
> Checking what the spec actually says on this seems to be a prerequisite
> before we can decide on a patch and/or reach out to the website's developers.

The spec says the same string needs to be passed.  But pragmatically, we should allow this.  We *should* also reach out to the website's developers, though.

Comment 30

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #29)
> (In reply to Milan Sreckovic [:milan] from comment #27)
> > Can we reach out to them to fix it? Vlad, how hard is to do what you're
> > suggesting?  Either way, and based on Comment 25, it doesn't sound to me
> > like the backout is the way to go.
> 
> Trivial to fix in our code, especially if we just do it for webgl.  Backout
> doesn't make sense.
> 
> (In reply to Benoit Jacob [:bjacob] from comment #28)
> > (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #26)
> > > But we may need to allow webgl and experimental-webgl to be used
> > > interchangeably.
> > 
> > Checking what the spec actually says on this seems to be a prerequisite
> > before we can decide on a patch and/or reach out to the website's developers.
> 
> The spec says the same string needs to be passed.  But pragmatically, we
> should allow this.  We *should* also reach out to the website's developers,
> though.

At the very least, we should emit a warning when they try to access with a different string than the one the created it with.

Comment 31

[Kelsey Gilbert [:jgilbert]]

In the long term, we should *not* allow interchangeable use, since the strings have different meanings.
If we're very concerned about breaking a bunch of content, we can temporarily make it interchangeable with a big DEPRECATED warning when it's used.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Also, this clearly needs a conformance test if Chrome appears to work. (On a machine where Chrome will give out "webgl" contexts, which is not all machines!)

Comment 33

[Ralph Giles (:rillian)]

In http://hellorun.helloenjoy.com/js/hellorun.min.js I see Detector.webgl try document.createElement.getContext('experimental-webgl'); but the actual context creation is getcontext('webgl')||getContext('experimental-webgl');

Detector.webgl only creates the canvas if window.WebGLRenderingContext exists, which is what Alice's commit introduces. Adding a getContext('webgl') to the Detector check should fix the site. OTOH, that doesn't match with the call sequence vlad saw in comment #26, so maybe I missed something.

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Allow deprecated requests for mismatched webgl ids

Allow for deprecated requests of the wrong type.

I'm still not sure we want to take this, as it's absolutely non-spec, and this issue should be solved through developer engagement.

I'll be posting a patch that we *should* take at least on nightly that warns when a user tries this, but does not allow it.

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Add test for deprecated mismatched webgl request ids.

Comment 36

[Kelsey Gilbert [:jgilbert]]

For some reason, this is not actually triggering a JS warning. Not sure why yet. It works otherwise, though.

Comment 37

[Kelsey Gilbert [:jgilbert]]

Nevermind, this works, but JS warnings aren't triggered by the web console. (weird)

Comment 38

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Warn when the wrong request id is used, and disallow.

This continues to disallow invalid requests, and emits a JS warning when a user tries.

Comment 39

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Test that mismatched requests fail

Comment 40

[Kelsey Gilbert [:jgilbert]]

With the new warning, this shows up in the web console for the test page:
[16:08:14.743] Error: WebGL: Retrieving a WebGL context from a canvas via a request id ('experimental-webgl') different from the id used to create the context ('webgl') is not allowed. @ http://hellorun.helloenjoy.com/js/three.min.js:412

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 800451 [details] [diff] [review]
patch: Warn when the wrong request id is used, and disallow.

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

::: content/html/content/src/HTMLCanvasElement.cpp
@@ +784,5 @@
> +IsContextIdWebGL(const nsAString& str)
> +{
> +  return str.EqualsLiteral("webgl") ||
> +         str.EqualsLiteral("experimental-webgl") ||
> +         str.EqualsLiteral("moz-webgl");

Time to drop moz-webgl. Nobody should have been using it for what, 3 years now.

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 800453 [details] [diff] [review]
patch: Test that mismatched requests fail

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

By the way, non-conf-test is a bad name for that directory! 'conf' could be an abbreviation for other things than 'conformance', and 'non-conformance-test' could be mis-parsed as "test that we are non-conformant" !

Comment 43

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #41)
> Comment on attachment 800451 [details] [diff] [review]
> patch: Warn when the wrong request id is used, and disallow.
> 
> Review of attachment 800451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/HTMLCanvasElement.cpp
> @@ +784,5 @@
> > +IsContextIdWebGL(const nsAString& str)
> > +{
> > +  return str.EqualsLiteral("webgl") ||
> > +         str.EqualsLiteral("experimental-webgl") ||
> > +         str.EqualsLiteral("moz-webgl");
> 
> Time to drop moz-webgl. Nobody should have been using it for what, 3 years
> now.

I agree. I filed bug 913597 for doing deprecation and removal.

Comment 44

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #42)
> Comment on attachment 800453 [details] [diff] [review]
> patch: Test that mismatched requests fail
> 
> Review of attachment 800453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> By the way, non-conf-test is a bad name for that directory! 'conf' could be
> an abbreviation for other things than 'conformance', and
> 'non-conformance-test' could be mis-parsed as "test that we are
> non-conformant" !

Alright. Let's do that in bug 913604.

Comment 45

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5eec009b3020
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5184488ec31a

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5eec009b3020
https://hg.mozilla.org/mozilla-central/rev/5184488ec31a

Comment 47

[bhavana bajaj [:bajaj]]

This has been leaning more towards a Tech Evangelism issue. I have already done some outreach to hellorun on their basic support side to get eyes on this bug, if anyone here has specific contacts we should follow-up.

For FX24 I spoke offline with Jeff and I don't think we are landing any fwd fix(Allow for deprecated requests of the wrong type) from our side as our implementation matches the spec.Please correct me if this does not match to anyone's expectation here as we go to build with our final Fx24 beta today.

Comment 48

[Ralph Giles (:rillian)]

Works for me.

Comment 49

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ack - imo we should land fixes as forward as we can. We absolutely do not need any more reasons for developers or users to assume content, especially webgl, doesn't work in Firefox but works in Chrome, no matter how in the "right" we are.

Comment 50

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #49)
> Ack - imo we should land fixes as forward as we can. We absolutely do not
> need any more reasons for developers or users to assume content, especially
> webgl, doesn't work in Firefox but works in Chrome, no matter how in the
> "right" we are.

It should be 'broken' in Chrome soon, too. I'll talk with Ken about it.

Comment 51

[bhavana bajaj [:bajaj]]

hellorun just got back to me that they have landed a solution which will fix the reported issue in this bug.
Adding verifyme for QA or anyone else who could help to confirm that that the reported STR work on all channels(nightly,aurora,beta) as expected now.

Comment 52

[Tracy Walker [:tracy]]

wfm on 24beta10 and Aurora and Nightly from 20130913

Comment 53

[Ralph Giles (:rillian)]

I've verified the issue no longer occurs with this website in Nightly, Aurora or Beta on Mac.