hellorun fails to create WebGL context

VERIFIED FIXED in Firefox 24

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: rillian, Assigned: jgilbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox23 unaffected, firefox24+ verified, firefox25+ verified, firefox26+ verified)

Details

(Whiteboard: [parity-chrome], )

Attachments

(4 attachments)

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
Whiteboard: [parity-chrome]
Blocks: 912610
Windows as well.  Works in release.
Is this a regression? If so, we need regression range.
Confirmed on Mac. Works in Release, fails in Nightly (26) and Aurora (25.0a2).
and fails on Beta (24.0) on Mac.
Probably easiest and fastest to just debug context creation and see why it fails than go through finding a regression range.
Unreal and Pearl Boy demo are both running fine, in the version where the hellorun fails.
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 ?
Flags: needinfo?(twalker)
(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.
this regressed on Nightly between:

Working - 2013-06-09-03-12-30
Broken - 2013-06-010-03-11-47
Flags: needinfo?(twalker)
Tracy, do you have changeset IDs from those builds?
about:buildconfig tells them.
Flags: needinfo?(twalker)
I've been running a bisect in the background this afternoon, but it will be later tomorrow before I have a revision.
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
Passing this on to Jeff given Bug 870232 is a suspect and also cc'ing :bjacob to get help here.
Assignee: nobody → jgilbert
alice, thank you
Flags: needinfo?(twalker)
Jeff,:bjacob can we backout Bug 870232 if there is no simple solution that can land for today's beta ?
Flags: needinfo?(jgilbert)
Flags: needinfo?(bjacob)
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.
Flags: needinfo?(bjacob)
(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.
I just tried and backing out bug 870232 does "fix" this.
I'm not able to reproduce this with several other webGL games or demos.
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.
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.
Blocks: 870232
Do we know this is our fault and not just a buggy web page?
(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.
Well, the web site may have some FF specific code which doesn't expect
getContext("webgl") to return anything.
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).
(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.
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.
(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.
(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.
(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.
Flags: needinfo?(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.
OS: Linux → All
Hardware: x86_64 → All
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!)
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.
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.
Attachment #800383 - Flags: feedback?(vladimir)
Attachment #800383 - Attachment description: cross-webgl-exp → patch: Allow deprecated requests for mismatched webgl ids
For some reason, this is not actually triggering a JS warning. Not sure why yet. It works otherwise, though.
Nevermind, this works, but JS warnings aren't triggered by the web console. (weird)
This continues to disallow invalid requests, and emits a JS warning when a user tries.
Attachment #800451 - Flags: review?(bjacob)
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 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.
Attachment #800451 - Flags: review?(bjacob) → review+
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" !
Attachment #800453 - Flags: review?(bjacob) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/5eec009b3020
https://hg.mozilla.org/mozilla-central/rev/5184488ec31a
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Works for me.
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.
(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.
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.
Keywords: verifyme
wfm on 24beta10 and Aurora and Nightly from 20130913
I've verified the issue no longer occurs with this website in Nightly, Aurora or Beta on Mac.
You need to log in before you can comment on or make changes to this bug.