1377010 - stylo: Allow dropping rule nodes after rule tree teardown

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case

STR:
1. Load the attached test case in a debug Stylo build
2. Allow pop-ups for the test case
3. Reload the test case

For me it hits the panic here:

  http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/servo/components/style/stylist.rs#1305

i.e. we fail to GC the rule nodes.

In this test case we request computed style on a node in a bfcache document that is also the root of a display:none subtree.

I *think* the key factors are:

1. The node is part of a display:none subtree so getComputedStyle can't just pull the style context off the frame but needs to recompute it.
2. The node is the *root* of the display:none subtree so we *do* have styles for it that we return instead of calling resolve_style which takes care to drop the resolved styles.

(2) might not actually be important, however.

Assigning to Bobby since I believe he had an idea for how to fix this.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c386506645d46109504da8ad73e7f8fbb7ea4413

Comment 2

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c386506645d46109504da8ad73e7f8fbb7ea4413

This is orange due to the refcount logging I added. We're green without it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=928ebab2ccc4f3486ece6b1652639a86fddf1721

Comment 3

[Brian Birtles (:birtles)]

Attachment: Test case as mochitest

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop GCing before dropping the root rule node. v1

We already gc before dropping the RuleTree.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hook into the Gecko leak checking machinery. v1

Since we're going to stop asserting that all RuleNodes have been
destroyed by the time the RuleTree is destroyed, we want reliable leak
checking.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use a different sentinel value to lock the free list. v1

We're going to use null to indicate that the final GC has already
occurred.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Allow StrongRuleNode drops after RuleTree destruction. v1

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8882653 [details] [diff] [review]
Part 2 - Hook into the Gecko leak checking machinery. v1

Review of attachment 8882653 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those

::: servo/components/style/rule_tree/mod.rs
@@ +586,5 @@
> +    pub fn NS_LogDtor(aPtr: *const c_void, aTypeName: *const c_char, aSize: u32);
> +}
> +
> +lazy_static! {
> +    static ref NAME: CString = CString::new("RuleNode").unwrap();

instead of lazy_static!:

static NAME: &'static [u8] = b"RuleNode\0";

@@ +592,5 @@
> +
> +/// Logs the creation of a heap-allocated object to Gecko's leak-checking machinery.
> +pub fn log_ctor(ptr: *const RuleNode) {
> +    unsafe {
> +        NS_LogCtor(ptr as *const c_void, NAME.as_ptr(), size_of::<RuleNode>() as u32);

You need to cast the pointer to *const c_char, which is not guaranteed to be unsigned.

@@ +599,5 @@
> +
> +/// Logs the destruction of a heap-allocated object to Gecko's leak-checking machinery.
> +pub fn log_dtor(ptr: *const RuleNode) {
> +    unsafe {
> +        NS_LogDtor(ptr as *const c_void, NAME.as_ptr(), size_of::<RuleNode>() as u32);

ditto.

@@ +778,5 @@
>      fn new(n: Box<RuleNode>) -> Self {
>          debug_assert!(n.parent.is_none() == !n.source.is_some());
>  
>          let ptr = Box::into_raw(n);
> +        log_new(ptr);

You're missing two while inserting in the rule tree, please update them too.

@@ +997,5 @@
>              }
>  
>              debug!("GC'ing {:?}", weak.ptr());
>              node.remove_from_child_list();
> +            let ptr = Box::from_raw(weak.ptr());

You can also:

log_drop(weak.ptr());
let _ = Box::from_raw(weak.ptr());

Which avoids the &*.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8882655 [details] [diff] [review]
Part 4 - Allow StrongRuleNode drops after RuleTree destruction. v1

Review of attachment 8882655 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/rule_tree/mod.rs
@@ +57,5 @@
> +
> +        // Remove the sentinel. This indicates that GCs will no longer occur.
> +        // Any further drops of StrongRuleNodes must occur on the main thread,
> +        // and will trigger synchronous dropping of the Rule nodes.
> +        self.root.get().next_free.store(ptr::null_mut(), Ordering::Relaxed);

Please document it properly in the free list pointer :)

Comment 10

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e01b0d6adf5386af3cfe8be6b510d847f6270036

Comment 11

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/17585

https://hg.mozilla.org/integration/autoland/rev/281a72862b4883a67b3c0370d12100ac8396707c

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be418ade341b
Test case for getting the computed style for a node in a document in the bfcache. r=bholley

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/be418ade341b