Closed Bug 1377010 Opened 4 years ago Closed 4 years ago

stylo: Allow dropping rule nodes after rule tree teardown

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: birtles, Assigned: bholley)

References

Details

Attachments

(6 files)

Attached file 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.
Summary: stylo: → stylo: Assertion that the rule tree is empty fails when restyling root of display:none subtree in bfcached document
(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
We already gc before dropping the RuleTree.
Attachment #8882652 - Flags: review?(emilio+bugs)
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.
Attachment #8882653 - Flags: review?(emilio+bugs)
We're going to use null to indicate that the final GC has already
occurred.
Attachment #8882654 - Flags: review?(emilio+bugs)
Summary: stylo: Assertion that the rule tree is empty fails when restyling root of display:none subtree in bfcached document → stylo: Allow dropping rule nodes after rule tree teardown
Attachment #8882652 - Flags: review?(emilio+bugs) → review+
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 &*.
Attachment #8882653 - Flags: review?(emilio+bugs) → review+
Attachment #8882654 - Flags: review?(emilio+bugs) → review+
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 :)
Attachment #8882655 - Flags: review?(emilio+bugs) → review+
Pushed by bholley@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/be418ade341b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1378005
You need to log in before you can comment on or make changes to this bug.