1371708 - Stylo: panic loading rules for a <link> element

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

DevTools tries to pull rules using inDOMUtils::GetCSSStyleRules when a node is selected in the inspector.  When you select a <link> element that references a stylesheet, such as:

  <link rel="stylesheet" type="text/css" href="style.css">

this appears to lead to a panic in tests like:

* devtools/client/inspector/markup/test/browser_markup_links_01.js[1]

17 INFO Testing attributes on node link
18 INFO Selecting the node for 'link'
GECKO(54309) | thread '<unnamed>' panicked at 'already immutably borrowed', /Users/jryans/projects/mozilla/gecko-dev-stylo-2/third_party/rust/atomic_refcell/src/lib.rs:198
GECKO(54309) | stack backtrace:
GECKO(54309) |    0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
GECKO(54309) |    1: std::panicking::default_hook::{{closure}}
GECKO(54309) |    2: std::panicking::default_hook
GECKO(54309) |    3: std::panicking::rust_panic_with_hook
GECKO(54309) |    4: std::panicking::begin_panic
GECKO(54309) |    5: std::panicking::begin_panic_fmt
GECKO(54309) |    6: atomic_refcell::AtomicBorrowRefMut::new
GECKO(54309) |    7: <atomic_refcell::AtomicRefCell<T>>::borrow_mut
GECKO(54309) |    8: style::gecko::data::PerDocumentStyleData::borrow_mut
GECKO(54309) |    9: geckoservo::glue::get_pseudo_style
GECKO(54309) |   10: geckoservo::glue::Servo_ResolveStyleLazily::{{closure}}::{{closure}}
GECKO(54309) |   11: <core::option::Option<T>>::and_then
GECKO(54309) |   12: geckoservo::glue::Servo_ResolveStyleLazily::{{closure}}
GECKO(54309) |   13: geckoservo::glue::Servo_ResolveStyleLazily::{{closure}}
GECKO(54309) |   14: style::traversal::resolve_style
GECKO(54309) |   15: Servo_ResolveStyleLazily
GECKO(54309) |   16: _ZN7mozilla13ServoStyleSet18ResolveStyleLazilyEPNS_3dom7ElementENS_20CSSPseudoElementTypeENS_18StyleRuleInclusionE
GECKO(54309) |   17: _ZN7mozilla13ServoStyleSet21ResolveTransientStyleEPNS_3dom7ElementEP7nsIAtomNS_20CSSPseudoElementTypeENS_18StyleRuleInclusionE
GECKO(54309) |   18: _ZN18nsComputedDOMStyle24DoGetStyleContextNoFlushEPN7mozilla3dom7ElementEP7nsIAtomP12nsIPresShellNS_9StyleTypeENS_13AnimationFlagE
GECKO(54309) |   19: _ZN18nsComputedDOMStyle15GetStyleContextEPN7mozilla3dom7ElementEP7nsIAtomP12nsIPresShellNS_9StyleTypeE
GECKO(54309) |   20: _ZN10inDOMUtils16GetCSSStyleRulesEP13nsIDOMElementRK9nsAStringPP18nsIArrayExtensions

[1]: https://treeherder.mozilla.org/logviewer.html#?job_id=105301367&repo=try&lineNumber=12449

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1371708 - Stop borrowing PerDocumentStyleData twice when resolving lazy pseudo styles.

Review commit: https://reviewboard.mozilla.org/r/148926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148926/

Comment 2

[Cameron McCormack (:heycam)]

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8877480 [details]
Bug 1371708 - Stop borrowing PerDocumentStyleData twice when resolving lazy pseudo styles.

https://reviewboard.mozilla.org/r/148926/#review153382

I'd swear I had written a very similar patch to this one and sent it for review, but apparently I forgot about the second part... Shrug. r=me

::: servo/ports/geckolib/glue.rs:2569
(Diff revision 1)
>      debug_assert!(!snapshots.is_null());
>      let global_style_data = &*GLOBAL_STYLE_DATA;
>      let guard = global_style_data.shared_lock.read();
>      let element = GeckoElement(element);
>      let doc_data = PerDocumentStyleData::from_ffi(raw_data);
> +    let data = doc_data.borrow();

Well, this adds a borrow in the common case, but I guess that's fine.

Comment 4

[Cameron McCormack (:heycam)]

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

Comment 7

[Cameron McCormack (:heycam)]

merged.