Closed Bug 1321284 Opened 8 years ago Closed 8 years ago

stylo: crash in nsCSSFrameConstructor::GetInsertionPrevSibling when trying to reframe native anonymous content

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(7 files, 1 obsolete file)

Bug 1321284 - Part 1: Make StyleChildrenIterator skip NAC generated by root element primary frame ancestors.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1321284 - Part 2: Add nsINode::GetFlattenedTreeParentNodeForStyle.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1321284 - Part 3: Make Servo-based styling use the "flattened tree parent for style".
58 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1321284 - Part 4: Factor out AllChildrenIterator's getting of document level NAC to a utility method.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1321284 - Part 5: Process document level NAC when restyling.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1321284 - Part 4.1: Add function to determine if we need to traverse from a given node for restyling.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1321284 - Part 4.2: Add iterator class to find all restyle roots.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Attached patch patch (obsolete) — — Splinter Review
We currently get this failure on all reftest-print tests: Assertion failure: aChild->GetProperty(nsGkAtoms::restylableAnonymousNode) (Someone passed native anonymous content directly into frame construction. Stop doing that!), at /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6815 TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/reftest-sanity/page-width-3.9in.html | application terminated with exit code 11 REFTEST PROCESS-CRASH | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/reftest-sanity/page-width-3.9in.html | application crashed [@ nsCSSFrameConstructor::GetInsertionPrevSibling] This is because: 1. We create the native anonymous content (scrollbars) of the root scroll frame in the paged print view before we style the root element. 2. We then try to eagerly style the newly created NAC, but we skip doing it since the root element (its parent) hasn't been styled yet. 3. We process pending restyles after the new pres shell for the print view has been set up, encounter the unstyled scrollbars, and generate ReconstructFrame hints for them since they've never been styled. 4. We assert when trying to handle the NAC frame reconstruction (which isn't something that can be handled). Stock Gecko resolves the styles context for those scroll bars with the parent style coming from the nsHTMLScrollFrame, which wraps the primary frame for the document element (the primary frame at this point still not having been created). Style restyling the NAC would make them inherit from the root element, which isn't really right, since then document styles could affect how the scroll bars look due to inheritance. It's probably not that important to inherit from the nsHTMLScrollFrame specifically. I think we could get away with pretending there is no parent element to inherit from, at the time we initially style the scrollbars. I'm not super happy with this, so if you have a better suggestion on how to handle the root element NAC styling I'm all ears. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4b048eeaa6abc0c34968d14cd8cc49b3902628
Attachment #8815701 - Flags: review?(bobbyholley)
Comment on attachment 8815701 [details] [diff] [review] patch Review of attachment 8815701 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Cameron McCormack (:heycam) from comment #0) > Created attachment 8815701 [details] [diff] [review] > patch > > We currently get this failure on all reftest-print tests: Unfortunately I can't reproduce this locally, presumably because mac does something slightly different. So my ability to experiment on my local machine is limited, FWIW. > 1. We create the native anonymous content (scrollbars) of the root scroll > frame in the paged print view before we style the root element. > > 2. We then try to eagerly style the newly created NAC, but we skip doing it > since the root element (its parent) hasn't been styled yet. Where does this skipping happen? > 3. We process pending restyles after the new pres shell for the print view > has been set up, encounter the unstyled scrollbars, and generate > ReconstructFrame hints for them since they've never been styled. > > 4. We assert when trying to handle the NAC frame reconstruction (which isn't > something that can be handled). Hm, my initial instinct was just to hack something up for this specific case during setup, but see below. > Stock Gecko resolves the styles context for those scroll bars with the > parent style coming from the nsHTMLScrollFrame, which wraps the primary > frame for the document element (the primary frame at this point still not > having been created). Style restyling the NAC would make them inherit from > the root element, which isn't really right, since then document styles could > affect how the scroll bars look due to inheritance. Ok. So this means that it isn't really a transient edge case during initialization as much as a fundamental difference of how the style inheritance should work, right? In particular, this also affects restyle when we modify the root style. In stock Gecko, I'm guessing we never reach the root scrollbars during traversal, since the highest restyle root is the root element, and its primary frame sits below the root scroll frame in the frame tree. But with Servo, we need a persistent mechanism to indicate that the root scrollbars don't inherit from the root element. > It's probably not that important to inherit from the nsHTMLScrollFrame > specifically. I think we could get away with pretending there is no parent > element to inherit from Yeah, ISTR dbaron or bz saying something similar last summer when we chatted about it. > , at the time we initially style the scrollbars. Per above, I think the issue isn't really limited to initial styling. > I'm not super happy with this, so if you have a better suggestion on how to > handle the root element NAC styling I'm all ears. I think the general approach is correct, but I'd rather move this handling into Gecko, specifically into GetFlattenedTreeParentNode, where all of our other nasty Gecko cases live. Note that we'll need to fix up the slow-path detection in the inline function in addition to the slow path itself. While we're at it, we should also fix up StyleChildrenIterator to skip the root scroll frame, so that we don't perform unnecessary traversals of the frame whenever we restyle the root. I'd write up a patch myself, but since I can't test it it's probably better for you to do it. > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4b048eeaa6abc0c34968d14cd8cc49b3902628 Good to see fewer crashes there! ::: servo/components/style/gecko/wrapper.rs @@ +624,5 @@ > } > + > + fn parent_element_for_styling(&self) -> Option<Self> { > + self.parent_element().and_then(|p| { > + if p.is_root() && self.is_native_anonymous_content_root() && p.get_data().is_none() { Per previous discussion, it seems like the is_none check is wrong, since we want this relationship to extend past initial styling. ::: servo/components/style/selector_parser.rs @@ +103,5 @@ > fn is_link(&self) -> bool; > > fn matches_user_and_author_rules(&self) -> bool; > + > + fn parent_element_for_styling(&self) -> Option<Self>; FWIW, I think a method like this should go in TElement, which avoids the duplication here. I'm pretty sure we don't need ElementExt anymore now that we've merge the crates and switched to conditional compilation.
Attachment #8815701 - Flags: review?(bobbyholley) → review-
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1) > > 2. We then try to eagerly style the newly created NAC, but we skip doing it > > since the root element (its parent) hasn't been styled yet. > > Where does this skipping happen? https://github.com/servo/servo/blob/e315da07319c115bd85f7da1baa1cf0577a1980b/ports/geckolib/glue.rs#L134-L141 > In stock Gecko, I'm guessing we never reach the root scrollbars during > traversal, since the highest restyle root is the root element, and its > primary frame sits below the root scroll frame in the frame tree. But with > Servo, we need a persistent mechanism to indicate that the root scrollbars > don't inherit from the root element. I think that's right. We do restyle them, if we post restyles for them explicitly, but I don't think we traverse into them from the root. > I think the general approach is correct, but I'd rather move this handling > into Gecko, specifically into GetFlattenedTreeParentNode, where all of our > other nasty Gecko cases live. Note that we'll need to fix up the slow-path > detection in the inline function in addition to the slow path itself. Is this really the best place for it? I feel like that then alters what the "flattened tree" concept in Gecko means, if we special case these root element NAC children. Although NAC already makes DOM concepts like the flattened tree strange, so perhaps it's OK. > While we're at it, we should also fix up StyleChildrenIterator to skip the > root scroll frame, so that we don't perform unnecessary traversals of the > frame whenever we restyle the root. Well, we do want to be able to restyle them, but we don't need to as a result of having restyled the root element. (I think we animate their opacity, for example, on macOS to do the fading in and out effect.) We could add some more logic to the StylingMode calculation that means "traverse children only if those children don't inherit from the current element" but maybe that's more trouble than it's worth.
(In reply to Cameron McCormack (:heycam) from comment #2) > Is this really the best place for it? I feel like that then alters what the > "flattened tree" concept in Gecko means, if we special case these root > element NAC children. Well, just the scroll frame. And it seems conceptually correct, right? The point of the flattened tree parent is to be the parent as far as the style system is concerned. And as far as the style system is concerned, these elements do not have a parent/child relationship with the root. > Although NAC already makes DOM concepts like the > flattened tree strange, so perhaps it's OK. My gut instinct is that it's ok, but definitely worth a try push and signoff from dbaron. > > While we're at it, we should also fix up StyleChildrenIterator to skip the > > root scroll frame, so that we don't perform unnecessary traversals of the > > frame whenever we restyle the root. > > Well, we do want to be able to restyle them, but we don't need to as a > result of having restyled the root element. (I think we animate their > opacity, for example, on macOS to do the fading in and out effect.) Oh that's a good point. But maybe we should just explicitly check dirtiness and traverse it in ServoStyleSet::StyleDocument instead?
I guess it wasn't clear to me that the flattened tree concept was only for styling purposes. If it is, then I agree it's fine to change. FWIW, there is other NAC that frames for the root element generate, such as those for the nsCanvasFrame: http://searchfox.org/mozilla-central/source/layout/generic/nsCanvasFrame.cpp#72 But they too shouldn't inherit from the root element's styles. > Oh that's a good point. But maybe we should just explicitly check dirtiness and traverse it > in ServoStyleSet::StyleDocument instead? OK, we can do that.
(In reply to Cameron McCormack (:heycam) from comment #4) > I guess it wasn't clear to me that the flattened tree concept was only for > styling purposes. If it is, then I agree it's fine to change. I guess that's not entirely true. The best spec reference I can find is here: https://w3c.github.io/webcomponents/spec/shadow/ All this root NAC is totally proprietary though, so it's not clear to me what would break with any of those things if the NAC wasn't a child of the root in the flat tree.
After some IRC discussion, we decided indeed to make StyleChildrenIterator not find this document level NAC, and to have a separate GetFlattenedTreeParentElementForStyle method that has this slightly different behaviour. I have some patches, which solve the problems with the print reftests, but now scrollbars don't appear for me. There might be a problem processing restyles for them. I will look into that tomorrow, but for now I am going to post some patches without requesting review, so you can see what I've got. One unfortunate thing is that it makes HasPendingRestyles a bit more complicated.
Comment on attachment 8816078 [details] Bug 1321284 - Part 1: Make StyleChildrenIterator skip NAC generated by root element primary frame ancestors. https://reviewboard.mozilla.org/r/96874/#review97324 ::: dom/base/ChildIterator.cpp:585 (Diff revision 1) > - // If the node has native anonymous content, return true. > + // If the node is not the document element and it has native anonymous > + // content, retun true. > + if (aElement != aElement->OwnerDoc()->GetRootElement()) { > - nsIAnonymousContentCreator* ac = do_QueryFrame(aElement->GetPrimaryFrame()); > + nsIAnonymousContentCreator* ac = do_QueryFrame(aElement->GetPrimaryFrame()); > - if (ac) { > + if (ac) { > - return true; > + return true; > - } > + } Don't we need to handle non-document level NAC for the root element? ::: dom/base/ChildIterator.cpp:586 (Diff revision 1) > return true; > } > } > > - // If the node has native anonymous content, return true. > + // If the node is not the document element and it has native anonymous > + // content, retun true. Nit: "return".
Comment on attachment 8816079 [details] Bug 1321284 - Part 2: Add nsINode::GetFlattenedTreeParentNodeForStyle. https://reviewboard.mozilla.org/r/96876/#review97328 ::: dom/base/nsIContentInlines.h:57 (Diff revision 1) > (parent && parent->IsContent() && > - parent->AsContent()->GetShadowRoot()); > + (parent->AsContent()->GetShadowRoot() || I think the grouping here is wrong?
Comment on attachment 8816080 [details] Bug 1321284 - Part 3: Make Servo-based styling use the "flattened tree parent for style". https://reviewboard.mozilla.org/r/96878/#review97330 ::: layout/style/ServoBindings.cpp:68 (Diff revision 1) > Gecko_GetParentNode(RawGeckoNodeBorrowed aNode) > { > - return aNode->GetFlattenedTreeParentNode(); > + return aNode->GetFlattenedTreeParentNodeForStyle(); > } We also need this on Gecko_GetParentElement, no?
Comment on attachment 8816082 [details] Bug 1321284 - Part 5: Process document level NAC when restyling. https://reviewboard.mozilla.org/r/96882/#review97332 ::: layout/base/ServoRestyleManager.h:89 (Diff revision 1) > - return root && root->HasDirtyDescendantsForServo(); > + Element* root = doc->GetRootElement(); > + if (root && root->HasDirtyDescendantsForServo()) { > + return true; > + } > + AutoTArray<nsIContent*, 8> elements; > + nsContentUtils::AppendDocumentLevelNativeAnonymousContentTo(doc, elements); Can we make this API operate on Elements, or do we ever actually have non-Element document-level NAC? ::: layout/style/ServoStyleSet.cpp:457 (Diff revision 1) > nsIDocument* doc = mPresContext->Document(); > Element* root = doc->GetRootElement(); > MOZ_ASSERT(root); > > - // Restyle the document. > + // Restyle the document from the root element. > + if (root->HasDirtyDescendantsForServo()) { This isn't right - the root itself may need styling. Same below. Though it looks like we make the same mistake in HasPendingRestyles. We should factor the correct thing [1] out into a helper? [1] The correct thing will unfortunately need to be an FFI call for now I think, to detect whether the element data is either Restyle or initial_unstyled.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15) > [1] The correct thing will unfortunately need to be an FFI call for now I > think, to detect whether the element data is either Restyle or > initial_unstyled. Though I guess maybe it's out of scope for this bug. I'm also introducing a nicer mechanism for this on the Servo side in another PR. So maybe just create the helper that just checks the descendants bit for now, add a FIXME, file a bug, and NeedInfo me?
Comment on attachment 8816082 [details] Bug 1321284 - Part 5: Process document level NAC when restyling. https://reviewboard.mozilla.org/r/96882/#review97332 > Can we make this API operate on Elements, or do we ever actually have non-Element document-level NAC? I doubt we do. Turns out to be slightly annoying making this API deal with it; I'll deal with it elsewhere.
Blocks: 1321766
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16) > Though I guess maybe it's out of scope for this bug. I'm also introducing a > nicer mechanism for this on the Servo side in another PR. So maybe just > create the helper that just checks the descendants bit for now, add a FIXME, > file a bug, and NeedInfo me? I needed to fix that here, turns out. Filed bug 1321766 to change it to that nicer mechanism.
Attachment #8815701 - Attachment is obsolete: true
Even with these patches and bug 1321754 worked around, scrollbars still don't work properly on Linux, although they do get displayed again in their broken state, like they used to recently. Filed bug 1321769 to get them to work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=509132dcfc11f8b11384c83c2904022e1c22ac81
Comment on attachment 8816078 [details] Bug 1321284 - Part 1: Make StyleChildrenIterator skip NAC generated by root element primary frame ancestors. https://reviewboard.mozilla.org/r/96874/#review97602
Attachment #8816078 - Flags: review?(bobbyholley) → review+
Comment on attachment 8816079 [details] Bug 1321284 - Part 2: Add nsINode::GetFlattenedTreeParentNodeForStyle. https://reviewboard.mozilla.org/r/96876/#review97606 ::: dom/base/nsIContentInlines.h:60 (Diff revision 2) > + (Type == nsIContent::eForStyle && > + aNode->IsContent() && > + aNode->AsContent()->IsRootOfNativeAnonymousSubtree() && > + aNode->OwnerDoc()->GetRootElement() == parent))); The indentation here is off. And IMO it's better to group the cases separately, and not include both cases in the same "(parent &&" expression. The newly-added case probably doesn't need an explicit parent null check.
Attachment #8816079 - Flags: review?(bobbyholley) → review+
Comment on attachment 8816080 [details] Bug 1321284 - Part 3: Make Servo-based styling use the "flattened tree parent for style". https://reviewboard.mozilla.org/r/96878/#review97608
Attachment #8816080 - Flags: review?(bobbyholley) → review+
Comment on attachment 8816081 [details] Bug 1321284 - Part 4: Factor out AllChildrenIterator's getting of document level NAC to a utility method. https://reviewboard.mozilla.org/r/96880/#review97604 ::: dom/base/Element.h:404 (Diff revision 2) > } > > Directionality GetComputedDirectionality() const; > > inline Element* GetFlattenedTreeParentElement() const; > + inline Element* GetFlattenedTreeParentElementForStyle() const; Please move this along with the definition into Part 2.
Attachment #8816081 - Flags: review?(bobbyholley) → review+
Comment on attachment 8816412 [details] Bug 1321284 - Part 4.1: Add function to determine if we need to traverse from a given node for restyling. https://reviewboard.mozilla.org/r/97162/#review97610 ::: dom/base/ElementInlines.h:52 (Diff revision 1) > +inline bool > +Element::MustTraverseForServo() Can you change the naming to ShouldTraverse? That'll match what I'm doing in Servo-land better. ::: servo/ports/geckolib/glue.rs:205 (Diff revision 1) > +pub extern "C" fn Servo_Element_MustTraverse(element: RawGeckoElementBorrowed) -> bool { > + if let Some(data) = GeckoElement(element).get_data() { Can you debug_assert against element.has_dirty_descendants() here?
Attachment #8816412 - Flags: review?(bobbyholley) → review+
Comment on attachment 8816413 [details] Bug 1321284 - Part 4.2: Add iterator class to find all restyle roots. https://reviewboard.mozilla.org/r/97164/#review97612
Attachment #8816413 - Flags: review?(bobbyholley) → review+
Comment on attachment 8816082 [details] Bug 1321284 - Part 5: Process document level NAC when restyling. https://reviewboard.mozilla.org/r/96882/#review97614
Attachment #8816082 - Flags: review?(bobbyholley) → review+
Whew, glad I punted on this in bug 1317016! Let's get this landed and sort out the scrollbar issues in followups.
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f1bb9b5c75c0 Part 1: Make StyleChildrenIterator skip NAC generated by root element primary frame ancestors. r=bholley https://hg.mozilla.org/integration/autoland/rev/25cd51dd7c98 Part 2: Add nsINode::GetFlattenedTreeParentNodeForStyle. r=bholley https://hg.mozilla.org/integration/autoland/rev/5654f66d96a0 Part 3: Make Servo-based styling use the "flattened tree parent for style". r=bholley https://hg.mozilla.org/integration/autoland/rev/f26dabbb6e09 Part 4: Factor out AllChildrenIterator's getting of document level NAC to a utility method. r=bholley https://hg.mozilla.org/integration/autoland/rev/a52bb1232e77 Part 4.1: Add function to determine if we need to traverse from a given node for restyling. r=bholley https://hg.mozilla.org/integration/autoland/rev/1cb9bcc84d12 Part 4.2: Add iterator class to find all restyle roots. r=bholley https://hg.mozilla.org/integration/autoland/rev/3976b04bf08a Part 5: Process document level NAC when restyling. r=bholley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: