1402684 - stylo: Crash in NoteDirtyElement

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

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

Comment 1

[Emilio Cobos Álvarez (: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).

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1402684: Clear the servo data early, but the flags later, in UnbindFromTree.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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).

Comment 4

[Bobby Holley (:bholley)]

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 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8911584 [details]
Bug 1402684: Clear the servo data early, but the flags later, in UnbindFromTree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182998/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f44a80769c80
Clear the servo data early, but the flags later, in UnbindFromTree. r=bholley

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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]:

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f44a80769c80

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911584 [details]
Bug 1402684: Clear the servo data early, but the flags later, in UnbindFromTree.

Fix a crash, taking in 57b3.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3fa652b178a2

Comment 12

[[:philipp]]

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

That is bug 1403028, I'll request an uplift for that now.