Passing 'undefined' explicitly to a JS function should utilize default parameter value

[frewsxcv]

I was looking into why this WPT test is failing. We specify a default value in the webidl. Currently, if we pass in undefined explicitly into the createTreeWalker function, it does not use the default parameter value (0xFFFFFFFF) and instead, it converts undefined to 0. If undefined gets passed in, we should use the default parameter value.

[mt2d2]

I took a stab at this with the help of mwu on IRC. We think that the value being converted by the spidermonkey long path is coming out as 0 without error, and established that neither the conversion routine nor spidermonkey has access to the default value. Thus, I think it's the responsibility of the caller to handle the the undefined case, which, here, is the binding code.

I'm not sure if this is the best way to do this. I would really appreciate input, since I don't understand the whole impact of changing the binding generator. Also would appreciate guidance on what tests should run.

./mach test-wpt using master:

Summary
=======

Ran 206865 tests (2488 parents, 204377 subtests)
Expected results: 206782
Unexpected results: 1 (CRASH: 1)
Skipped: 82

Unexpected Results
==================

/websockets/constructor/004.html
--------------------------------

undefined_conversion_binding

Summary
=======

Ran 206865 tests (2488 parents, 204377 subtests)
Expected results: 206781
Unexpected results: 2 (CRASH: 1, PASS: 1)
Skipped: 82

Unexpected Results
==================

/dom/traversal/TreeWalker-basic.html
------------------------------------
PASS expected FAIL Construct a TreeWalker by document.createTreeWalker(root, undefined, undefined).
/websockets/constructor/004.html
--------------------------------

[jdm]

FWIW I suspect the code that handles this in Gecko is http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#5460 .

[frewsxcv]

Also would appreciate guidance on what tests should run.

./mach test-wpt tests/wpt/web-platform-tests/dom/traversal/TreeWalker-basic.html

In particular, this test. The result of document.createTreeWalker(root, undefined, undefined) should be the same as document.createTreeWalker(root), but this is apparently not the case.

[frewsxcv]

Ignore my previous comment.

@mt2d2 I think the code you pasted above does the right thing, no?

Also, I updated it to work with the latest changes:

diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py
index a46c58f..760ebcd 100644
--- a/components/script/dom/bindings/codegen/CodegenRust.py
+++ b/components/script/dom/bindings/codegen/CodegenRust.py
@@ -1066,12 +1066,6 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
     if type.nullable():
         declType = CGWrapper(declType, pre="Option<", post=">")

-    template = (
-        "match FromJSValConvertible::from_jsval(cx, ${val}, %s) {\n"
-        "    Ok(v) => v,\n"
-        "    Err(_) => { %s }\n"
-        "}" % (conversionBehavior, exceptionCode))
-
     if defaultValue is not None:
         if isinstance(defaultValue, IDLNullValue):
             assert type.nullable()
@@ -1089,6 +1083,27 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
     else:
         defaultStr = None

+    if defaultStr is not None and defaultStr != "None":
+        # If null or undefined is explicitly passed into a parameter that has a
+        # default value specified in the WebIDL (and the default value is not
+        # None), we should ignore the null or undefined and use the default
+        # value.
+        template = (
+            "if ${val}.get().is_null_or_undefined() {\n"
+            "    %s\n"
+            "} else {\n"
+            "    match FromJSValConvertible::from_jsval(cx, ${val}, %s) {\n"
+            "        Ok(v) => v,\n"
+            "        Err(_) => { %s }\n"
+            "    }\n"
+            "}" % (defaultStr, conversionBehavior, exceptionCode))
+    else:
+        template = (
+            "match FromJSValConvertible::from_jsval(cx, ${val}, %s) {\n"
+            "    Ok(v) => v,\n"
+            "    Err(_) => { %s }\n"
+            "}" % (conversionBehavior, exceptionCode))
+
     return handleOptional(template, declType, defaultStr)

[mt2d2]

@frewsxcv yes, the key was use the default string if the value is 'undefined'. It did make this test pass, but I was worried about possible regressions. ./mach test-wpt came back clean (aside from a crash that happened on master, too). Changing the generator just seemed far-reaching to me. I'll happily bring the patch up-to-date if you guys agree with this approach. Thanks!

[Ms2ger]

The null part of

+        # If null or undefined is explicitly passed into a parameter that has a
+        # default value specified in the WebIDL (and the default value is not
+        # None), we should ignore the null or undefined and use the default
+        # value.

is not true. I'd be very concerned if that didn't cause any tests to fail.

[frewsxcv]

I was thinking that, but forgot to test it. I can create a regression test for it in web-platform-tests, unless we want to include it with the PR for this issue

[frewsxcv]

Regression test added in web-platform-tests/wpt#2058

[mt2d2]

@Ms2ger thank you for the insight! I can confirm that the previous implementation causes a failure on @frewsxcv new regression tests. I've taken both your input on commenting and null checking to produce a new patch that passes the target as well as the new regression tests.

I have a new spin that I'm working on validating against master. The important bit follows. If you think this is the correct approach, I can continue working on it toward final submission.

if defaultStr is not None and defaultStr != "None":
    # If undefined is explicitly passed into a parameter that has a default
    # value specified in the WebIDL (and the default value is not None), we
    # should ignore undefined and use the default value. The same does not
    # apply to null.
    template = (
        "if ${val}.get().is_undefined() {\n"
        "    %s\n"
        "} else {\n"
        "    match FromJSValConvertible::from_jsval(cx, ${val}, %s) {\n"
        "        Ok(v) => v,\n"
        "        Err(_) => { %s }\n"
        "    }\n"
        "}" % (defaultStr, conversionBehavior, exceptionCode))

[frewsxcv]

For me at least, it looks alright

[Ms2ger]

I'm very skeptical about anything that reads defaultStr.