Closed Bug 1838694 Opened 1 year ago Closed 1 year ago

`GPUError` hierarchy in WebGPU is not up to spec.

Categories

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

defect

Tracking

()

RESOLVED FIXED
117 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox-esr115 --- unaffected
firefox114 --- unaffected
firefox115 --- unaffected
firefox116 --- disabled
firefox117 --- fixed

People

(Reporter: ErichDonGubler, Assigned: ErichDonGubler)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Bug 1837557 actually got WebGPU logic far enough that we are actually trying to return errors into error scopes! πŸ™ŒπŸ» Unfortunately…some of that is broken. In particular, we're out-of-date for the GPUError hierarchy. We currently represent GPUError as a union of types (permalink):

typedef (GPUOutOfMemoryError or GPUValidationError) GPUError; # do not want :(

…but GPUError has changed to be a parent interface of these, further adding the "internal"/GPUInternalError variant to the GPUError API (permalink; see also the official spec's sections on GPUError):

# This should be the shape of new code: 

interface GPUError {
    readonly attribute DOMString message;
};

interface GPUValidationError
        : GPUError { # N.B.: now extends `GPUError`
    constructor(DOMString message);
};

interface GPUOutOfMemoryError
        : GPUError { # N.B.: now extends `GPUError`
    constructor(DOMString message);
};

interface GPUInternalError # new
        : GPUError {
    constructor(DOMString message);
};

enum GPUErrorFilter {
    "validation",
    "out-of-memory",
    "internal", # new
};

This bug's scope is to bring this API back up to spec.

Set release status flags based on info from the regressing bug 1837557

:jgilbert, since you are the author of the regressor, bug 1837557, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

One motivating issue for this bug being done now is that bug 1831263 is running into an issue that starts at dom/webgpu/Device.cpp:378 where the out-of-memory variant is not being initialized properly; see this Try push for an example of failures. D'oh!

EDIT: :jgilbert filed bug 1838703 so we have more compiler assistance with avoiding this in the future. EDIT 2: Ah, the motivating issue will be handled there instead of here. πŸ˜† Noice!

Assignee: nobody → egubler
Severity: -- → S3
Priority: -- → P1
Flags: needinfo?(jgilbert)

This bug is true, but the root cause for the regression was not this heirarchy, but rather bug 1838739.

Status: NEW → ASSIGNED
No longer blocks: webgpu-v1-cts-windows

Looks like this will make some CTS tests pass because the internal error variant will finally be available. For example, this test is currently failing because of it: https://gpuweb.github.io/cts/standalone/?q=webgpu:api,validation,encoding,createRenderBundleEncoder:attachment_state,limits,maxColorAttachments:*

CC :jimb, who wrote the test_too_many_color_attachments.ini file that I'm blood-sworn to slay bent on deleting once this test passes. πŸ™ƒ

Priority: P1 → P3

From https://bugzilla.mozilla.org/show_bug.cgi?id=1838694#c3 , marking the regressor as bug 1838739. Please feel free to remove if that is not the case.

Regressed by: 1838739
No longer blocks: 1836479
Priority: P3 → P1

TODO: Validate that the previous commit actually unblocks the
maxColorAttachments test in WPT.

Depends on D181690

Attachment #9340389 - Attachment description: WIP: Bug 1838694: fix(webgpu): impl. correct `GPUError` types r?#webgpu-reviewers → Bug 1838694: fix(webgpu): impl. correct `GPUError` types r?#webgpu-reviewers,jgilbert

Depends on D181690

Attachment #9340390 - Attachment description: WIP: Bug 1838694: test(webgpu): remove `test_too_many_color_attachments` r?#webgpu-reviewers,jimb → Bug 1838694: test(webgpu): remove `test_too_many_color_attachments` r?#webgpu-reviewers,jimb
Blocks: 1840179

For my own benefit: There's quite a few Try builds I pushed getting this tested: 1, 2, 3, 4, 5, 6

As a reminder soft code freeze for 116 starts tomorrow in case this still needed to land for 116

Pushed by egubler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/109ebd10d0c3
fix(webgpu): impl. correct `GPUError` types r=jgilbert,webgpu-reviewers,webidl,peterv
https://hg.mozilla.org/integration/autoland/rev/f6477420370e
fix(webgpu): only set `PopErrorScopeResult::resultType` in `Device::PopErrorScope` if an error was actually found r=webgpu-reviewers,jgilbert
https://hg.mozilla.org/integration/autoland/rev/892fed91da4e
test(webgpu): remove `test_too_many_color_attachments` r=jimb,webgpu-reviewers

Backed out for causing mochitests failures in test_interfaces_secureContext.html.

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_interfaces_secureContext.html | If this is failing: DANGER, are you sure you want to expose the new interface GPUInternalError to all webpages as a property on 'window'? Do not make a change to this file without a review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals)
Flags: needinfo?(egubler)

I have investigated, and pushed up a fix for the errors governing the decision to back out. I had added new interfaces accessible from secure contexts; this test's interface snapshot in dom/tests/mochitest/general/test_interface.js doesn't contain GPUError or GPUInternalError yet. CC :peterv, who I'll also tag in the relevant D181690.

Flags: needinfo?(egubler)
Pushed by egubler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83d3204888fd
fix(webgpu): impl. correct `GPUError` types r=jgilbert,webgpu-reviewers,webidl,peterv
https://hg.mozilla.org/integration/autoland/rev/7fdcca116760
fix(webgpu): only set `PopErrorScopeResult::resultType` in `Device::PopErrorScope` if an error was actually found r=webgpu-reviewers,jgilbert
https://hg.mozilla.org/integration/autoland/rev/8f7835cd7d0c
test(webgpu): remove `test_too_many_color_attachments` r=jimb,webgpu-reviewers

Set release status flags based on info from the regressing bug 1838739

Backed out for causing webgpu failures.
This seems to affect only Windows 11 x64/x86.

Flags: needinfo?(egubler)

Investigating!

Flags: needinfo?(egubler)
Blocks: 1842020
Pushed by egubler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f697f6c07e36
fix(webgpu): impl. correct `GPUError` types r=jgilbert,webgpu-reviewers,webidl,peterv
https://hg.mozilla.org/integration/autoland/rev/f08b314b5972
fix(webgpu): only set `PopErrorScopeResult::resultType` in `Device::PopErrorScope` if an error was actually found r=webgpu-reviewers,jgilbert
https://hg.mozilla.org/integration/autoland/rev/7555a6077330
test(webgpu): remove `test_too_many_color_attachments` r=jimb,webgpu-reviewers
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch

The patch landed in nightly and beta is affected.
:ErichDonGubler, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox116 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(egubler)
Flags: needinfo?(egubler)
Blocks: 1838551
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: