Closed Bug 1403712 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure aRoot->IsInComposedDoc() (and subsequent crash) on Feedly

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
58 Branch
Platform:
x86_64
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: mozbugz, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(4 files)

debug build backtrace
7.54 KB, text/plain
Details
Bug 1403712: Make some assertions not assert for conditions that may happen mid-unbind.
59 bytes, text/x-review-board-request
bholley
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1403712: Clear the restyle root at the end of UnbindFromTree too.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1403712: Crashtest.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
https://crash-stats.mozilla.com/report/index/c2d32cbe-e5d8-4a2c-abcf-d8b530170927

STR:
1. Go to a Feedly feed, e.g. https://feedly.com/i/subscription/feed%2Fhttp%3A%2F%2Fplanet.mozilla.org%2Frss20.xml (you might need an account?)
2. Click on the hamburger-looking icon near the top-right, it should open a drop-down with some layout options (don't select any, leave it open!)
3. Click on the downward chevron that's just two icons to the left of the hamburger icon you just clicked.
Boom!

I can reproduce this crash consistently on Nightly 58.0a1 (2017-09-26), Mac OS X 10.12.6. Let me know if you want me to try something...
Attached file debug build backtrace — 
In a debug build from this morning, I get:

Assertion failure: aRoot == aRoot->OwnerDocAsNode() || (aRoot->IsElement() && aRoot->IsInComposedDoc()), at /files/mozilla/mc/w/dom/base/nsIDocumentInlines.h:92
This looks related to some recent stuff emilio's been looking at.
Flags: needinfo?(emilio)
Summary: stylo: Crash in mozalloc_abort | abort | geckoservo::glue::Servo_TraverseSubtree → stylo: Assertion failure aRoot->IsInComposedDoc() (and subsequent crash) on Feedly
Priority: -- → P2
Thanks for the great report Gerald!
Assignee: nobody → emilio
I got a fix, trying to reduce it to build a test-case now.
Flags: needinfo?(emilio)
Comment on attachment 8913183 [details]
Bug 1403712: Make some assertions not assert for conditions that may happen mid-unbind.

https://reviewboard.mozilla.org/r/184590/#review189856

::: dom/base/Element.cpp:4293
(Diff revision 1)
> +    // !parentNode can only happen mid-unbind.
>      MOZ_ASSERT_IF(!curr,
> +                  !parentNode ||

Nit: Please put this comment inline, like so:

MOZ_ASSERT_IF(!curr,
              !parentNode /* can happen mid-unbind */ ||
              parentNode == aElement->OwnerDoc() ||
              parentNode == parentNode->OwnerDoc()->GetRootElement());
Attachment #8913183 - Flags: review?(bobbyholley) → review+
Comment on attachment 8913185 [details]
Bug 1403712: Crashtest.

https://reviewboard.mozilla.org/r/184594/#review189860
Attachment #8913185 - Flags: review?(bobbyholley) → review+
Comment on attachment 8913184 [details]
Bug 1403712: Clear the restyle root at the end of UnbindFromTree too.

https://reviewboard.mozilla.org/r/184592/#review189866

::: dom/base/Element.cpp:2008
(Diff revision 1)
> +  if (document && document->GetServoRestyleRoot() == this) {
> +    document->ClearServoRestyleRoot();
> +  }

Add a comment that we could equivalently do this part in NoteDirtyElement?
Attachment #8913184 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> Comment on attachment 8913184 [details]
> Bug 1403712: Clear the restyle root at the end of UnbindFromTree too.
> 
> https://reviewboard.mozilla.org/r/184592/#review189866
> 
> ::: dom/base/Element.cpp:2008
> (Diff revision 1)
> > +  if (document && document->GetServoRestyleRoot() == this) {
> > +    document->ClearServoRestyleRoot();
> > +  }
> 
> Add a comment that we could equivalently do this part in NoteDirtyElement?

Per IRC, ignore this.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b350a3a643bb -d dccd87824efa: rebasing 423204:b350a3a643bb "Bug 1403712: Make some assertions not assert for conditions that may happen mid-unbind. r=bholley"
merging dom/base/Element.cpp
rebasing 423205:5be3fb724c05 "Bug 1403712: Clear the restyle root at the end of UnbindFromTree too. r=bholley"
merging dom/base/Element.cpp
rebasing 423206:05f091e926c6 "Bug 1403712: Crashtest. r=bholley" (tip)
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/56032de853c4
Make some assertions not assert for conditions that may happen mid-unbind. r=bholley
https://hg.mozilla.org/integration/autoland/rev/51d9d64884e2
Clear the restyle root at the end of UnbindFromTree too. r=bholley
https://hg.mozilla.org/integration/autoland/rev/8ef9a047e8c3
Crashtest. r=bholley
I guess we would like to uplift that to 57, right?
Flags: needinfo?(emilio)
Comment on attachment 8913183 [details]
Bug 1403712: Make some assertions not assert for conditions that may happen mid-unbind.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1400936 (in particular part 2, which removes a pretty big wallpaper)
[User impact if declined]: Crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: just some extra cleanup that we need to do while the tree is in an inconsistent state.
[String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8913183 - Flags: approval-mozilla-beta?
Comment on attachment 8913183 [details]
Bug 1403712: Make some assertions not assert for conditions that may happen mid-unbind.

Fix an assert, taking it.
Should be in 57b5
Attachment #8913183 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: