From c0ddabcca11ef78234f6ec8c76cdb063af301a5c Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 14 Jan 2019 13:09:05 -0800 Subject: [PATCH 1/3] Sanity check `exposed_globals` map Make sure that we don't actually try to expose something when they've already been written, causing an internal tool panic rather than wrong JS. --- crates/cli-support/src/js/mod.rs | 66 ++++++++++++++++++-------------- crates/cli-support/src/lib.rs | 1 + 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index b068cbca..7f054d4d 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -21,6 +21,7 @@ mod closures; pub struct Context<'a> { pub globals: String, + pub globals_written: bool, pub imports: String, pub imports_post: String, pub footer: String, @@ -119,6 +120,11 @@ const INITIAL_HEAP_VALUES: &[&str] = &["undefined", "null", "true", "false"]; const INITIAL_HEAP_OFFSET: usize = 32; impl<'a> Context<'a> { + fn should_write_global(&mut self, name: &'static str) -> bool { + assert!(!self.globals_written); + self.exposed_globals.insert(name) + } + fn export(&mut self, name: &str, contents: &str, comments: Option) { let contents = contents.trim(); if let Some(ref c) = comments { @@ -478,6 +484,10 @@ impl<'a> Context<'a> { self.rewrite_imports(module_name); self.update_producers_section(); + // Cause any future calls to `should_write_global` to panic, making sure + // we don't ask for items which we can no longer emit. + self.globals_written = true; + let mut js = if self.config.threads.is_some() { // TODO: It's not clear right now how to best use threads with // bundlers like webpack. We need a way to get the existing @@ -874,7 +884,7 @@ impl<'a> Context<'a> { } fn expose_drop_ref(&mut self) { - if !self.exposed_globals.insert("drop_ref") { + if !self.should_write_global("drop_ref") { return; } self.expose_global_heap(); @@ -901,7 +911,7 @@ impl<'a> Context<'a> { } fn expose_global_heap(&mut self) { - if !self.exposed_globals.insert("heap") { + if !self.should_write_global("heap") { return; } self.global(&format!("const heap = new Array({});", INITIAL_HEAP_OFFSET)); @@ -910,7 +920,7 @@ impl<'a> Context<'a> { } fn expose_global_heap_next(&mut self) { - if !self.exposed_globals.insert("heap_next") { + if !self.should_write_global("heap_next") { return; } self.expose_global_heap(); @@ -918,7 +928,7 @@ impl<'a> Context<'a> { } fn expose_get_object(&mut self) { - if !self.exposed_globals.insert("get_object") { + if !self.should_write_global("get_object") { return; } self.expose_global_heap(); @@ -929,7 +939,7 @@ impl<'a> Context<'a> { } fn expose_assert_num(&mut self) { - if !self.exposed_globals.insert("assert_num") { + if !self.should_write_global("assert_num") { return; } self.global(&format!( @@ -942,7 +952,7 @@ impl<'a> Context<'a> { } fn expose_assert_bool(&mut self) { - if !self.exposed_globals.insert("assert_bool") { + if !self.should_write_global("assert_bool") { return; } self.global(&format!( @@ -957,14 +967,14 @@ impl<'a> Context<'a> { } fn expose_wasm_vector_len(&mut self) { - if !self.exposed_globals.insert("wasm_vector_len") { + if !self.should_write_global("wasm_vector_len") { return; } self.global("let WASM_VECTOR_LEN = 0;"); } fn expose_pass_string_to_wasm(&mut self) -> Result<(), Error> { - if !self.exposed_globals.insert("pass_string_to_wasm") { + if !self.should_write_global("pass_string_to_wasm") { return Ok(()); } self.require_internal_export("__wbindgen_malloc")?; @@ -1025,7 +1035,7 @@ impl<'a> Context<'a> { } fn expose_pass_array_jsvalue_to_wasm(&mut self) -> Result<(), Error> { - if !self.exposed_globals.insert("pass_array_jsvalue") { + if !self.should_write_global("pass_array_jsvalue") { return Ok(()); } self.require_internal_export("__wbindgen_malloc")?; @@ -1054,7 +1064,7 @@ impl<'a> Context<'a> { delegate: &str, size: usize, ) -> Result<(), Error> { - if !self.exposed_globals.insert(name) { + if !self.should_write_global(name) { return Ok(()); } self.require_internal_export("__wbindgen_malloc")?; @@ -1076,14 +1086,14 @@ impl<'a> Context<'a> { } fn expose_text_encoder(&mut self) { - if !self.exposed_globals.insert("text_encoder") { + if !self.should_write_global("text_encoder") { return; } self.expose_text_processor("TextEncoder"); } fn expose_text_decoder(&mut self) { - if !self.exposed_globals.insert("text_decoder") { + if !self.should_write_global("text_decoder") { return; } self.expose_text_processor("TextDecoder"); @@ -1112,7 +1122,7 @@ impl<'a> Context<'a> { } fn expose_get_string_from_wasm(&mut self) { - if !self.exposed_globals.insert("get_string_from_wasm") { + if !self.should_write_global("get_string_from_wasm") { return; } self.expose_text_decoder(); @@ -1148,7 +1158,7 @@ impl<'a> Context<'a> { } fn expose_get_array_js_value_from_wasm(&mut self) { - if !self.exposed_globals.insert("get_array_js_value_from_wasm") { + if !self.should_write_global("get_array_js_value_from_wasm") { return; } self.expose_uint32_memory(); @@ -1224,7 +1234,7 @@ impl<'a> Context<'a> { } fn arrayget(&mut self, name: &'static str, mem: &'static str, size: usize) { - if !self.exposed_globals.insert(name) { + if !self.should_write_global(name) { return; } self.global(&format!( @@ -1341,7 +1351,7 @@ impl<'a> Context<'a> { } fn memview(&mut self, name: &'static str, js: &str) { - if !self.exposed_globals.insert(name) { + if !self.should_write_global(name) { return; } let mem = self.memory(); @@ -1362,7 +1372,7 @@ impl<'a> Context<'a> { } fn expose_assert_class(&mut self) { - if !self.exposed_globals.insert("assert_class") { + if !self.should_write_global("assert_class") { return; } self.global( @@ -1378,14 +1388,14 @@ impl<'a> Context<'a> { } fn expose_global_stack_pointer(&mut self) { - if !self.exposed_globals.insert("stack_pointer") { + if !self.should_write_global("stack_pointer") { return; } self.global(&format!("let stack_pointer = {};", INITIAL_HEAP_OFFSET)); } fn expose_borrowed_objects(&mut self) { - if !self.exposed_globals.insert("borrowed_objects") { + if !self.should_write_global("borrowed_objects") { return; } self.expose_global_heap(); @@ -1407,7 +1417,7 @@ impl<'a> Context<'a> { } fn expose_take_object(&mut self) { - if !self.exposed_globals.insert("take_object") { + if !self.should_write_global("take_object") { return; } self.expose_get_object(); @@ -1424,7 +1434,7 @@ impl<'a> Context<'a> { } fn expose_add_heap_object(&mut self) { - if !self.exposed_globals.insert("add_heap_object") { + if !self.should_write_global("add_heap_object") { return; } self.expose_global_heap(); @@ -1566,7 +1576,7 @@ impl<'a> Context<'a> { } fn expose_global_argument_ptr(&mut self) -> Result<(), Error> { - if !self.exposed_globals.insert("global_argument_ptr") { + if !self.should_write_global("global_argument_ptr") { return Ok(()); } self.require_internal_export("__wbindgen_global_argument_ptr")?; @@ -1585,7 +1595,7 @@ impl<'a> Context<'a> { } fn expose_get_inherited_descriptor(&mut self) { - if !self.exposed_globals.insert("get_inherited_descriptor") { + if !self.should_write_global("get_inherited_descriptor") { return; } // It looks like while rare some browsers will move descriptors up the @@ -1613,7 +1623,7 @@ impl<'a> Context<'a> { fn expose_u32_cvt_shim(&mut self) -> &'static str { let name = "u32CvtShim"; - if !self.exposed_globals.insert(name) { + if !self.should_write_global(name) { return name; } self.global(&format!("const {} = new Uint32Array(2);", name)); @@ -1622,7 +1632,7 @@ impl<'a> Context<'a> { fn expose_int64_cvt_shim(&mut self) -> &'static str { let name = "int64CvtShim"; - if !self.exposed_globals.insert(name) { + if !self.should_write_global(name) { return name; } let n = self.expose_u32_cvt_shim(); @@ -1635,7 +1645,7 @@ impl<'a> Context<'a> { fn expose_uint64_cvt_shim(&mut self) -> &'static str { let name = "uint64CvtShim"; - if !self.exposed_globals.insert(name) { + if !self.should_write_global(name) { return name; } let n = self.expose_u32_cvt_shim(); @@ -1647,7 +1657,7 @@ impl<'a> Context<'a> { } fn expose_is_like_none(&mut self) { - if !self.exposed_globals.insert("is_like_none") { + if !self.should_write_global("is_like_none") { return; } self.global( @@ -1660,7 +1670,7 @@ impl<'a> Context<'a> { } fn expose_cleanup_groups(&mut self) { - if !self.exposed_globals.insert("cleanup_groups") { + if !self.should_write_global("cleanup_groups") { return; } self.global( diff --git a/crates/cli-support/src/lib.rs b/crates/cli-support/src/lib.rs index 41145965..29a7a483 100644 --- a/crates/cli-support/src/lib.rs +++ b/crates/cli-support/src/lib.rs @@ -196,6 +196,7 @@ impl Bindgen { let (js, ts) = { let mut cx = js::Context { globals: String::new(), + globals_written: false, imports: String::new(), imports_post: String::new(), footer: String::new(), From b21489368cdbece236277820ccbae2d0c7495dbf Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 14 Jan 2019 13:11:07 -0800 Subject: [PATCH 2/3] Ensure internal `exported_classes` map is consistent Throw it in an `Option` and then `take()` it when we consume it to ensure that future calls to insert data into it panic instead of producing inconsistent JS. --- crates/cli-support/src/js/mod.rs | 19 +++++++++++-------- crates/cli-support/src/lib.rs | 5 ++--- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index 7f054d4d..fa34d795 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -21,12 +21,11 @@ mod closures; pub struct Context<'a> { pub globals: String, - pub globals_written: bool, pub imports: String, pub imports_post: String, pub footer: String, pub typescript: String, - pub exposed_globals: HashSet<&'static str>, + pub exposed_globals: Option>, pub required_internal_exports: HashSet<&'static str>, pub imported_functions: HashSet<&'a str>, pub imported_statics: HashSet<&'a str>, @@ -56,7 +55,7 @@ pub struct Context<'a> { /// wasm-bindgen emits. pub direct_imports: HashMap<&'a str, (&'a str, &'a str)>, - pub exported_classes: HashMap, + pub exported_classes: Option>, pub function_table_needed: bool, pub interpreter: &'a mut Interpreter, pub memory_init: Option, @@ -121,8 +120,7 @@ const INITIAL_HEAP_OFFSET: usize = 32; impl<'a> Context<'a> { fn should_write_global(&mut self, name: &'static str) -> bool { - assert!(!self.globals_written); - self.exposed_globals.insert(name) + self.exposed_globals.as_mut().unwrap().insert(name) } fn export(&mut self, name: &str, contents: &str, comments: Option) { @@ -486,7 +484,7 @@ impl<'a> Context<'a> { // Cause any future calls to `should_write_global` to panic, making sure // we don't ask for items which we can no longer emit. - self.globals_written = true; + drop(self.exposed_globals.take().unwrap()); let mut js = if self.config.threads.is_some() { // TODO: It's not clear right now how to best use threads with @@ -661,8 +659,7 @@ impl<'a> Context<'a> { } fn write_classes(&mut self) -> Result<(), Error> { - let classes = mem::replace(&mut self.exported_classes, Default::default()); - for (class, exports) in classes { + for (class, exports) in self.exported_classes.take().unwrap() { self.write_class(&class, &exports)?; } Ok(()) @@ -1751,6 +1748,8 @@ impl<'a> Context<'a> { fn require_class_wrap(&mut self, class: &str) { self.exported_classes + .as_mut() + .expect("classes already written") .entry(class.to_string()) .or_insert_with(ExportedClass::default) .wrap_needed = true; @@ -2311,6 +2310,8 @@ impl<'a, 'b> SubContext<'a, 'b> { let class = self .cx .exported_classes + .as_mut() + .expect("classes already written") .entry(class_name.to_string()) .or_insert(ExportedClass::default()); class @@ -2540,6 +2541,8 @@ impl<'a, 'b> SubContext<'a, 'b> { let class = self .cx .exported_classes + .as_mut() + .expect("classes already written") .entry(struct_.name.to_string()) .or_insert_with(Default::default); class.comments = format_doc_comments(&struct_.comments, None); diff --git a/crates/cli-support/src/lib.rs b/crates/cli-support/src/lib.rs index 29a7a483..1a07c032 100644 --- a/crates/cli-support/src/lib.rs +++ b/crates/cli-support/src/lib.rs @@ -196,16 +196,15 @@ impl Bindgen { let (js, ts) = { let mut cx = js::Context { globals: String::new(), - globals_written: false, imports: String::new(), imports_post: String::new(), footer: String::new(), typescript: format!("/* tslint:disable */\n"), - exposed_globals: Default::default(), + exposed_globals: Some(Default::default()), required_internal_exports: Default::default(), imported_names: Default::default(), imported_identifiers: Default::default(), - exported_classes: Default::default(), + exported_classes: Some(Default::default()), config: &self, module: &mut module, function_table_needed: false, From 20e871f676ad6c8660fa94c21478eb1c5e0a2b97 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 14 Jan 2019 13:13:38 -0800 Subject: [PATCH 3/3] Fix an issue where closure rewriting required class internals Surfaced through previous sanity-checking commits, this reorders some internal operations to... Closes #1174 --- crates/cli-support/src/js/mod.rs | 3 +-- tests/wasm/closures.js | 5 +++++ tests/wasm/closures.rs | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index fa34d795..12d3fd6e 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -171,8 +171,6 @@ impl<'a> Context<'a> { } pub fn finalize(&mut self, module_name: &str) -> Result<(String, String), Error> { - self.write_classes()?; - self.bind("__wbindgen_object_clone_ref", &|me| { me.expose_get_object(); me.expose_add_heap_object(); @@ -429,6 +427,7 @@ impl<'a> Context<'a> { })?; closures::rewrite(self).with_context(|_| "failed to generate internal closure shims")?; + self.write_classes()?; self.unexport_unused_internal_exports(); // Handle the `start` function, if one was specified. If we're in a diff --git a/tests/wasm/closures.js b/tests/wasm/closures.js index e1048bed..641d56a6 100644 --- a/tests/wasm/closures.js +++ b/tests/wasm/closures.js @@ -1,4 +1,5 @@ const assert = require('assert'); +const wasm = require('wasm-bindgen-test'); exports.works_call = a => { a(); @@ -97,3 +98,7 @@ exports.drop_during_call_save = f => { DROP_DURING_CALL = f; }; exports.drop_during_call_call = () => DROP_DURING_CALL(); + +exports.js_test_closure_returner = () => { + wasm.closure_returner().someKey(); +}; diff --git a/tests/wasm/closures.rs b/tests/wasm/closures.rs index 443d270e..8e8fb5cf 100644 --- a/tests/wasm/closures.rs +++ b/tests/wasm/closures.rs @@ -63,6 +63,8 @@ extern "C" { fn drop_during_call_save(a: &Closure); fn drop_during_call_call(); + + fn js_test_closure_returner(); } #[wasm_bindgen_test] @@ -279,3 +281,33 @@ fn drop_during_call_ok() { assert!(HIT); } } + +#[wasm_bindgen_test] +fn test_closure_returner() { + type ClosureType = FnMut() -> BadStruct; + + use js_sys::{Object, Reflect}; + use wasm_bindgen::JsCast; + + js_test_closure_returner(); + + #[wasm_bindgen] + pub struct ClosureHandle(Closure); + + #[wasm_bindgen] + pub struct BadStruct {} + + #[wasm_bindgen] + pub fn closure_returner() -> Result { + + let o = Object::new(); + + let some_fn = Closure::wrap(Box::new(move || BadStruct {}) as Box); + Reflect::set(&o, &JsValue::from("someKey"), &some_fn.as_ref().unchecked_ref()) + .unwrap(); + Reflect::set(&o, &JsValue::from("handle"), &JsValue::from(ClosureHandle(some_fn))) + .unwrap(); + + Ok(o) + } +}