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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: cpeterson, Unassigned)
References
Details
(Keywords: crash, Whiteboard: [stylo:m2])
Attachments
(8 files, 3 obsolete files)
1.36 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
8.12 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
20.97 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
4.57 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
5.84 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
6.71 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
8.49 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
14.97 KB,
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.
Comment 1•8 years ago
|
||
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.
Comment 2•8 years ago
|
||
Moving out of the HTML Parser component based on comment 1.
Component: HTML: Parser → CSS Parsing and Computation
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
Attachment #8781759 -
Flags: review?(cam)
Comment 6•8 years ago
|
||
This gives us more control over what gets restyled when.
Attachment #8781760 -
Flags: review?(cam)
Comment 7•8 years ago
|
||
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)
Comment 8•8 years ago
|
||
Attachment #8781762 -
Flags: review?(cam)
Comment 9•8 years ago
|
||
FYI emilio.
Comment 10•8 years ago
|
||
Comment 11•8 years ago
|
||
(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 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8781758 -
Attachment is obsolete: true
Attachment #8781758 -
Flags: review?(cam)
Comment 14•8 years ago
|
||
Comment 15•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8781757 -
Flags: review?(cam) → review+
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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 18•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8781762 -
Flags: review?(cam) → review+
Comment 19•8 years ago
|
||
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+
Comment 20•8 years ago
|
||
(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.
Comment 21•8 years ago
|
||
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)
Comment 22•8 years ago
|
||
Doing this in SetInDoc is kind of gross, and it leads to nice symmetry with
UnbindFromTree.
Attachment #8784633 -
Flags: review?(cam)
Comment 23•8 years ago
|
||
Attachment #8784634 -
Flags: review?(cam)
Updated•8 years ago
|
Attachment #8781759 -
Attachment is obsolete: true
Comment 24•8 years ago
|
||
Updated•8 years ago
|
Attachment #8781760 -
Attachment is obsolete: true
Attachment #8781760 -
Flags: review?(cam)
Comment 25•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8784633 -
Flags: review?(cam) → review+
Updated•8 years ago
|
Attachment #8784634 -
Flags: review?(cam) → review+
Comment 26•8 years ago
|
||
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 27•8 years ago
|
||
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+
Comment 28•8 years ago
|
||
(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).
Comment 29•8 years ago
|
||
Comment 30•8 years ago
|
||
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
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5c03203fbf49
https://hg.mozilla.org/mozilla-central/rev/fa3534af141f
https://hg.mozilla.org/mozilla-central/rev/0efe171aa2ed
https://hg.mozilla.org/mozilla-central/rev/bf5c0fb68628
https://hg.mozilla.org/mozilla-central/rev/543b6fffb61b
https://hg.mozilla.org/mozilla-central/rev/721abe87ead2
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•