Closed Bug 1617512 Opened 1 month ago Closed 1 month ago

Categories

(Core :: Canvas: WebGL, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla75
Tracking Status
firefox-esr68 --- unaffected
firefox73 --- unaffected
firefox74 + wontfix
firefox75 --- verified

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

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

Another likely regression from bug 1477756.

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

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?

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.

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)

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

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

Tracking for 74 as this is a new P1 regression.

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

Attachment #9128953 - Attachment is obsolete: true
Pushed by jgilbert@mozilla.com:
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.

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

Backout by aiakab@mozilla.com:
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

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

Flags: needinfo?(jgilbert)

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

Attachment #9128952 - Attachment description: Bug 1617512 - Fix WebGLSL comment parsing regex. → Bug 1617512 - Fix WebGLSL comment parsing code.

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

Flags: needinfo?(jgilbert)
Pushed by jgilbert@mozilla.com:
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 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
Flags: needinfo?(jgilbert)
Attachment #9128952 - Flags: approval-mozilla-beta?
Attachment #9128959 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

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

Attachment #9128952 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9128959 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: in-testsuite+
Flags: needinfo?(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).

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+ → qe-verify-
Regressions: 1620876
Flags: needinfo?(jgilbert)
Attachment #9128959 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.