Closed
Bug 912606
Opened 11 years ago
Closed 11 years ago
hellorun fails to create WebGL context
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
VERIFIED
FIXED
mozilla26
People
(Reporter: rillian, Assigned: jgilbert)
References
()
Details
(Whiteboard: [parity-chrome])
Attachments
(4 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•11 years ago
|
Whiteboard: [parity-chrome]
Windows as well. Works in release.
Updated•11 years ago
|
status-firefox23:
--- → unaffected
Updated•11 years ago
|
status-firefox24:
--- → affected
Updated•11 years ago
|
status-firefox25:
--- → affected
status-firefox26:
--- → affected
Comment 2•11 years ago
|
||
Is this a regression? If so, we need regression range.
Reporter | ||
Comment 3•11 years ago
|
||
Confirmed on Mac. Works in Release, fails in Nightly (26) and Aurora (25.0a2).
Reporter | ||
Comment 4•11 years ago
|
||
and fails on Beta (24.0) on Mac.
Reporter | ||
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Probably easiest and fastest to just debug context creation and see why it fails than go through finding a regression range.
Updated•11 years ago
|
Comment 6•11 years ago
|
||
Unreal and Pearl Boy demo are both running fine, in the version where the hellorun fails.
Comment 7•11 years ago
|
||
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)
Updated•11 years ago
|
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
this regressed on Nightly between:
Working - 2013-06-09-03-12-30
Broken - 2013-06-010-03-11-47
Flags: needinfo?(twalker)
Updated•11 years ago
|
Keywords: qawanted,
regressionwindow-wanted
Comment 10•11 years ago
|
||
Tracy, do you have changeset IDs from those builds?
about:buildconfig tells them.
Flags: needinfo?(twalker)
Reporter | ||
Comment 11•11 years ago
|
||
I've been running a bisect in the background this afternoon, but it will be later tomorrow before I have a revision.
Comment 12•11 years ago
|
||
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•11 years ago
|
||
Passing this on to Jeff given Bug 870232 is a suspect and also cc'ing :bjacob to get help here.
Assignee: nobody → jgilbert
Comment 15•11 years ago
|
||
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)
Comment 16•11 years ago
|
||
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)
Comment 17•11 years ago
|
||
(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•11 years ago
|
||
I just tried and backing out bug 870232 does "fix" this.
Comment 19•11 years ago
|
||
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.
Comment 21•11 years ago
|
||
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•11 years ago
|
||
Do we know this is our fault and not just a buggy web page?
Comment 23•11 years ago
|
||
(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•11 years ago
|
||
Well, the web site may have some FF specific code which doesn't expect
getContext("webgl") to return anything.
Comment 25•11 years ago
|
||
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.
Comment 27•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 30•11 years ago
|
||
(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)
Assignee | ||
Comment 31•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 32•11 years ago
|
||
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!)
Reporter | ||
Comment 33•11 years ago
|
||
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.
Assignee | ||
Comment 34•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #800383 -
Attachment description: cross-webgl-exp → patch: Allow deprecated requests for mismatched webgl ids
Assignee | ||
Comment 35•11 years ago
|
||
Assignee | ||
Comment 36•11 years ago
|
||
For some reason, this is not actually triggering a JS warning. Not sure why yet. It works otherwise, though.
Assignee | ||
Comment 37•11 years ago
|
||
Nevermind, this works, but JS warnings aren't triggered by the web console. (weird)
Assignee | ||
Comment 38•11 years ago
|
||
This continues to disallow invalid requests, and emits a JS warning when a user tries.
Attachment #800451 -
Flags: review?(bjacob)
Assignee | ||
Comment 39•11 years ago
|
||
Attachment #800453 -
Flags: review?(bjacob)
Assignee | ||
Comment 40•11 years ago
|
||
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•11 years ago
|
||
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 42•11 years ago
|
||
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+
Assignee | ||
Comment 43•11 years ago
|
||
(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.
Assignee | ||
Comment 44•11 years ago
|
||
(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.
Assignee | ||
Comment 45•11 years ago
|
||
Comment 46•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5eec009b3020
https://hg.mozilla.org/mozilla-central/rev/5184488ec31a
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 47•11 years ago
|
||
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.
Reporter | ||
Comment 48•11 years ago
|
||
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.
Assignee | ||
Comment 50•11 years ago
|
||
(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•11 years ago
|
||
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
Comment 52•11 years ago
|
||
wfm on 24beta10 and Aurora and Nightly from 20130913
Status: RESOLVED → VERIFIED
Keywords: verifyme
Reporter | ||
Comment 53•11 years ago
|
||
I've verified the issue no longer occurs with this website in Nightly, Aurora or Beta on Mac.
Attachment #800383 -
Flags: feedback?(vladimir)
You need to log in
before you can comment on or make changes to this bug.
Description
•