1617512 - [NSFW] WebGL game broken: https://love-joint-games.com/DfD/public/ch14/1.0.0/

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Kelsey Gilbert [:jgilbert]]

Via webcompat: https://github.com/webcompat/web-bugs/issues/48441

Another likely regression from bug 1477756.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: shader source

For the record it works in Chrome.

On load in affected Firefox, there's an alert() box with "Could not compile vertex shader:". Pressing OK will dump the shader's source to console.log(). (Thanks devs!)

Also in the console is a warning from Gecko: (thanks me!) 

> Error: WebGL warning: shaderSource: `source` contains illegal character 0x5c.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Like 76 of the shader itself (not counting "Begin Shader" header) is:

		// Normally we'd have the light positions\directions back-transformed from world to object space

I guess we don't have CTS tests for this?

Comment 3

[Kelsey Gilbert [:jgilbert]]

While this is definitely not supported by ESSL100, WebGL 1 is laxer:

The exception is that any character allowed in an HTML DOMString [DOMSTRING] may be used in GLSL comments. Such use must not generate an error.

Comment 4

[Kelsey Gilbert [:jgilbert]]

I think we don't specify whether that implies essl300 style \-line-continuation of // line-comments.
Rules-as-written, I don't think they should line-continue in WebGL 1. However, it's preferable to spec that they match essl300's behavior. (I really don't want more comment regexes)

Comment 5

[Kelsey Gilbert [:jgilbert]]

Oh my bad, this isn't line-continuing in this case, but that might still be a problem.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: SFW testcase backslash-in-glsl-comment.html

Comment 7

[Kelsey Gilbert [:jgilbert]]

[Tracking Requested - why for this release]:
Broken content due to regression.

Comment 8

[Kelsey Gilbert [:jgilbert]]

https://github.com/KhronosGroup/WebGL/pull/3028

Comment 9

[Pascal Chevrel:pascalc]

Tracking for 74 as this is a new P1 regression.

Comment 10

[Kelsey Gilbert [:jgilbert]]

The live conformance test:
https://www.khronos.org/registry/webgl/sdk/tests/conformance/glsl/preprocessor/comments.html?webglVersion=1&quiet=0&quick=1

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1617512 - Fix WebGLSL comment parsing code.

Test: https://www.khronos.org/registry/webgl/sdk/tests/conformance/glsl/preprocessor/comments.html?webglVersion=1&quiet=0&quick=1

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1617512 - Update CTS checkout to pick up WebGLSL comment parsing test.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1617512 - Delete dom/canvas/test/webgl-conf/generated and regenerate.

There were a bunch of orphaned files, and this cleans them out.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ccaa83aabee7
Fix WebGLSL comment parsing regex. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/ceff9529d28c
Delete dom/canvas/test/webgl-conf/generated and regenerate.

Comment 15

[Kelsey Gilbert [:jgilbert]]

With this fix, Firefox gets past the loading screen, and no longer has the error.

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dca658411cd5
Backed out 2 changesets for causing mochitest failures on test_conformance__ogles__GL__build__build_049_to_056.html

Comment 17

[Arthur Iakab [arthur_iakab]]

Backed out 2 changesets (bug 1617512) for causing mochitest failures on test_conformance__ogles__GL__build__build_049_to_056.html

Backout revision https://hg.mozilla.org/integration/autoland/rev/dca658411cd57be17476657c19925ee6ebddb5bf

Failures https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=290475536&fromchange=0aad2ca3e74b1c597d2042a35f5531373078dc24&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Ctest-linux1804-64%2Fdebug-mochitest-webgl2-ext-e10s-4%2Cm%28gl2e4%29

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=ceff9529d28ca291901ea312e2a359e9814b97bd

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=290475492&repo=autoland

Jeff can you please take a look?

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase-msvc-regex-nongreedy-eol.cpp

On MSVC, this fails on i = '\n', where it yields a match of "/* ".
There are no mismatches when compile on my Linux clang env.

Comment 19

[Kelsey Gilbert [:jgilbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b74caa13cf516eee2ae00a6b6729031a0bc2bf71

Comment 20

[Kelsey Gilbert [:jgilbert]]

Upstreaming more CTS subtests too: https://github.com/KhronosGroup/WebGL/pull/3029

Comment 21

[Pascal Chevrel:pascalc]

Jeff, we are out of beta now and we build RC on Monday. Is that bug important enough to require an uplift in our RC build or a driver for a RC2 next week? Thanks

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30bbb50fff90
Fix WebGLSL comment parsing code. r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/64793894a3a7
Delete dom/canvas/test/webgl-conf/generated and regenerate.

Comment 23

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 9128952 [details]
Bug 1617512 - Fix WebGLSL comment parsing code.

Beta/Release Uplift Approval Request

• User impact if declined: Broken WebGL content (e.g. the link from this bug)
  I think we should try to uplift because I'm worried about this regression breaking other content
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Testcase (sfw): https://bug1617512.bmoattachments.org/attachment.cgi?id=9128672
  Should show "PASS", not "FAIL". Output should match Chrome.

The NSFW STR is linked from this bug as well.
Before this patch, the game errors and makes an alert dialog box that mentions shader compilation failure.
After this patch, the game should load, like it Chrome.

• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Medium risk: This touches some gross code, though I think it's a great simplification that's easier to understand.
  I am worried about jumping straight into the RC, but I think we should trust our now-even-better tests and try for it.
• String changes made/needed: none

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/30bbb50fff90
https://hg.mozilla.org/mozilla-central/rev/64793894a3a7

Comment 25

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Reproduced the issue with Firefox Nightly 75.0a1 (2020-02-23) under Windows 10 using the SFW testcase.

The issue is fixed on Nightly 75.0a1 (2020-03-01). Tests were performed on Windows 10, Ubuntu 18.04 and macOS 10.15.

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9128952 [details]
Bug 1617512 - Fix WebGLSL comment parsing code.

P1 regression in 74, has tests and was verified by QA in nightly, let's take it to the beta branch before RC, thanks.

Comment 27

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on canvas/test/webgl-conf/generated-mochitest.ini.

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/609993945f749197ee7cb2d45eed356053b187f4
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=291231203&repo=mozilla-beta&lineNumber=1403

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Re-pushed with the test in question un-removed from the second commit since it was still referenced in the manifest.

https://hg.mozilla.org/releases/mozilla-beta/rev/6011727e1d41
https://hg.mozilla.org/releases/mozilla-beta/rev/56347334409f

Comment 29

[Kelsey Gilbert [:jgilbert]]

Oh, oops, I didn't mean to uplift the second patch. It's an unused test file clean up, so it's not bad to have (or not).

Comment 30

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Reproduced the issue with Firefox Nightly 75.0a1 (2020-02-23) under Windows 10 using the SFW testcase.

The issue is fixed on 74.0(ID:20200302184608) . Tests were performed on Windows 10, Ubuntu 18.04 and macOS 10.15.

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out on request by pascalc for causing bug 1620876:
https://hg.mozilla.org/releases/mozilla-release/rev/c6e493873ba543907dadabb42b64208a3679d43d