DomStringMap::get() throws an unexpected error

[j-devel]

Describe the Bug

DomStringMap::get() throws an unexpected error when non-existent key string is passed as argument.

Steps to Reproduce

1. Build the todomvc example:

$ cd examples/todomvc
$ npm i && npm run serve

2. Visit the locally served webpage, open dev console, and add some todo items.
3. Click on 'x' to remove one of the items.

Expected Behavior

The remove operation should be successful without errors.

Actual Behavior

The output is errors starting with:

index.js:98 wasm-bindgen: imported JS function that was not marked as `catch` threw an error: expected a string argument

Stack:
Error: expected a string argument
    at passStringToWasm (webpack:///./pkg/index.js?:225:45)
    at Module.__widl_f_get_DOMStringMap (webpack:///./pkg/index.js?:439:22)
    at __widl_f_get_DOMStringMap (http://localhost:8082/index.js:83:94)
    at web_sys::DomStringMap::get::h22a3e60caadf998d (wasm-function[617]:167)
    at todomvc::element::Element::dataset_get::h7e42e0c4503b93ca (wasm-function[174]:400)
    at todomvc::view::item_id::{{closure}}::h53639abff4334373 (wasm-function[141]:136)
    at core::option::Option<T>::map::h4dd41af953263be4 (wasm-function[240]:255)
    at todomvc::view::item_id::h3ead9cd27809c346 (wasm-function[931]:85)
    at todomvc::view::View::bind_remove_item::{{closure}}::h6f34d6986b964f1c (wasm-function[124]:308)
    at todomvc::element::Element::delegate::{{closure}}::h89df052d5a99b170 (wasm-function[127]:1150)

Additional Context

• I have confirmed this issue exists since wasm-bindgen 0.2.33
• The error is being raised at el.dataset().get(key) when el.dataset() (which is a DomStringMap) doesn't have the corresponding key:
  

  wasm-bindgen/examples/todomvc/src/element.rs

  Lines 292 to 303 in 7fd6702

  	pub fn dataset_get(&mut self, key: &str) -> String {
  	let mut text = String::new();
  	if let Some(el) = self.el.take() {
  	{
  	if let Some(el) = wasm_bindgen::JsCast::dyn_ref::<web_sys::HtmlElement>(&el) {
  	text = el.dataset().get(key);
  	}
  	}
  	self.el = Some(el);
  	}
  	text
  	}

  
  Per API, I think .get() should simply return "" instead in that case. (like wasm-bindgen <= 0.2.32)
• For clarification, I experimented with DomStringMap related test cases

    assert_eq!(element.dataset().get("id"), "", "Shouldn't have data-id");
    element.dataset().set("id", "123").unwrap();
    assert_eq!(element.dataset().get("id"), "123", "Should have data-id");

by modifying https://github.com/rustwasm/wasm-bindgen/blob/master/crates/web-sys/tests/wasm/html_element.rs

In my local experiment with the latest wasm-bindgen version, the first line assert_eq!(element.dataset().get("id"), "", "Shouldn't have data-id"); is failing as

$ cd crates/web-sys
$ cargo +nightly  test --target wasm32-unknown-unknown --all-features

...
test wasm::html_element::test_html_element ... FAIL
...
failures:

---- wasm::html_element::test_html_element output ----
    error output:
        wasm-bindgen: imported JS function that was not marked as `catch` threw an error: expec
ted a string argument

        Stack:
        passStringToWasm@http://127.0.0.1:50399/wasm-bindgen-test:241:41
        init/imports.wbg.__widl_f_get_DOMStringMap@http://127.0.0.1:50399/wasm-bindgen-test:948
:26
        web_sys::DomStringMap::get::h7dbc1ad3c254416a@http://127.0.0.1:50399/wasm-bindgen-test_
bg.wasm:wasm-function[1786]:0xfb0d3
        wasm::html_element::test_html_element::h56a8b75625ff47a5@http://127.0.0.1:50399/wasm-bindgen-test_bg.wasm:wasm-function[533]:0x1d138
        core::ops::function::FnOnce::call_once::h2f109aedc6512641@http://127.0.0.1:50399/wasm-bindgen-test_bg.wasm:wasm-function[6316]:0x177fe3
...

• I'm novice at WebIDL details and not sure how to fix the above error.

[Pauan]

Well, this is interesting. The WebIDL says that the getter returns a DOMString, but both Chrome and Firefox return undefined:

document.body.dataset["foo"]

So I'm not sure if this is a spec bug (?) or a browser bug.

It seems document.body.style["foo"] behaves the same, so I guess we have to assume that any getter in WebIDL might return undefined.

[alexcrichton]

Thanks for the report @j-devel! Do you or @Pauan know if this is a bug in our WebIDL or is this a bug in our intepretation of the WebIDL spec?

[Pauan]

@alexcrichton I believe it's a bug in our interpretation of WebIDL.

According to the official spec, it says that DOMStringMap has a getter which returns DOMString.

So web-sys generates a DomStringMap::get(&self, name: &str) -> String binding.

This all seems fine, except the WebIDL is fibbing a bit: it might return a DOMString, or it might return undefined.

So we need to change web-sys so it generates DomStringMap::get(&self, name: &str) -> Option<String> instead.

I think we only need to do this for getters, not for non-getters.

We should also check to see whether other getters are affected, or if it's only DomStringMap.

[j-devel]

@alexcrichton I think it's the ambiguity of our interpretation due to the official spec being implicit about when to use undefined. It seems only when getters do return the empty string, the spec explicitly states so (e.g. element.title and element.dir cases). Otherwise, undefined is being used in actual browser implementations as @Pauan pointed out:

document.body.dataset["foo"] // undefined
document.body.style["foo"] // undefined

Following @Pauan 's suggestion, I did more tests starting with CssStyleDeclaration and here's the results and observations:

    //---- impl CssStyleDeclaration  getters
    // pub fn get_property_priority(&self, property: &str) -> String
    assert_eq!(element.style().get_property_priority("foo"), "", "debug"); // pass

    // pub fn get_property_value(&self, property: &str) -> Result<String, JsValue>
    assert_eq!(element.style().get_property_value("foo").unwrap(), "", "debug"); // pass

    // pub fn item(&self, index: u32) -> String
    assert_eq!(element.style().item(0), "", "debug"); // pass

    // pub fn get(&self, index: u32) -> String
    //assert_eq!(element.style().get(0), "", "debug"); // fail  !!!!

    // pub fn css_text(&self) -> String
    assert_eq!(element.style().css_text(), "", "debug"); // pass

    // pub fn length(&self) -> u32
    assert_eq!(element.style().length(), 0, "debug"); // pass 

    // pub fn parent_rule(&self) -> Option<CssRule>
    assert_eq!(element.style().parent_rule(), None, "debug"); // pass

    //---- impl DomStringMap
    // pub fn get(&self, name: &str) -> String
    //assert_eq!(element.dataset().get("id"), "", "Shouldn't have data-id"); // fail !!!!

CssStyleDeclaration::get() is failing with the same "error" as the DomStringMap case, which is a problem. However, I'm more wondering why the signature is pub fn get(&self, index: u32) -> String (isn't this for ::item() ?).

Interestingly, pub fn get_property_value(&self, property: &str) -> Result<String, JsValue> is doing the right job. So, to be consistent, DomStringMap::get(&self, name: &str) -> Result<String, JsValue> (instead of Option<String>) can be also considered?

Thanks for follow-ups. I'm new to Rust and excuse me if I were saying something out of focus.

[Pauan]

@j-devel However, I'm more wondering why the signature is pub fn get(&self, index: u32) -> String (isn't this for ::item() ?).

CSS styles are a bit weird. They accept either a number or a string:

document.body.style["backgroundColor"] = "red";
console.log(document.body.style["backgroundColor"]);
console.log(document.body.style[0]);

But according to the CSS spec, the getter is on item, and so we generate the same types for both item and get.

Interestingly, get_property_value is doing the right job.

Yes, because it's not a getter, it's a method, so it never returns undefined. As far as I know, it's only getters that return undefined.

It's also important to note that get_property_value does not return Err on unknown values, instead it returns Ok(""), because it didn't actually error.

So, to be consistent, DomStringMap::get(&self, name: &str) -> Result<String, JsValue> can be also considered?

I don't think that's a good idea. Result is for errors, but this isn't an error situation, it's just the API returning undefined. Using Option is correct for that case.

[j-devel]

@Pauan Your explanations make sense to me.

[alexcrichton]

Ok I've tagged this as a breaking change for now to ensure we get back to it, but it'd be good to investigate this in the meantime if someone gets a chance! (in that it'd be great to fix in a non-breaking way now so we can have something to move over when we do release breaking changes)

[j-devel]

Currently, web-sys imports 46 indexing getters that are our concern. I've dumped them all along with their signatures. We can see 24 getters (in pink) are already wrapped as Option<T> and 8 getters (in yellow) as Result<T, JsValue>. So, the rest 14 getters (including the troubling __widl_f_get_DOMStringMap and __widl_f_get_CSSStyleDeclaration that we saw) can potentially cause the issue.

Now I'm leaning toward fixing this issue conservatively by wrapping the 14 getters' return types as Option<T>, which is a breaking change. I refer to #1789 for implementation and accompanying tests.

After the change applied, we have the following bindings with updated signatures:

If this approach sounds ok, I'd like to go ahead to update the examples so they become consistent with the new signatures.

[j-devel]

Closing since this issue has been fixed by #1789. Many thanks!