Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasm-bindgen: Added support for Internet Explorer 11 #139

Merged
merged 1 commit into from Apr 27, 2020

Conversation

zer0x64
Copy link
Contributor

@zer0x64 zer0x64 commented Apr 24, 2020

It is a pretty obscure use case, but I did stumble upon it:

When using wasm2js to compile WASM to Javascript for use in older browsers as recommended by the wasm-bindgen documentation, we found out that rand crate did not work on Internet Explorer 11 since the window.crypto object is named window.msCrypto.

This PR adds a check to use msCrypto if crypto is not present.

Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @zer0x64 for this patch. If Rust docs reccomend wasm2js, and supporting IE isn't that hard, we should do it. Note, we would probably not support IE versions before 11.

Quick question, is there an easy way to test this in CI? I don't think so, but I thought I would ask.

Comment on lines 97 to 101

// Internet Explorer 11
#[wasm_bindgen(method, getter, js_name="msCrypto", structural)]
fn ms_crypto(me: &Self_) -> JsValue;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting nit here:

    type Self_;
    #[wasm_bindgen(method, getter, structural)]
    fn crypto(me: &Self_) -> JsValue;
    #[wasm_bindgen(method, getter, js_name = "msCrypto", structural)]
    fn ms_crypto(me: &Self_) -> JsValue;

We should explain the use of msCrypto above (when we use it)

@@ -64,9 +64,15 @@ fn getrandom_init() -> Result<RngSource, Error> {
// `crypto.getRandomValues`, but if `crypto` isn't defined we assume
// we're in an older web browser and the OS RNG isn't available.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document the crypto vs mscrypto distinction here, so it's all in one place. Something like

        // If `self` is defined then we're in a browser somehow (main window
        // or web worker). We get `self.crypto` (called `msCrypto` on IE), so we
        // can call `crypto.getRandomValues`. If `crypto` isn't defined, we
        // assume we're in an older web browser and the OS RNG isn't available.

Comment on lines 69 to 75
// If crypto is undefined, we can also check if msCrypto is defined,
// in case it's Internet Explorer
crypto = self_.ms_crypto();

if crypto.is_undefined() {
return Err(BINDGEN_CRYPTO_UNDEF);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this logic more straightforward using Rust's match guards:

        let crypto: BrowserCrypto = match (self_.crypto(), self_.ms_crypto()) {
            (crypto, _) if !crypto.is_undefined() => crypto.into(),
            (_, crypto) if !crypto.is_undefined() => crypto.into(),
            _ => return Err(BINDGEN_CRYPTO_UNDEF),
        };

        // Test if `crypto.getRandomValues` is undefined as well
        if crypto.get_random_values_fn().is_undefined() {
            return Err(BINDGEN_GRV_UNDEF);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that!

@josephlr josephlr mentioned this pull request Apr 25, 2020
@josephlr
Copy link
Member

@zer0x64, also be sure to run cargo fmt --all -- **/*.rs before submitting. By default, cargo fmt won't format the target-specific files.

@zer0x64
Copy link
Contributor Author

zer0x64 commented Apr 25, 2020

Great, I'll address the changes tomorrow!

For the CI, unfortunately, wasm-bindgen-test doesn't support headless IE, so the only way I see to test this in a CI is to use a framework like Selenium. It is certainly doable, but not as straightforward as testing Firefox/Chrome. Also, I don't think this can be done without a Windows build server. However, I can help integrate it if you decide to add this check to the CI!

EDIT: Also forgot to mention that since IE does not support webassembly, we would need to use wasm2js to translate it to javascript and, from experience, that part is mostly manual work and does not integrates well in a CI for now. According to the wasm-bindgen doc, some day it should be supported out of the box without manual work.

@josephlr
Copy link
Member

@zer0x64 would wasm-bindgen's vendor_prefix do all of this for us?

Could we just do:

    type Self_;
    #[wasm_bindgen(method, getter, vendor_prefix = "ms", structural)]
    fn crypto(me: &Self_) -> JsValue;

and not need any additional code? I don't know if msCrypto and mscrypto are the same thing in JS.

EDIT: Also forgot to mention that since IE does not support webassembly, we would need to use wasm2js to translate it to javascript and, from experience, that part is mostly manual work and does not integrates well in a CI for now. According to the wasm-bindgen doc, some day it should be supported out of the box without manual work.

This seems reasonable. We can always add more testing later once the tools are in a better state. Our coverage right now is fairly good (Node, Chrome, Firefox).

@zer0x64
Copy link
Contributor Author

zer0x64 commented Apr 25, 2020

I did some tests with the vendor_prefix and it simply doesn't seem to work in this context at all. I think it might be since the annotation is made for type and not for fn. Although even if it worked, we'd still have the problem that it exports msCrypto and not mscrypto.

@zer0x64
Copy link
Contributor Author

zer0x64 commented Apr 25, 2020

I've just pushed the fixes for your comments!

Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just need #141 to be approved, then we can merge this.

@josephlr
Copy link
Member

josephlr commented Apr 27, 2020

@zer0x64 can you rebase this onto the 0.2 branch (right now this PR is against the master branch), this will allow you to pickup #141

The only change you'll need will be to turn BINDGEN_CRYPTO_UNDEF into Error::BINDGEN_CRYPTO_UNDEF

I would do this myself, but I don't have access to modify your PR.

EDIT: Actually, having this PR against the master branch is fine.

@josephlr
Copy link
Member

Actually, @zer0x64, having this against the master branch is fine. I'll handle putting the changes on the 0.2 branch. Just rebase this on #142 when that lands.

@josephlr
Copy link
Member

@zer0x64 #142 has been merged, can you rebase this onto master?

(crypto, _) if !crypto.is_undefined() => crypto.into(),
(_, crypto) if !crypto.is_undefined() => crypto.into(),
_ => return Err(BINDGEN_CRYPTO_UNDEF),
};

// Test if `crypto.getRandomValues` is undefined as well
let crypto: BrowserCrypto = crypto.into();
Copy link
Member

@josephlr josephlr Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need this line (as we already convert the JsValue to a BrowserCrypto above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem!

@zer0x64
Copy link
Contributor Author

zer0x64 commented Apr 27, 2020

Rebased and tested on my side, so when the CI pass it will be ready to merge!

@josephlr josephlr merged commit 0ad1c77 into rust-random:master Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants