Bug 240219 - foo(optional unsigned long long offset = 0) IDL functions don't work when called like foo(undefined) or foo()
Summary: foo(optional unsigned long long offset = 0) IDL functions don't work when cal...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 232558
  Show dependency treegraph
 
Reported: 2022-05-08 16:36 PDT by Myles C. Maxfield
Modified: 2022-09-07 22:41 PDT (History)
3 users (show)

See Also:


Attachments
Test file (478 bytes, text/html)
2022-05-15 11:47 PDT, Myles C. Maxfield
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2022-05-08 16:36:21 PDT
In WebGPU, see mapAsync() and getMappedRange(). The C++ version of these functions never get called when the optional parameters with default values are specified by JS as undefined or omitted.
Comment 1 Chris Dumez 2022-05-08 16:51:23 PDT
I find his very surprising. Pretty sure we already have similar IDL in the code Base. I’d expect our generated code to call the implementation function with 0 if the parameter is omitted or undefined.
Comment 2 Chris Dumez 2022-05-12 18:11:08 PDT
I have just double-checked here and things seem to work as expected. I validated with the following patch:
```
diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index 0636bd5b4363..370a789c0a9e 100644
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -9202,6 +9202,11 @@ bool Document::isSameSiteForCookies(const URL& url) const
     return domain.matches(url);
 }
 
+void Document::testOptionalUnsignedLongLong(uint64_t param)
+{
+    WTFLogAlways("CHRIS: Document::testOptionalUnsignedLongLong(%llu)", param);
+}
+
 } // namespace WebCore
 
 #undef DOCUMENT_RELEASE_LOG
diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h
index 866abc6d7986..fcc1319cadb0 100644
--- a/Source/WebCore/dom/Document.h
+++ b/Source/WebCore/dom/Document.h
@@ -699,6 +699,8 @@ public:
 
     void cancelParsing();
 
+    void testOptionalUnsignedLongLong(uint64_t);
+
     ExceptionOr<void> write(Document* entryDocument, SegmentedString&&);
     WEBCORE_EXPORT ExceptionOr<void> write(Document* entryDocument, FixedVector<String>&&);
     WEBCORE_EXPORT ExceptionOr<void> writeln(Document* entryDocument, FixedVector<String>&&);
diff --git a/Source/WebCore/dom/Document.idl b/Source/WebCore/dom/Document.idl
index b5cc4978066a..db9726d66df7 100644
--- a/Source/WebCore/dom/Document.idl
+++ b/Source/WebCore/dom/Document.idl
@@ -94,6 +94,8 @@ typedef (HTMLScriptElement or SVGScriptElement) HTMLOrSVGScriptElement;
 
     // Non standard: It has been dropped from Blink already.
     RenderingContext? getCSSCanvasContext(DOMString contextId, DOMString name, long width, long height);
+
+    undefined testOptionalUnsignedLongLong(optional unsigned long long parameter = 0);
 };
 
 enum DocumentReadyState { "loading", "interactive", "complete" };
```

document.testOptionalUnsignedLongLong() -> implementation called with 0
document.testOptionalUnsignedLongLong(0) -> implementation called with 0
document.testOptionalUnsignedLongLong(1) -> implementation called with 1
document.testOptionalUnsignedLongLong(undefined) -> implementation called with 0

Works as expected IMO.
Comment 3 Myles C. Maxfield 2022-05-13 15:01:12 PDT
Cool, thanks for trying it out. I'll try to figure out why the WebGPU stuff isn't working then.
Comment 4 Myles C. Maxfield 2022-05-15 11:47:12 PDT
Created attachment 459381 [details]
Test file

Here is a test file demonstrating the problem. (You have to open it in DumpRenderTree; it's not hooked up into WK2 yet, and is intentionally disabled in Minibrowser.) The IDL has:

Promise<undefined> mapAsync(GPUMapModeFlags mode, optional GPUSize64 offset = 0, optional GPUSize64 size);

And if you call it like

buffer.mapAsync(GPUMapMode.WRITE);

then our generated bindings code does this:

auto offset = convert<IDLEnforceRangeAdaptor<IDLUnsignedLongLong>>(*lexicalGlobalObject, argument1.value());

which gets sent to:

double JSValue::toNumberSlowCase(JSGlobalObject* globalObject) const
{
    ...
    return isUndefined() ? PNaN : 0; // null and false both convert to 0.
}

and then sent to

static double enforceRange(JSGlobalObject& lexicalGlobalObject, double x, double minimum, double maximum)
{
    ...
    if (std::isnan(x) || std::isinf(x)) {
        throwTypeError(&lexicalGlobalObject, scope, rangeErrorString(x, minimum, maximum));
        return 0;
    }
    ...
}
Comment 5 Radar WebKit Bug Importer 2022-05-15 16:37:12 PDT
<rdar://problem/93317646>
Comment 6 Myles C. Maxfield 2022-05-17 18:03:12 PDT
Pull request: https://github.com/WebKit/WebKit/pull/710
Comment 7 Ahmad Saleem 2022-09-03 04:04:05 PDT
In Pull Request from Comment 06, there is mention that it was landed as part of following:

https://trac.webkit.org/changeset/294373/webkit

Is anything else needed here?

BY THE WAY - the test file show "failure" in all browsers (Chrome Canary 107, Firefox Nightly 106 and Safari Technology Preview 152).
Comment 8 Myles C. Maxfield 2022-09-07 22:41:16 PDT
Yes, there is more to do here. The bug itself isn't fixed; that pull request just worked around the problem.