Rust miscompiles Servo after upgrading to new revision of Rust

[zwarich]

In the current Servo Rust upgrade branch (rustup-20140804-debug-segfault 4f40685a), Servo currently dies with a double borrow error loading essentially any page:

$ ./servo http://www.google.com
task 'LayoutWorker' failed at 'RefCell<T> already borrowed', /Users/jack/src/rust/src/libcore/cell.rs:306

Moving a single line of code out of an unsafe block makes this problem go away, which points to a miscompilation in at least one version of this code:

diff --git a/src/components/layout/layout_task.rs b/src/components/layout/layout_task.rs
index 632e3af..dd1bb09 100644
--- a/src/components/layout/layout_task.rs
+++ b/src/components/layout/layout_task.rs
@@ -573,9 +573,9 @@ impl LayoutTask {

     /// The high-level routine that performs layout tasks.
     fn handle_reflow(&mut self, data: &Reflow) {
+        let mut node: JS<Node> = unsafe { JS::from_trusted_node_address(data.document_root) };
         // FIXME: Isolate this transmutation into a "bridge" module.
         let node: &mut LayoutNode = unsafe {
-            let mut node: JS<Node> = JS::from_trusted_node_address(data.document_root);
             mem::transmute(&mut node)
         };

[larsbergstrom]

There is a Servo branch that preserves this behavior at:
https://github.com/servo/servo/tree/rustup-20140804-debug-segfault

[larsbergstrom]

@brson Can you help us find an owner for this? We landed a Rust upgrade by changing our code to avoid this compilation issue, so it's not blocking immediately, but still a scary bug.

[brson]

Nominating.

[pnkfelix]

Assigning P-high, not 1.0.

(Niko has volunteered to look into it further, which hopefully will yield more data for us to work with.)

[metajack]

It's possible we just hit this again tracking down random graphics stack failures. See the Servo issue linked above.

[nikomatsakis]

So I can certainly reproduce the failure. Investigating more.

[nikomatsakis]

Reading this patch more clearly, I think the real problem is that the old code was just wrong and abusing transmute. I'm sure the crash is due to the new code that reuses stack space more aggressively. Take a look at what's going on here (original code, variables renamed for clarity):

     fn handle_reflow(&mut self, data: &Reflow) {
         // FIXME: Isolate this transmutation into a "bridge" module.
         let node2: &mut LayoutNode = unsafe {
             let mut node1: JS<Node> = JS::from_trusted_node_address(data.document_root);
             mem::transmute(&mut node1)
         };

Here a block is created and then a variable node1 inside that block. The address of node1 is taken and it is transmuted and stored in node2. But at that point the block terminates and hence the stack space for node1 is reclaimed. This should certainly not type check -- it actually might, but I suspect PR #16453 will fix it if it does, if not that's good to know -- but in any case here there is a transmute so the type checker is helpless.

[huonw]

(FWIW, that suggests that the bug (in Servo) may've been triggered by #15863, since that presumably allowed LLVM to start reusing the stack space of node1.)

[metajack]

I'm not sure if this is related, but @zwarich and @mrobinson have been tracking down more bugs caused by the Rust upgrade and found this:

servo/servo#3084

[dotdash]

Yep, similar bug there creating a dangling *mut pointer that points to a temporary in a match expression. Submitted a fix for that to rust-azure.