Closed Bug 1394935 Opened 2 years ago Closed 2 years ago

stylo: panicked at '<div id=subwrap> (0xb264dc0) has still dirty bit true or animation-only dirty bit false'

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: bc, Assigned: emilio)

References

()

Details

(Keywords: crash)

Attachments

(13 files, 3 obsolete files)

log
143.05 KB, text/plain
Details
1.51 KB, text/plain
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
59 bytes, text/x-review-board-request
bholley
: review+
Details
1.42 KB, text/plain
bholley
: review+
Details
Attached file log
1. STYLO_FORCE_ENABLED=1
2. http://www.kongregate.com/games/graebor/sort-the-court
   in debug build on Linux
3. Crash

Crash reason:  EXCEPTION_ILLEGAL_INSTRUCTION
Crash address: 0x1046a8f3
Assertion: Unknown assertion type 0x00000000
Process uptime: not available

Thread 0 (crashed)
 0  xul.dll!panic_abort::__rust_start_panic [lib.rs:0ade339411587887bf01bcfa2e9ae4414c8900d4 : 55 + 0x3]
    eip = 0x1046a8f3   esp = 0x0102ed00   ebp = 0x0102ed00   ebx = 0x0102ee70
    esi = 0x14ab8bc8   edi = 0x1024dc30   eax = 0x00000011   ecx = 0x15cf1dc0
    edx = 0x14ac0610   efl = 0x00010202

thread '<unnamed>' panicked at '<div id=subwrap> (0xb264dc0) has still dirty bit true or animation-only dirty bit false', Z:/build/build/src/servo/ports/geckolib/glue.rs:3311

This is with a fresh build from https://hg.mozilla.org/mozilla-central/rev/e336d84fc1d2d1fde7387dd5f86fe06fa59abe10

log contains sections

==== Marionette Log ====
marionette stuff
crash report
==== Gecko Log ====
gecko log stuff
This might be the same as bug 1394586. Bob, can you reproduce with that patch applied?
Flags: needinfo?(bob)
I lost the ability to reproduce with the original url with or without the patch. Fortunately, I had a number of other urls to try.

With the patch, https://www.neowin.net/ reproduces

thread '<unnamed>' panicked at '<div id=global_below_community_activity class="ad"> (0x7f9f718fcdc0) has still dirty bit true or animation-only dirty bit false', /mozilla/builds/nightly/mozilla/servo/ports/geckolib/glue.rs:3311

I still have 18 more urls in case that one stops working. ;-)
Flags: needinfo?(bob)
Priority: -- → P2
Could you post them all in this bug? Tomorrow I'll investigate these, and it'd be helpful to see if the others repro too, to be able to work on them sooner if they happen to be different bugs.

Thanks!
Flags: needinfo?(bob)
(ni? me so I don't forget about this bug)
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Attached file urls
I ran these quickly through with some reloading to see if that would help crash. I marked the ones that reproduced a related dirty panic. One has an unrelated assertion I have yet to file. Also note, some of these originally reproduced only on Windows.
Flags: needinfo?(bob)
Thank you.

I've been distracted today fixing bustage, but I debugged this and found a few sources of fishyness in the current restyle root patch.

This crash is a regression from that. I have fixes in progress.
Depends on: 1383332
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b5aae17a20cf297339093d09862a8c1693cdc85&selectedJob=127112211

That looks green, modulo some extra assertions I added that also fail on trunk, and I'll address separately.
Comment on attachment 8902917 [details]
Bug 1394935: Add a special case for marking something as dirty from invalidation code.

https://reviewboard.mozilla.org/r/174636/#review179706
Attachment #8902917 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902918 [details]
Bug 1394935: Add a (commented out for now) assertion about clobbering dirty bits.

https://reviewboard.mozilla.org/r/174638/#review179708

File a bug on this just so we don't lose track of it? r=me with that.
Attachment #8902918 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902919 [details]
Bug 1394935: Assert that the content we're marking dirty is under the restyle root.

https://reviewboard.mozilla.org/r/174640/#review179712

::: commit-message-b1339:3
(Diff revision 1)
> +Bug 1394935: Assert that the content we're marking dirty is under the restyle root. r?bholley
> +
> +This would also have catched the bug earlier.

Nit: s/catched/caught/
Attachment #8902919 - Flags: review?(bobbyholley) → review+
Blocks: 1395351
Comment on attachment 8902920 [details]
Bug 1394935: Make the restyle root thing more obviously correct and less error prone.

https://reviewboard.mozilla.org/r/174642/#review179764

We discussed this at length on IRC and came up with a compromise approach.
Attachment #8902920 - Flags: review?(bobbyholley) → review-
Attachment #8902918 - Attachment is obsolete: true
Attachment #8902919 - Attachment is obsolete: true
Attachment #8902920 - Attachment is obsolete: true
Comment on attachment 8902917 [details]
Bug 1394935: Add a special case for marking something as dirty from invalidation code.

Mozreview is drunk
Attachment #8902917 - Flags: review+ → review?(bobbyholley)
Blocks: 1394560
Comment on attachment 8902917 [details]
Bug 1394935: Add a special case for marking something as dirty from invalidation code.

Mozreview is still drunk, sorry for the bugspam
Attachment #8902917 - Flags: review?(bobbyholley)
Comment on attachment 8902917 [details]
Bug 1394935: Add a special case for marking something as dirty from invalidation code.

https://reviewboard.mozilla.org/r/174636/#review179878

::: dom/base/Element.h:478
(Diff revision 4)
>      ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO |
>      NODE_DESCENDANTS_NEED_FRAMES;
>  
> +  /**
> +   * Notes that something in the given subtree of `aSubtreeRoot` needs dirtying,
> +   * and that all the relevant dirty bits have already been setup.

I'd say "...have already been propagated up to the given element."

::: dom/base/Element.cpp:4426
(Diff revision 4)
> -  MOZ_ASSERT(doc->GetServoRestyleRootDirtyBits() & aBit);
> +  MOZ_ASSERT(doc->GetServoRestyleRootDirtyBits() & aBits);
> +}
> +
> +/* static */
> +void
> +Element::NoteDirtySubtreeForServo(Element& aSubtreeRoot)

Let's make this non-static, and have it operate on |this| instead of an arg. The comment will need to change as well.

Also, though it doesn't matter given the above, Element* in dom.

::: dom/base/Element.cpp:4428
(Diff revision 4)
> +  MOZ_ASSERT(aSubtreeRoot.IsInComposedDoc());
> +  MOZ_ASSERT(aSubtreeRoot.HasServoData());
> +

I think we can drop these assertions, since we'll get equivalent checking when we call into NoteDirtyElement, and it'll keep this method smaller.

::: dom/base/Element.cpp:4433
(Diff revision 4)
> +  MOZ_ASSERT(aSubtreeRoot.IsInComposedDoc());
> +  MOZ_ASSERT(aSubtreeRoot.HasServoData());
> +
> +  nsIDocument* doc = aSubtreeRoot.GetComposedDoc();
> +  nsINode* existingRoot = doc->GetServoRestyleRoot();
> +  uint32_t bits = existingRoot ? doc->GetServoRestyleRootDirtyBits() : 0;

Maybe |existingBits|?

::: dom/base/Element.cpp:4437
(Diff revision 4)
> +  // NOTE(emilio): This could return true unnecessarily if the root is a
> +  // scrollbar or children of it, and aSubtreeRoot is the document element, but
> +  // it'll do the right thing anyway, given PropagateBits will return null.

Per IRC, I think it would be easier to reason about to just add a |ContentIsFlattenedTreeDescendantOfForStyle| which would use the style flattened tree, and then there's no ambiguity here and we can drop the comment and the scrollbar handling.

::: layout/style/ServoBindings.cpp:373
(Diff revision 4)
>  void
>  Gecko_NoteDirtyElement(RawGeckoElementBorrowed aElement)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  const_cast<Element*>(aElement)->NoteDirtyForServo();
> +  Element::NoteDirtySubtreeForServo(*const_cast<Element*>(aElement));

Hm, I guesss this lets you avoid doing any servo-side changes? I'd really prefer if we only called into this function from stylesheet invalidation, and not from the other callsites. I'd be ok with leaving it like this for now if you bundle a fix into your next coordiland.
Attachment #8902917 - Flags: review?(bobbyholley) → review-
Comment on attachment 8902981 [details]
Bug 1394935: Assert that the new root is always higher up in the tree than the old root.

https://reviewboard.mozilla.org/r/174744/#review180060

::: dom/base/nsIDocumentInlines.h:70
(Diff revision 2)
>    MOZ_ASSERT(aDirtyBits);
>    MOZ_ASSERT((aDirtyBits & ~Element::kAllServoDescendantBits) == 0);
>  
> +  MOZ_ASSERT(!mServoRestyleRoot ||
> +             mServoRestyleRoot == aRoot ||
> +             nsContentUtils::ContentIsFlattenedTreeDescendantOf(mServoRestyleRoot, aRoot));

Let's switch this to the ForStyle variant.
Attachment #8902981 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902982 [details]
Bug 1394935: Add a (commented out for now) assertion about clobbering dirty bits.

https://reviewboard.mozilla.org/r/174746/#review180062
Attachment #8902982 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902983 [details]
Bug 1394935: Assert that the content we're marking dirty is under the restyle root.

https://reviewboard.mozilla.org/r/174748/#review180064

::: dom/base/Element.cpp:4390
(Diff revision 2)
>        doc->SetServoRestyleRoot(doc, existingBits | aBit);
>      }
>    }
>  
>    MOZ_ASSERT(aElement == doc->GetServoRestyleRoot() ||
> +             nsContentUtils::ContentIsFlattenedTreeDescendantOf(

ForStyle.
Attachment #8902983 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902984 [details]
Bug 1394935: Assert that we don't call into NoteDirtyElement with extra bits on the restyle root's parent chain.

https://reviewboard.mozilla.org/r/174750/#review180066
Attachment #8902984 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902985 [details]
Bug 1394935: Fix a little typo in NoteDirtyElement.

https://reviewboard.mozilla.org/r/174752/#review180068

(I'm fine with bundling comment/typo fixes into other changes if that makes life easier for you)
Attachment #8902985 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902986 [details]
Bug 1394935: Introduce nsContentUtils::CommonFlattenedTreeAncestorForStyle.

https://reviewboard.mozilla.org/r/174754/#review180070
Attachment #8902986 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902987 [details]
Bug 1394935: Assert that if we find a common ancestor using the dirty bits, it is the actual common flattened tree ancestor.

https://reviewboard.mozilla.org/r/174756/#review180072

r=me with that fixed.

::: dom/base/Element.cpp:4393
(Diff revision 2)
> +      MOZ_ASSERT(commonAncestor == aElement ||
> +                 nsContentUtils::GetCommonFlattenedTreeAncestorForStyle(aElement, rootParent));

I think the right-hand side of the || isn't checking what you intend. Shouldn't it be:

MOZ_ASSERT(commonAncestor == aElement ||
           commonAncestor == nsContentUtils::...)?
Attachment #8902987 - Flags: review?(bobbyholley) → review+
Comment on attachment 8902917 [details]
Bug 1394935: Add a special case for marking something as dirty from invalidation code.

https://reviewboard.mozilla.org/r/174636/#review180108
Attachment #8902917 - Flags: review?(bobbyholley) → review+
Comment on attachment 8903256 [details]
Bug 1394935: Return the document as the flattened tree parent of doc-level NAC.

https://reviewboard.mozilla.org/r/175032/#review180110
Attachment #8903256 - Flags: review?(bobbyholley) → review+
Comment on attachment 8903257 [details]
Bug 1394935: Introduce ContentIsFlattenedTreeDescendantOfForStyle.

https://reviewboard.mozilla.org/r/175044/#review180112
Attachment #8903257 - Flags: review?(bobbyholley) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/23e8ef26bb40
Return the document as the flattened tree parent of doc-level NAC. r=bholley
https://hg.mozilla.org/integration/autoland/rev/d4830b06540f
Introduce ContentIsFlattenedTreeDescendantOfForStyle. r=bholley
https://hg.mozilla.org/integration/autoland/rev/f8c9e983f36c
Introduce nsContentUtils::CommonFlattenedTreeAncestorForStyle. r=bholley
https://hg.mozilla.org/integration/autoland/rev/88227ffe3c6e
Assert that the new root is always higher up in the tree than the old root. r=bholley
https://hg.mozilla.org/integration/autoland/rev/5ededad9baca
Add a (commented out for now) assertion about clobbering dirty bits. r=bholley
https://hg.mozilla.org/integration/autoland/rev/9fad64b68cbc
Assert that the content we're marking dirty is under the restyle root. r=bholley
https://hg.mozilla.org/integration/autoland/rev/089cbcabf67a
Assert that we don't call into NoteDirtyElement with extra bits on the restyle root's parent chain. r=bholley
https://hg.mozilla.org/integration/autoland/rev/3b1074717dac
Fix a little typo in NoteDirtyElement. r=bholley
https://hg.mozilla.org/integration/autoland/rev/c3f9a3e2ce96
Assert that if we find a common ancestor using the dirty bits, it is the actual common flattened tree ancestor. r=bholley
https://hg.mozilla.org/integration/autoland/rev/8daf204f77fb
Add a special case for marking something as dirty from invalidation code. r=bholley
Jesse posted a test case for this panic in bug 1389645.  I did confirm the test case does not crash on autoland locally. I think we can use the test case for an automation test case for this bug.  I did push a try with the test case based on autoland now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f74446fac18129907690ca5204aaca69975baa77
Attached file A crash test
The try looks good.
Attachment #8903383 - Flags: review?(bobbyholley)
Attachment #8903383 - Flags: review?(bobbyholley) → review+
Flags: needinfo?(emilio)
Depends on: 1400936
You need to log in before you can comment on or make changes to this bug.