Remove the implementation of DerefMut for FlowRef. #6503

Ms2ger · 2015-06-27T19:27:56Z

FlowRef is basically Arc<Flow>, so DerefMut trivially exposes aliased &mut Flows.

SimonSapin · 2015-07-15T05:55:10Z

I tried just removing the DerefMut impl. After getting past a few initial errors, I got a bunch more :(

   Compiling layout v0.0.1 (file:///home/simon/projects/servo/components/servo)
/home/simon/projects/servo/components/layout/construct.rs:444:31: 444:46 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:444             let inline_flow = inline_flow_ref.as_inline();
                                                                                            ^~~~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/construct.rs:772:40: 772:44 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:772                         let kid_node = flow.as_block().fragment.node;
                                                                                                     ^~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:750:9: 823:10 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:773:41: 773:45 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:773                         let kid_style = flow.as_block().fragment.style.clone();
                                                                                                      ^~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:750:9: 823:10 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:774:50: 774:54 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:774                         let kid_restyle_damage = flow.as_block().fragment.restyle_damage;
                                                                                                               ^~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:750:9: 823:10 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:962:25: 962:33 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:962                         kid_flow.as_block()
                                                                                      ^~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:958:9: 972:10 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:1046:35: 1046:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1046         let is_fixed_positioned = wrapper_flow.as_block().is_fixed();
                                                                                                 ^~~~~~~~~~~~
/home/simon/projects/servo/components/layout/construct.rs:1262:37: 1262:43 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1262                 flow::mut_base(&mut **flow).restyle_damage.insert(damage);
                                                                                                   ^~~~~~
/home/simon/projects/servo/components/layout/construct.rs:1263:17: 1263:21 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1263                 flow.repair_style_and_bubble_inline_sizes(&style);
                                                                               ^~~~
/home/simon/projects/servo/components/layout/construct.rs:1277:49: 1277:80 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1277                             flow::mut_base(&mut *inline_block_fragment.flow_ref).restyle_damage
                                                                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:1272:17: 1306:18 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:1280:29: 1280:59 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1280                             inline_block_fragment.flow_ref
                                                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:1272:17: 1306:18 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:1285:49: 1285:96 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1285                             flow::mut_base(&mut *inline_absolute_hypothetical_fragment.flow_ref)
                                                                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:1272:17: 1306:18 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:1288:29: 1289:42 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1288                             inline_absolute_hypothetical_fragment
/home/simon/projects/servo/components/layout/construct.rs:1289                                 .flow_ref
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:1272:17: 1306:18 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:1293:49: 1293:83 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1293                             flow::mut_base(&mut *inline_absolute_fragment.flow_ref).restyle_damage
                                                                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:1272:17: 1306:18 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:1296:29: 1296:62 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1296                             inline_absolute_fragment.flow_ref
                                                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/construct.rs:1272:17: 1306:18 note: expansion site
/home/simon/projects/servo/components/layout/construct.rs:1587:48: 1587:58 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1587             let kid_base = flow::mut_base(&mut *new_child);
                                                                                                              ^~~~~~~~~~
/home/simon/projects/servo/components/layout/construct.rs:1591:40: 1591:46 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1591         let base = flow::mut_base(&mut **self);
                                                                                                      ^~~~~~
/home/simon/projects/servo/components/layout/construct.rs:1606:13: 1606:17 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/construct.rs:1606             self.bubble_inline_sizes()
                                                                           ^~~~
/home/simon/projects/servo/components/layout/display_list_builder.rs:1673:43: 1673:63 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/display_list_builder.rs:1673                     let block_flow = &mut *block_flow.flow_ref;
                                                                                                                    ^~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/display_list_builder.rs:1654:9: 1689:10 note: expansion site
/home/simon/projects/servo/components/layout/display_list_builder.rs:1678:43: 1678:63 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/display_list_builder.rs:1678                     let block_flow = &mut *block_flow.flow_ref;
                                                                                                                    ^~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/display_list_builder.rs:1654:9: 1689:10 note: expansion site
/home/simon/projects/servo/components/layout/display_list_builder.rs:1683:43: 1683:63 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/display_list_builder.rs:1683                     let block_flow = &mut *block_flow.flow_ref;
                                                                                                                    ^~~~~~~~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/display_list_builder.rs:1654:9: 1689:10 note: expansion site
/home/simon/projects/servo/components/layout/flow.rs:747:42: 747:48 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/flow.rs:747         self.iter.next().map(|flow| &mut **flow)
                                                                                                  ^~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/flow.rs:747:30: 747:48 note: expansion site
/home/simon/projects/servo/components/layout/flow.rs:1351:34: 1351:40 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/flow.rs:1351         let base = mut_base(&mut **self);
                                                                                           ^~~~~~
/home/simon/projects/servo/components/layout/flow_list.rs:36:48: 36:54 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/flow_list.rs:36         self.flows.front_mut().map(|head| &mut **head)
                                                                                                            ^~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/flow_list.rs:36:36: 36:54 note: expansion site
/home/simon/projects/servo/components/layout/flow_list.rs:49:47: 49:53 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/flow_list.rs:49         self.flows.back_mut().map(|tail| &mut **tail)
                                                                                                           ^~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/flow_list.rs:49:35: 49:53 note: expansion site
/home/simon/projects/servo/components/layout/flow_list.rs:127:37: 127:40 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/flow_list.rs:127         self.it.next().map(|x| &mut **x)
                                                                                                  ^~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/flow_list.rs:127:28: 127:40 note: expansion site
/home/simon/projects/servo/components/layout/fragment.rs:1273:34: 1273:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1273                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1277:34: 1277:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1277                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1651:34: 1651:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1651                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1659:34: 1659:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1659                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1667:34: 1667:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1667                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1771:34: 1771:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1771                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1777:34: 1777:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1777                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1782:34: 1782:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1782                 let block_flow = info.flow_ref.as_block();
                                                                                               ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1902:30: 1902:56 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1902             let block_flow = inline_block_info.flow_ref.as_block();
                                                                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of if let expansion
/home/simon/projects/servo/components/layout/fragment.rs:1901:9: 1907:10 note: expansion site
/home/simon/projects/servo/components/layout/fragment.rs:1914:17: 1914:30 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1914                 info.flow_ref.update_late_computed_inline_position_if_necessary(position)
                                                                              ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/fragment.rs:1924:17: 1924:30 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/fragment.rs:1924                 info.flow_ref.update_late_computed_block_position_if_necessary(position)
                                                                              ^~~~~~~~~~~~~
/home/simon/projects/servo/components/layout/layout_task.rs:764:9: 764:13 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:764         flow.mark_as_root();
                                                                        ^~~~
/home/simon/projects/servo/components/layout/layout_task.rs:858:33: 858:46 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:858             flow::mut_base(&mut **layout_root).stacking_relative_position =
                                                                                                ^~~~~~~~~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/layout_task.rs:854:17: 918:10 note: expansion site
/home/simon/projects/servo/components/layout/layout_task.rs:862:33: 862:46 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:862             flow::mut_base(&mut **layout_root).clip =
                                                                                                ^~~~~~~~~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/layout_task.rs:854:17: 918:10 note: expansion site
/home/simon/projects/servo/components/layout/layout_task.rs:882:81: 882:94 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:882                 let root_background_color = get_root_flow_background_color(&mut **layout_root);
                                                                                                                                                ^~~~~~~~~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/layout_task.rs:854:17: 918:10 note: expansion site
/home/simon/projects/servo/components/layout/layout_task.rs:888:37: 888:50 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:888                 flow::mut_base(&mut **layout_root).display_list_building_result
                                                                                                    ^~~~~~~~~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/layout_task.rs:854:17: 918:10 note: expansion site
/home/simon/projects/servo/components/layout/layout_task.rs:981:51: 981:56 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:981                 LayoutTask::reflow_all_nodes(&mut *flow);
                                                                                                                  ^~~~~
note: in expansion of if let expansion
/home/simon/projects/servo/components/layout/layout_task.rs:980:13: 982:14 note: expansion site
/home/simon/projects/servo/components/layout/layout_task.rs:1149:53: 1149:62 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:1149             if opts::get().nonincremental_layout || root_flow.compute_layout_damage()
                                                                                                                     ^~~~~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/layout_task.rs:1148:17: 1153:10 note: expansion site
/home/simon/projects/servo/components/layout/layout_task.rs:1151:17: 1151:26 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/layout_task.rs:1151                 root_flow.reflow_entire_document()
                                                                                 ^~~~~~~~~
note: in expansion of closure expansion
/home/simon/projects/servo/components/layout/layout_task.rs:1148:17: 1153:10 note: expansion site
/home/simon/projects/servo/components/layout/inline.rs:1494:41: 1494:55 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/inline.rs:1494                     flow::mut_base(&mut *info.flow_ref).clip = clip;
                                                                                                    ^~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/inline.rs:1480:9: 1533:10 note: expansion site
/home/simon/projects/servo/components/layout/inline.rs:1496:38: 1496:51 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/inline.rs:1496                     let block_flow = info.flow_ref.as_block();
                                                                                                 ^~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/inline.rs:1480:9: 1533:10 note: expansion site
/home/simon/projects/servo/components/layout/inline.rs:1504:41: 1504:55 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/inline.rs:1504                     flow::mut_base(&mut *info.flow_ref).clip = clip;
                                                                                                    ^~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/inline.rs:1480:9: 1533:10 note: expansion site
/home/simon/projects/servo/components/layout/inline.rs:1505:38: 1505:51 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/inline.rs:1505                     let block_flow = info.flow_ref.as_block();
                                                                                                 ^~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/inline.rs:1480:9: 1533:10 note: expansion site
/home/simon/projects/servo/components/layout/inline.rs:1514:41: 1514:55 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/inline.rs:1514                     flow::mut_base(&mut *info.flow_ref).clip = clip;
                                                                                                    ^~~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/inline.rs:1480:9: 1533:10 note: expansion site
/home/simon/projects/servo/components/layout/inline.rs:1516:38: 1516:51 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/inline.rs:1516                     let block_flow = info.flow_ref.as_block();
                                                                                                 ^~~~~~~~~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/inline.rs:1480:9: 1533:10 note: expansion site
/home/simon/projects/servo/components/layout/parallel.rs:249:45: 249:51 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:249                 if self.should_process(&mut **flow) {
                                                                                                         ^~~~~~
/home/simon/projects/servo/components/layout/parallel.rs:250:39: 250:45 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:250                     self.process(&mut **flow);
                                                                                                   ^~~~~~
/home/simon/projects/servo/components/layout/parallel.rs:254:48: 254:54 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:254                 let base = flow::mut_base(&mut **flow);
                                                                                                            ^~~~~~
/home/simon/projects/servo/components/layout/parallel.rs:271:55: 271:63 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:271                 let parent_base = flow::mut_base(&mut **parent);
                                                                                                                   ^~~~~~~~
/home/simon/projects/servo/components/layout/parallel.rs:306:41: 306:47 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:306                     flow::mut_base(&mut **flow).thread_id = proxy.worker_index();
                                                                                                     ^~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/parallel.rs:299:9: 325:10 note: expansion site
/home/simon/projects/servo/components/layout/parallel.rs:309:45: 309:51 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:309                 if self.should_process(&mut **flow) {
                                                                                                         ^~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/parallel.rs:299:9: 325:10 note: expansion site
/home/simon/projects/servo/components/layout/parallel.rs:311:39: 311:45 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:311                     self.process(&mut **flow);
                                                                                                   ^~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/parallel.rs:299:9: 325:10 note: expansion site
/home/simon/projects/servo/components/layout/parallel.rs:315:50: 315:56 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:315                 for kid in flow::child_iter(&mut **flow) {
                                                                                                              ^~~~~~
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/parallel.rs:315:17: 318:18 note: expansion site
note: in expansion of for loop expansion
/home/simon/projects/servo/components/layout/parallel.rs:299:9: 325:10 note: expansion site
/home/simon/projects/servo/components/layout/parallel.rs:478:9: 478:13 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/parallel.rs:478         root.traverse_postorder(&bubble_inline_sizes);
                                                                     ^~~~
/home/simon/projects/servo/components/layout/sequential.rs:58:15: 58:21 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/sequential.rs:58     doit(&mut **root, 0, &mut traversal)
                                                                            ^~~~~~
/home/simon/projects/servo/components/layout/sequential.rs:81:21: 81:27 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/sequential.rs:81     let root = &mut **root;
                                                                                  ^~~~~~
/home/simon/projects/servo/components/layout/sequential.rs:119:15: 119:21 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/sequential.rs:119     doit(&mut **root, compute_absolute_positions, build_display_list);
                                                                             ^~~~~~
/home/simon/projects/servo/components/layout/sequential.rs:142:15: 142:21 error: cannot borrow immutable borrowed content as mutable
/home/simon/projects/servo/components/layout/sequential.rs:142     doit(&mut **root, iterator, &ZERO_POINT);
                                                                             ^~~~~~
error: aborting due to 63 previous errors
Could not compile `layout`.

metajack · 2015-07-15T06:00:05Z

This can't be easily removed. I tried this as well and gave up. The core issue is that during flow construction we need to mutate these unsafely, but once hte flows are constructed, we use them without DerefMut. We want to avoid constructing the tree in one data type and then converting it to another that is then shared. I'm not sure the best way to get around this. @pcwalton may have some thoughts; he wrote the original code.

Ms2ger · 2015-07-15T09:10:30Z

get_mut with a runtime refcount check?

SimonSapin · 2015-07-15T10:10:33Z

So Arc<RwLock>?

metajack · 2015-07-15T11:15:02Z

@Ms2ger That may work. I think get_mut was not available on Arc when I tried this, although I had a discussion with @aturon about this in Whistlier, and perhaps they've added it now.

@SimonSapin I don't think we're willing to pay the price of locking. All the fancy unsafety in layout is designed around Flows, and slowing down Flow access is probably a non-starter.

In any case, I agree this needs to be done; I was only trying to point out that it's more difficult than it seems at first.

SimonSapin · 2015-07-15T11:22:05Z

Ah, yes, refcount for mutable access made me think of RefCell, which in multi-thread translates to RwLock, but you’re right that we can have something like Arc::get_mut instead.

Is there a reason not to use Arc directly? Arc::get_mut is still unstable but now un-deprecated. (The data race has been solved in rust-lang/rust#26610.)

metajack · 2015-07-15T11:30:36Z

Using Arc directly is the desired outcome. How to reconcile this with flow construction is an exercise no one has completed yet. I'm pretty sure when I tried was right after they removed get_mut but before they fixed the race and put it back. It's worth trying again.

metajack · 2015-07-15T11:31:16Z

@SimonSapin It should be noted that @aturon was interested in helping us solve this, so if get_mut is insufficient, we should definitely talk to the Rust team about what options we have.

aturon · 2015-07-16T21:53:41Z

Yep, happy to help further on this, just let me know what the blockers are.

Ms2ger · 2015-07-23T17:11:14Z

get_mut isn't going to cut it; we do call into this even with refcounts greater than one. This is plain and simple UB.

metajack · 2015-07-27T15:41:18Z

@Ms2ger What is UB?

jdm · 2015-07-27T16:04:21Z

Undefined behaviour.


        Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

See #6503.


        Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

See #6503.


        Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

See #6503.

SimonSapin · 2015-08-18T17:58:01Z

This is only a problem if aliasing actually happens. Arc::get_mut knows it can’t happen when the reference count is 1.

@pcwalton writes #7237

The tree structure should ensure that it's safe. However, that crucially depends on the guts of layout never being able to see the FlowRefs of siblings and parents (and absolute containing block links). The current code is (or at least was) carefully crafted to try to avoid this (hence all the comments around ContainingBlockLink).


        Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

See #6503.


        Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

See #6503.

eefriedman · 2015-09-26T07:20:05Z

Some additional observations:

Calling flow_ref::deref_mut doesn't lead to undefined behavior unless the Arc in question is dereferenced again while the return value of deref_mut is still in scope. Arc::get_mut dynamically proves the safety using the reference count, but you can use a different invariant if you're willing to write unsafe code.
The current implementation of reflow depends on a tree property to make its usage essentially safe; using Box instead of Arc, you can implement something similar to the current flow tree without using any unsafe code. (As far as I can tell, there are currently three places which violate the tree-ness of flows in layout: ContainingBlockLink, layout_debug, and PrivateLayoutData. ContainingBlockLink is essentially a shortcut to avoid the cost of propagating certain reflow information down the tree, layout_debug is just for debugging, and PrivateLayoutData is never used during reflow because the DOM can't be accessed during layout.)
I think the usage of ContainingBlockLink alongside flow_ref::deref_mut leads to undefined behavior, although probably not in a way the compiler will actually ever break. (Strictly speaking, when an mutable reference is in scope, all accesses to the referenced data must go through that reference.)
Given that Flows form a tree, there's very little benefit to making them Arcs as opposed to Boxes.


        Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

See servo#6503.

nox · 2016-06-06T12:42:52Z

This starts warning:

warning: lifetime parameter `'a` declared on fn `flow_ref::deref_mut` appears only in the return type, but here is required to be higher-ranked, which means that `'a` must appear in both argument and return types, #[warn(hr_lifetime_in_assoc_type)] on by default
   --> /Users/nox/src/servo/components/layout/flow_list.rs:124:28
    |>
124 |>         self.it.next().map(flow_ref::deref_mut)
    |>                            ^^^^^^^^^^^^^^^^^^^


        Auto merge of #14053 - notriddle:flow_ref_unsafe, r=<try>

Make `FlowRef` a newtype This creates a sharp distinction between `Arc<Flow>`s, which may be owned by anyone, and `FlowRef`s, which may only be owned by the traversal code. By checking the reference count, we ensure that a `Flow` cannot be pointed to by `Arc`s and `FlowRef`s simultaneously. This is not a complete fix for #6503, though it is a necessary start (enforcing the no-aliasing rule of `FlowRef::deref_mut` will require far more work). --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14014 (github issue number if applicable). - [X] These changes do not require tests because the existing tests, plus the added assertions, should be sufficient  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14053)


        Auto merge of #14053 - notriddle:flow_ref_unsafe, r=<try>

Make `FlowRef` a newtype This creates a sharp distinction between `Arc<Flow>`s, which may be owned by anyone, and `FlowRef`s, which may only be owned by the traversal code. By checking the reference count, we ensure that a `Flow` cannot be pointed to by `Arc`s and `FlowRef`s simultaneously. This is not a complete fix for #6503, though it is a necessary start (enforcing the no-aliasing rule of `FlowRef::deref_mut` will require far more work). --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14014 (github issue number if applicable). - [X] These changes do not require tests because the existing tests, plus the added assertions, should be sufficient  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14053)


        Auto merge of #14053 - notriddle:flow_ref_unsafe, r=<try>

Make `FlowRef` a newtype This creates a sharp distinction between `Arc<Flow>`s, which may be owned by anyone, and `FlowRef`s, which may only be owned by the traversal code. By checking the reference count, we ensure that a `Flow` cannot be pointed to by `Arc`s and `FlowRef`s simultaneously. This is not a complete fix for #6503, though it is a necessary start (enforcing the no-aliasing rule of `FlowRef::deref_mut` will require far more work). --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14014 (github issue number if applicable). - [X] These changes do not require tests because the existing tests, plus the added assertions, should be sufficient  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14053)


        Auto merge of #14053 - notriddle:flow_ref_unsafe, r=pcwalton

Make `FlowRef` a newtype This creates a sharp distinction between `Arc<Flow>`s, which may be owned by anyone, and `FlowRef`s, which may only be owned by the traversal code. By checking the reference count, we ensure that a `Flow` cannot be pointed to by `Arc`s and `FlowRef`s simultaneously. This is not a complete fix for #6503, though it is a necessary start (enforcing the no-aliasing rule of `FlowRef::deref_mut` will require far more work). --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14014 (github issue number if applicable). - [X] These changes do not require tests because the existing tests, plus the added assertions, should be sufficient  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14053)

mbrubeck · 2017-12-14T19:06:12Z

Shouldn't FlowRef::deref_mut at least be an unsafe fn?

Ms2ger added A-layout/uncategorized I-safety labels Jun 27, 2015

SimonSapin added a commit that referenced this issue Aug 14, 2015

Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

4991322

See #6503.

SimonSapin added a commit that referenced this issue Aug 15, 2015

Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

b75c36d

See #6503.

SimonSapin mentioned this issue Aug 18, 2015

Replace FlowRef with Arc<Flow>, now that Arc supports DST. #7237

Merged

SimonSapin added a commit that referenced this issue Aug 18, 2015

Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

ac95d4c

See #6503.

SimonSapin added a commit that referenced this issue Aug 20, 2015

Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

1842344

See #6503.

SimonSapin added a commit that referenced this issue Aug 20, 2015

Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

2d22aa8

See #6503.

josiahdaniels added a commit to josiahdaniels/servo that referenced this issue Sep 28, 2015

Replace the unsound `impl DerefMut for FlowRef` with an unsafe function.

33dfedb

See servo#6503.

notriddle mentioned this issue Nov 3, 2016

Make `FlowRef` a newtype #14053

Merged

4 of 4 tasks complete

servo / servo

Remove the implementation of DerefMut for FlowRef. #6503

Remove the implementation of DerefMut for FlowRef. #6503

Ms2ger commented Jun 27, 2015

SimonSapin commented Jul 15, 2015

metajack commented Jul 15, 2015

Ms2ger commented Jul 15, 2015

SimonSapin commented Jul 15, 2015

metajack commented Jul 15, 2015

SimonSapin commented Jul 15, 2015

metajack commented Jul 15, 2015

metajack commented Jul 15, 2015

aturon commented Jul 16, 2015

Ms2ger commented Jul 23, 2015

metajack commented Jul 27, 2015

jdm commented Jul 27, 2015

SimonSapin commented Aug 18, 2015

eefriedman commented Sep 26, 2015

nox commented Jun 6, 2016

mbrubeck commented Dec 14, 2017

servo / servo

Join GitHub today

Remove the implementation of DerefMut for FlowRef. #6503

Remove the implementation of DerefMut for FlowRef. #6503

Comments

Ms2ger commented Jun 27, 2015

SimonSapin commented Jul 15, 2015

metajack commented Jul 15, 2015

Ms2ger commented Jul 15, 2015

SimonSapin commented Jul 15, 2015

metajack commented Jul 15, 2015

SimonSapin commented Jul 15, 2015

metajack commented Jul 15, 2015

metajack commented Jul 15, 2015

aturon commented Jul 16, 2015

Ms2ger commented Jul 23, 2015

metajack commented Jul 27, 2015

jdm commented Jul 27, 2015

SimonSapin commented Aug 18, 2015

eefriedman commented Sep 26, 2015

nox commented Jun 6, 2016

mbrubeck commented Dec 14, 2017