Root nodes for the duration of their CSS transitions #16295

jdm · 2017-04-07T05:16:55Z

This ensures that we can pass a node address as part of the asynchronous
transition end notification, making it safe to fire the corresponding
DOM event on the node from the script thread. Without explicitly rooting
this node when the transition starts, we risk the node being GCed before
the transition is complete.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14972
There are tests for these changes

This change is

highfive · 2017-04-07T05:16:58Z

Heads up! This PR modifies the following files:

@bholley: components/style/matching.rs, components/style/animation.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script_layout_interface/rpc.rs
@fitzgen: components/script/script_thread.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script_layout_interface/rpc.rs
@emilio: components/layout/animation.rs, components/layout/query.rs, components/style/matching.rs, components/style/animation.rs

highfive · 2017-04-07T05:16:59Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm · 2017-04-07T05:17:06Z

@bors-servo: try

bors-servo · 2017-04-07T05:17:10Z

⌛ Trying commit 66d93d2 with merge 8b5c8b8...


        Auto merge of #16295 - jdm:transition-safety, r=<try>

Root nodes for the duration of their CSS transitions This ensures that we can pass a node address as part of the asynchronous transition end notification, making it safe to fire the corresponding DOM event on the node from the script thread. Without explicitly rooting this node when the transition starts, we risk the node being GCed before the transition is complete. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14972 - [X] There are tests for these changes

bors-servo · 2017-04-07T05:24:56Z

💔 Test failed - android

jdm · 2017-04-07T06:00:06Z

As I feared, there could be problems with this approach when the animations are triggered from the compositor:

  ▶ CRASH [expected TIMEOUT] /dom/events/EventListener-invoke-legacy.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ assertion failed: rw_data.newly_transitioning_nodes.is_empty() (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at /Users/servo/buildbot/slave/mac-rel-wpt1/build/components/layout_thread/lib.rs:1411)
  │ 2017-04-07 01:35:16.662 servo[70066:646481199] Metadata.framework [Error]: couldn't get the client port
  │ stack backtrace:
  │    0:        0x10459e49e - backtrace::backtrace::trace::h47a32bf8c6f00c9b
  │    1:        0x10459ebec - backtrace::capture::Backtrace::new::hb5a725a088a2a2fc
  │    2:        0x102e7d5b8 - servo::main::_$u7b$$u7b$closure$u7d$$u7d$::hab3ba3e402ac1f0b
  │    3:        0x1049ff9c9 - std::panicking::rust_panic_with_hook::h05996066754c6be9
  │    4:        0x102faa4f4 - std::panicking::begin_panic::haab64c72e0aa1aab
  │    5:        0x10300ecdc - layout_thread::LayoutThread::perform_post_style_recalc_layout_passes::h139d5d3cd94b81e4
  │    6:        0x10300d2d5 - layout_thread::LayoutThread::tick_all_animations::h90b400a44eeb0e6f
  │    7:        0x103003306 - layout_thread::LayoutThread::handle_request_helper::h307a64d67c3c4130
  │    8:        0x102f944a0 - profile_traits::mem::ProfilerChan::run_with_memory_reporting::h6184f9f4fcb8e847
  │    9:        0x103001173 - _$LT$layout_thread..LayoutThread$u20$as$u20$layout_traits..LayoutThreadFactory$GT$::create::_$u7b$$u7b$closure$u7d$$u7d$::h8028856a97c8c244
  │   10:        0x102faadfd - std::panicking::try::do_call::ha664f0ed882ba9a8
  │   11:        0x104a0099a - __rust_maybe_catch_panic
  │   12:        0x102fc41e6 - _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::hc02c54adca1db771
  │   13:        0x1049fc4e4 - std::sys::imp::thread::Thread::new::thread_start::h4008e1859fbd98b8
  │   14:     0x7fff8af9f059 - _pthread_body
  │   15:     0x7fff8af9efd6 - _pthread_start
  │ ERROR:servo: assertion failed: rw_data.newly_transitioning_nodes.is_empty()
  │ Pipeline failed in hard-fail mode.  Crashing!
  └ thread panicked while processing panic. aborting.

bors-servo · 2017-04-09T13:24:04Z

☔ The latest upstream changes (presumably #16316) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-04-10T09:57:19Z

@bors-servo: try

bors-servo · 2017-04-10T09:57:24Z

⌛ Trying commit 226e433 with merge bef0a38...


        Auto merge of #16295 - jdm:transition-safety, r=<try>

Root nodes for the duration of their CSS transitions This ensures that we can pass a node address as part of the asynchronous transition end notification, making it safe to fire the corresponding DOM event on the node from the script thread. Without explicitly rooting this node when the transition starts, we risk the node being GCed before the transition is complete. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14972 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16295)

bors-servo · 2017-04-10T10:21:15Z

💔 Test failed - linux-rel-wpt

jdm · 2017-04-10T22:51:00Z

@bors-servo: try

bors-servo · 2017-04-10T22:51:04Z

⌛ Trying commit 212f998 with merge 39f1fc5...


        Auto merge of #16295 - jdm:transition-safety, r=<try>

Root nodes for the duration of their CSS transitions This ensures that we can pass a node address as part of the asynchronous transition end notification, making it safe to fire the corresponding DOM event on the node from the script thread. Without explicitly rooting this node when the transition starts, we risk the node being GCed before the transition is complete. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14972 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16295)

bors-servo · 2017-04-10T23:26:31Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
State: approved= try=True

jdm · 2017-04-11T00:03:16Z

@nox do you want to review this?

nox · 2017-04-11T08:39:32Z

r? @nox

jdm · 2017-05-15T12:58:02Z

@bors-servo: r=nox

bors-servo · 2017-05-15T12:58:03Z

📌 Commit d26e45b has been approved by nox


        Root nodes for the duration of their CSS transitions.

This ensures that we can pass a node address as part of the asynchronous transition end notification, making it safe to fire the corresponding DOM event on the node from the script thread. Without explicitly rooting this node when the transition starts, we risk the node being GCed before the transition is complete.


        Accumulate transitioning nodes inside the layout context.


        Send information to script as part of finishing layout.

This avoids the need for multiple layout RPC operations immediately following return of control to script. This means that layout and script can continue to operate in parallel at this point, rather than one potentially waiting on the shared mutex to be unlocked.


        Warn about ignored transitions which can't be rooted.


        Make methods storing layout-originating nodes unsafe.

jdm · 2017-05-15T18:13:18Z

@bors-servo: r=nox

bors-servo · 2017-05-15T18:13:20Z

📌 Commit b0bf2b4 has been approved by nox

bors-servo · 2017-05-15T20:00:21Z

⌛ Testing commit b0bf2b4 with merge fa251ec...


        Auto merge of #16295 - jdm:transition-safety, r=nox

Root nodes for the duration of their CSS transitions This ensures that we can pass a node address as part of the asynchronous transition end notification, making it safe to fire the corresponding DOM event on the node from the script thread. Without explicitly rooting this node when the transition starts, we risk the node being GCed before the transition is complete. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14972 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16295)

bors-servo · 2017-05-15T21:38:09Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: nox
Pushing fa251ec to master...

highfive assigned KiChjang Apr 7, 2017

highfive added the S-awaiting-review label Apr 7, 2017

highfive added the S-tests-failed label Apr 7, 2017

highfive added the S-needs-rebase label Apr 9, 2017

jdm force-pushed the jdm:transition-safety branch from 66d93d2 to 204f531 Apr 10, 2017

highfive removed the S-tests-failed label Apr 10, 2017

jdm force-pushed the jdm:transition-safety branch from 204f531 to 226e433 Apr 10, 2017

highfive added the S-tests-failed label Apr 10, 2017

jdm force-pushed the jdm:transition-safety branch from 226e433 to 212f998 Apr 10, 2017

highfive removed the S-tests-failed label Apr 10, 2017

jdm removed the S-needs-rebase label Apr 11, 2017

highfive unassigned KiChjang Apr 11, 2017

nox added S-needs-code-changes and removed S-awaiting-review labels May 15, 2017

highfive added S-awaiting-merge and removed S-needs-rebase S-needs-code-changes labels May 15, 2017

jdm added S-needs-rebase and removed S-awaiting-merge labels May 15, 2017

jdm added 5 commits Apr 6, 2017

Accumulate transitioning nodes inside the layout context.

c3b9714

Warn about ignored transitions which can't be rooted.

2ca80a8

Make methods storing layout-originating nodes unsafe.

Loading status checks…

b0bf2b4

jdm force-pushed the jdm:transition-safety branch from d26e45b to b0bf2b4 May 15, 2017

highfive added the S-awaiting-review label May 15, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels May 15, 2017

This was referenced May 15, 2017

Implement :-moz-anonymous-content in stylo #16877

Merged

provide more information when bindgen fails #16880

Merged

bors-servo merged commit b0bf2b4 into servo:master May 15, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 15, 2017

This was referenced May 15, 2017

Mutation observer API #16668

Closed

Properly handle scroll offsets in hit testing #16865

Merged

servo / servo

Root nodes for the duration of their CSS transitions #16295

Root nodes for the duration of their CSS transitions #16295

jdm commented Apr 7, 2017 •

edited by larsbergstrom

highfive commented Apr 7, 2017

highfive commented Apr 7, 2017

jdm commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bors-servo commented Apr 7, 2017

jdm commented Apr 7, 2017

bors-servo commented Apr 9, 2017

jdm commented Apr 10, 2017

bors-servo commented Apr 10, 2017

bors-servo commented Apr 10, 2017

jdm commented Apr 10, 2017

bors-servo commented Apr 10, 2017

bors-servo commented Apr 10, 2017

jdm commented Apr 11, 2017

nox commented Apr 11, 2017

jdm commented May 15, 2017

bors-servo commented May 15, 2017

jdm commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

servo / servo

Join GitHub today

Root nodes for the duration of their CSS transitions #16295

Root nodes for the duration of their CSS transitions #16295

Conversation

jdm commented Apr 7, 2017 • edited by larsbergstrom

highfive commented Apr 7, 2017

highfive commented Apr 7, 2017

jdm commented Apr 7, 2017

bors-servo commented Apr 7, 2017

bors-servo commented Apr 7, 2017

jdm commented Apr 7, 2017

bors-servo commented Apr 9, 2017

jdm commented Apr 10, 2017

bors-servo commented Apr 10, 2017

bors-servo commented Apr 10, 2017

jdm commented Apr 10, 2017

bors-servo commented Apr 10, 2017

bors-servo commented Apr 10, 2017

jdm commented Apr 11, 2017

nox commented Apr 11, 2017

jdm commented May 15, 2017

bors-servo commented May 15, 2017

jdm commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

bors-servo commented May 15, 2017

jdm commented Apr 7, 2017 •

edited by larsbergstrom