stylo: Crash in NoteDirtyElement

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: calixte, Assigned: emilio)

Tracking

(Depends on 1 bug, Blocks 1 bug, {crash, regression})

58 Branch
mozilla58
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [clouseau], crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-6b1533ba-7f7f-4f1f-a757-dad520170924.
=============================================================

There are 23 crashes in nightly 58 with buildid 20170923100045. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1400936.

[1] https://hg.mozilla.org/mozilla-central/rev?node=dba974e6b1b4ecf1d0ee788e0279fd2b9f410b75
Flags: needinfo?(emilio)
Hmm, indeed this is caused by that patch. Looks like before either the !aElement->HasServoData() in ContentStateChanged or the !parent->HasServoData() check in NoteDirtyElement were preventing this from happening, but now we clear the data later.

So effectively what has changed is which invariant we enforce during unbinding. Before, we used to enforce "HasServoData() implies InComposedDoc()", but dishonored "child data implies parent data", and could leave all sorts of dirty state in the tree.

Changing this to honor _all_ our invariants would be ideal (that means clearing the IsInDocument flag _after_ unbinding the children), but that's specially tricky since there's tons of code that depend on it.

So there are two options:

 * Check IsInComposedDoc in the relevant places in ServoRestyleManager.
 * Clear the data _before_ unbinding the children, but the bits _after_.

I don't feel too strongly about any of those, so I'm going to go with (2).
Let me know if the first solution I posted early sounds better, Bobby, again I don't feel strongly about any of both.

Seems like changing the time we unset IsInDocument would break a bunch of stuff, including the SubtreeRoot tracking, so given I can't think of an easy fix for that I'm not even considering that (though It'd be perfect for us in that sense, since it'll allow us to keep all our invariants even during unbind).
Flags: needinfo?(emilio)
Comment on attachment 8911584 [details]
Bug 1402684: Clear the servo data early, but the flags later, in UnbindFromTree.

https://reviewboard.mozilla.org/r/182998/#review188184

Yeah, I think the HasServoData->IsInDoc invariant is important. I like this patch, since the net delta on top of bug 1400936 is just clearing some extra flags again, which is low risk (and risk around this stuff matters a lot right now).

r=me with those comment fixes, thanks for jumping on this. A crashtest would be nice if you can swing it.

::: dom/base/Element.cpp:2009
(Diff revision 1)
> -  // recompute it anyway if we ever insert the nodes back into a document.
> -  if (IsStyledByServo()) {
> -    if (document) {
> -      ClearServoData(document);
> -    } else {
> +  // "child has style data implies parent has it too". As such, the restyle root
> +  // tracking may incorrectly end up setting dirty bits on the parent chain when
> +  // moving from a not yet unbound root with already unbound parents to a root
> +  // higher up in the tree, so we clear those (again, since they're also cleared
> +  // in ClearServoData) here.

Mention that this specifically happens if an unbind impl triggers style invalidations?

Also would be good to be a bit more precise about where these bits get set (IIUC, between the element that gets invalidated and the detached root?).

::: dom/base/Element.cpp:2015
(Diff revision 1)
> -  if (IsStyledByServo()) {
> -    if (document) {
> -      ClearServoData(document);
> -    } else {
> -      MOZ_ASSERT(!HasServoData());
> -      MOZ_ASSERT(!HasAnyOfFlags(kAllServoDescendantBits | NODE_NEEDS_FRAME));
> +  // tracking may incorrectly end up setting dirty bits on the parent chain when
> +  // moving from a not yet unbound root with already unbound parents to a root
> +  // higher up in the tree, so we clear those (again, since they're also cleared
> +  // in ClearServoData) here.
> +  //
> +  // Note that clearing the data itself here has its own set of problems, since

s/has/would have/ to make it clear we don't do that.
Comment on attachment 8911584 [details]
Bug 1402684: Clear the servo data early, but the flags later, in UnbindFromTree.

https://reviewboard.mozilla.org/r/182998/#review188186

Whoops, forgot to set the flag. See comments above.
Attachment #8911584 - Flags: review?(bobbyholley) → review+
Assignee: nobody → emilio
Priority: -- → P2
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f44a80769c80
Clear the servo data early, but the flags later, in UnbindFromTree. r=bholley
Comment on attachment 8911584 [details]
Bug 1402684: Clear the servo data early, but the flags later, in UnbindFromTree.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1400936#c21

Approval Request Comment
[Feature/Bug causing the regression]: bug 1400936
[User impact if declined]: Crashes, but note that bug 1400936 isn't in beta yet.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet, but I'd expect no more crashes with this signature.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: not much in combination with bug 1400936, see the uplift request there.
[Why is the change risky/not risky?]: see the uplift request in bug 1400936.
[String changes made/needed]:
Attachment #8911584 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/f44a80769c80
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8911584 [details]
Bug 1402684: Clear the servo data early, but the flags later, in UnbindFromTree.

Fix a crash, taking in 57b3.
Attachment #8911584 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
the fix for this landed in 57.0b3 but crashes with this signature are still occurring there:
https://crash-stats.mozilla.com/signature/?signature=NoteDirtyElement#reports
Flags: needinfo?(emilio)
That is bug 1403028, I'll request an uplift for that now.
Flags: needinfo?(emilio)
Depends on: 1403511
Depends on: 1404134
You need to log in before you can comment on or make changes to this bug.