Closed Bug 1292279 Opened 8 years ago Closed 8 years ago

Stylo crash in parser/htmlparser/tests/reftest/bug608373-1.html test

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:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: cpeterson, Unassigned)

References

Details

(Keywords: crash, Whiteboard: [stylo:m2])

Attachments

(8 files, 3 obsolete files)

Part 1 - Consult the dirty bits to determine whether we have pending restyles. v1
(deleted), patch
heycam
: review+
Details | Diff | Splinter Review
Part 5 - Add a helper to get the real container and use it in nsPresShell::Content{Inserted,Removed}. v1
(deleted), patch
heycam
: review+
Details | Diff | Splinter Review
Part 6 - Pass non-element containers through to ServoRestyleManager. v1
(deleted), patch
heycam
: review+
Details | Diff | Splinter Review
Part 2 - Add an API to propagate the HAS_DIRTY_DESCENDANTS bit upwards. v2
(deleted), patch
heycam
: review+
Details | Diff | Splinter Review
Part 0A - Add an API on nsIContent to propagate the dirty bit up the tree, and use it in ServoRestyleManager. v1
(deleted), patch
heycam
: review+
Details | Diff | Splinter Review
Part OB - Explicitly handle dirtiness in BindToTree. v1
(deleted), patch
heycam
: review+
Details | Diff | Splinter Review
Part 3 - Clear up the semantics of our Servo traversal APIs. v3
(deleted), patch
heycam
: review+
Details | Diff | Splinter Review
Part 4 - Explicitly style new children in Content{Appended/Inserted} rather than using restyle hints. v3
(deleted), patch
emilio
: review+
Details | Diff | Splinter Review 
To reproduce, run `./mach reftest --disable-e10s <filename>` or replace <filename> with `layout/reftest/reftest.list`.

TEST-UNEXPECTED-FAIL | file:///home/shinglyu/workspace/stylo/gecko-dev/parser/htmlparser/tests/reftest/bug608373-1.html | application terminated with exit code 6
REFTEST PROCESS-CRASH | file:///home/shinglyu/workspace/stylo/gecko-dev/parser/htmlparser/tests/reftest/bug608373-1.html | application crashed [None]

Crashes during the cascade because the <html> element (the parent of the element we're attempt to style) has not been styled for some reason.
This happens because we're trying to eagerly style a native anonymous subtree but the root hasn't been styled yet.

I'm working on fixing this in bug 1292662.
Depends on: 1292662
Moving out of the HTML Parser component based on comment 1.
Component: HTML: Parser → CSS Parsing and Computation
The current mechanism assumes that everything can be expressed in terms of restyle
hints, which has several limitations (in particular, we can't specify that a
subtree is dirty without also claiming that the root is dirty, since eRestyleSubtree
implies eRestyleSelf).

We may eventually decide that restyles hints give us everything we need, but while
we're experimenting I'd like the flexibility to do things both ways.
Attachment #8781757 - Flags: review?(cam)
I didn't end up needing this API for this bug, since it turns out that we
generally need to style new content immediately in Content{Appended/Inserted}.
But we may well need it when optimizing for lazy frame construction, so I want
to get it in tree.
Attachment #8781758 - Flags: review?(cam)
This gives us more control over what gets restyled when.
Attachment #8781760 - Flags: review?(cam)
The existing static_cast checks are totally wrong, by the way, since
nsIDocuments are never nsIContent. Looks like they were erroneously
added in bug 862763.
Attachment #8781761 - Flags: review?(cam)
FYI emilio.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Created attachment 8781759 [details] [diff] [review]
> Part 3 - Clear up the semantics of our Servo traversal APIs. v1

Thanks for doing this, at some point I said I'll do it, then I forgot about it :-)

FWIW next time you could have reminded me of doing it ;-)
Comment on attachment 8781759 [details] [diff] [review]
Part 3 - Clear up the semantics of our Servo traversal APIs. v1

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

Only a few comments, it's not a thorough review.

::: layout/base/ServoRestyleManager.cpp
@@ +177,5 @@
> +{
> +  MOZ_ASSERT(aRoot->IsDirtyForServo());
> +  FlattenedChildIterator it(aRoot);
> +  for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {
> +    MOZ_ASSERT(aRoot->HasDirtyDescendantsForServo());

Maybe instead of repeating this assertion for every child, grabbing the first node from the iterator early, then if there's no node, assert that !aRoot->HasDirtyDescendantsForServo()?

::: layout/style/ServoStyleSet.cpp
@@ +434,5 @@
> +    return;
> +  }
> +
> +  FlattenedChildIterator it(aContent);
> +  for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {

We probably could/should use the servo bottom-up traversal here, doesn't seem too disruptive to pass the flag, stick it into the context, and then return that from `needs_postorder_traversal`?

Although I think it's fine to do it as a followup I guess.
I didn't end up needing this API for this bug, since it turns out that we
generally need to style new content immediately in Content{Appended/Inserted}.
But we may well need it when optimizing for lazy frame construction, so I want
to get it in tree.
Attachment #8781770 - Flags: review?(cam)
Attachment #8781758 - Attachment is obsolete: true
Attachment #8781758 - Flags: review?(cam)
Comment on attachment 8781759 [details] [diff] [review]
Part 3 - Clear up the semantics of our Servo traversal APIs. v1

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

::: layout/base/ServoRestyleManager.cpp
@@ +177,5 @@
> +{
> +  MOZ_ASSERT(aRoot->IsDirtyForServo());
> +  FlattenedChildIterator it(aRoot);
> +  for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {
> +    MOZ_ASSERT(aRoot->HasDirtyDescendantsForServo());

Yeah either works - this just made the code a bit more compact, and I think the extra assertion overhead is dwarfed by the iterator overhead.

::: layout/style/ServoStyleSet.cpp
@@ +434,5 @@
> +    return;
> +  }
> +
> +  FlattenedChildIterator it(aContent);
> +  for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {

Yeah I'm planning to file a followup about optionally clearing the dirty bits from servo. It's also not clear to me whether it actually needs to be a bottom-up traversal or whether we can just do it immediately after the cascade.
Attachment #8781757 - Flags: review?(cam) → review+
Comment on attachment 8781759 [details] [diff] [review]
Part 3 - Clear up the semantics of our Servo traversal APIs. v1

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

::: layout/style/ServoStyleSet.cpp
@@ +6,5 @@
>  
>  #include "mozilla/ServoStyleSet.h"
>  
>  #include "mozilla/ServoRestyleManager.h"
> +#include "ChildIterator.h"

I guess this should be mozilla/dom/ChildIterator.h, though I'm not sure why the bare filename works here, outside of dom/.
Attachment #8781759 - Flags: review?(cam) → review+
Comment on attachment 8781760 [details] [diff] [review]
Part 4 - Explicitly style new children in Content{Appended/Inserted} rather than using restyle hints. v1

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

::: layout/base/ServoRestyleManager.cpp
@@ +306,5 @@
> +  // XXXbholley: Need the superclass logic too.
> +  //
> +
> +  AssertSubtreeIsExplicitlyDirty(aChild);
> +  StyleSet()->StyleNewSubtree(aChild);

In PresShell::CharacterDataChanged, we can call RestyleForInsertOrChange when a text node's contents changes and we have an :empty selector or similar that could be affected.  In this case the text node won't have the dirty bits set and we'll end up asserting, is that right?

@@ +320,5 @@
> +
> +#ifdef DEBUG
> +  nsIContent* currContent = aFirstNewContent;
> +  while (currContent) {
> +    AssertSubtreeIsExplicitlyDirty(currContent);

Same here too, from PresShell::CharacterDataChanged.
Comment on attachment 8781761 [details] [diff] [review]
Part 5 - Add a helper to get the real container and use it in nsPresShell::Content{Inserted,Removed}. v1

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

::: layout/base/nsPresShell.cpp
@@ +4336,5 @@
>                             int32_t     aNewIndexInContainer)
>  {
>    NS_PRECONDITION(!mIsDocumentGone, "Unexpected ContentAppended");
>    NS_PRECONDITION(aDocument == mDocument, "Unexpected aDocument");
> +  MOZ_ASSERT(aContainer, "Documents can only have one child, so we never call "

Documents can have at most one Element child, but also, a DocumentType and any number of Comments and ProcessingInstruction nodes.  These are are all nsIContent, and we can document.appendChild() them.  But given the NS_PRECONDITION here, do we still never call ContentAppended for those nodes?

If so, r=me, but can we add some assertions that aFirstNewContent must be an Element, and for good measure assert that aContainer must be an Element or DocumentFragment?  And then tweak the assertion message to say we rely on ContentAppended only being called for Element children (of which there can be at most one for Documents).

@@ +4395,2 @@
>  
> +  if (container == aDocument &&

(I reckon we could just assert that container is aDocument if we've got a DocumentType node -- I don't think they're allowed to be inserted anywhere else.)
Attachment #8781761 - Flags: review?(cam) → review+
Attachment #8781762 - Flags: review?(cam) → review+
Comment on attachment 8781770 [details] [diff] [review]
Part 2 - Add an API to propagate the HAS_DIRTY_DESCENDANTS bit upwards. v2

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

::: layout/base/ServoRestyleManager.h
@@ +74,5 @@
>    }
>  
>    nsresult ReparentStyleContext(nsIFrame* aFrame);
>  
> +  void NoteExplicitlyDirtyChildren(nsINode* aNode);

Can you add a short comment saying what this does -- I don't think it's clear from the name.
Attachment #8781770 - Flags: review?(cam) → review+
Depends on: 1296509
(In reply to Cameron McCormack (:heycam) from comment #18)
> But given the
> NS_PRECONDITION here, do we still never call ContentAppended for those nodes?

That's my understanding. The callers are a bit too convoluted for me to verify easily.
 
> If so, r=me, but can we add some assertions that aFirstNewContent must be an
> Element

It's not clear to me why we can assert that, since I could imagine that we might call it with a non-element aFirstNewContent in the non-document-container case.

, and for good measure assert that aContainer must be an Element or
> DocumentFragment?  And then tweak the assertion message to say we rely on
> ContentAppended only being called for Element children (of which there can
> be at most one for Documents).

I've added these assertions.
We don't need this API outside of its current consumers yet, but will probably
need it when we handle lazy frame construction.
Attachment #8784632 - Flags: review?(cam)
Doing this in SetInDoc is kind of gross, and it leads to nice symmetry with
UnbindFromTree.
Attachment #8784633 - Flags: review?(cam)
Attachment #8781759 - Attachment is obsolete: true
Attachment #8781760 - Attachment is obsolete: true
Attachment #8781760 - Flags: review?(cam)
Comment on attachment 8784632 [details] [diff] [review]
Part 0A - Add an API on nsIContent to propagate the dirty bit up the tree, and use it in ServoRestyleManager. v1

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

Can we keep doing this with a loop rather than using recursion?  It doesn't look like the recursion buys us anything and we may as well not pay the function call price while going up the tree.
Attachment #8784633 - Flags: review?(cam) → review+
Attachment #8784634 - Flags: review?(cam) → review+
Comment on attachment 8784632 [details] [diff] [review]
Part 0A - Add an API on nsIContent to propagate the dirty bit up the tree, and use it in ServoRestyleManager. v1

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

r=me if you change to a loop.
Attachment #8784632 - Flags: review?(cam) → review+
Comment on attachment 8784635 [details] [diff] [review]
Part 4 - Explicitly style new children in Content{Appended/Inserted} rather than using restyle hints. v3

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

::: layout/base/ServoRestyleManager.cpp
@@ +354,5 @@
> +    // and assertions here.
> +    return;
> +  }
> +
> +  StyleSet()->StyleNewSubtree(aChild);

Can you leave here a descriptive comment about why are we styling explicitly instead of going through the normal restyling path? (to avoid all the roundtrip given frame construction is going to be called immediately after this runs, and all that?)

Also, this unfortunately regresses things like handling restyles for positional selectors (:first-child, etc.), but per the talk today it seems to be acceptable. I guess the comment in RestyleForInsertOrChange takes care of announcing this, but being more explicit about it there would be nice IMO.

@@ +364,5 @@
>  ServoRestyleManager::RestyleForAppend(Element* aContainer,
>                                        nsIContent* aFirstNewContent)
>  {
> +  //
> +  // XXXbholley: Need the Gecko logic here too - see bug 1297899.

Also, please add to this comment something like, while we need all this logic, we don't need to post restyle events so aggressively given what the callers are, and the previous handling in ContentAppended.

@@ +380,5 @@
> +  }
> +
> +  if (aFirstNewContent->GetNextSibling()) {
> +    aContainer->SetHasDirtyDescendantsForServo();
> +    StyleSet()->StyleNewChildren(aContainer);

I guess given we're doing this if it has later siblings, we can also do it in the general case and avoid regressing it though. I'll leave it up to you, but known regressions are sort of scary, I guess.
Attachment #8784635 - Flags: review+
No longer depends on: 1292662
Blocks: 1292662
(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> > +  if (aFirstNewContent->GetNextSibling()) {
> > +    aContainer->SetHasDirtyDescendantsForServo();
> > +    StyleSet()->StyleNewChildren(aContainer);
> 
> I guess given we're doing this if it has later siblings, we can also do it
> in the general case and avoid regressing it though. I'll leave it up to you,
> but known regressions are sort of scary, I guess.

Well, not quite - in the later siblings case we're traversing from the parent, but that doesn't mean we're marking the parent as dirty. That's what I'm trying to avoid here (which should be correct once we share the gecko logic).
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c03203fbf49
Add an API on nsIContent to propagate the dirty bit up the tree, and use it in ServoRestyleManager. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3534af141f
Explicitly handle dirtiness in BindToTree. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0efe171aa2ed
Consult the dirty bits to determine whether we have pending restyles. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5c0fb68628
Clear up the semantics of our Servo traversal APIs. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/543b6fffb61b
Explicitly style new children in Content{Appended/Inserted} rather than using restyle hints. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/721abe87ead2
Add a helper to get the real container and use it in nsPresShell::Content{Inserted,Removed}. r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: