1234701 - Various crashes with CSS "writing-mode: vertical-lr" and float

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: crash testcase

== Symptoms (debug) ==

Hangs for a few seconds, then asserts and crashes:

###!!! ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, placeholder)', file layout/generic/nsFrame.cpp, line 632

###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow', file layout/generic/nsPlaceholderFrame.h, line 160

Crash [@ nsLayoutUtils::GetFloatFromPlaceholder]


== Symptoms (ASan) ==

Crashes only when the page is closed:

[@ mozilla::layout::FrameChildListIterator::FrameChildListIterator]

Comment 1

[Jesse Ruderman]

Attachment: hang testcase

Comment 2

[Jesse Ruderman]

Stir DOM found this by adding a appendChild call to layout/reftests/css-grid/grid-min-max-content-sizing-001-ref.html, so gj Mats. (Reduced testcase does not use grid)

Comment 3

[Jesse Ruderman]

Since variants crash in different ways, I'd appreciate a quick fix.

Comment 4

[Mats Palmgren (inactive)]

I tested this on Linux64 using an ASAN Opt build and a DEBUG build.
The testcases works fine for me (in both e10s and non-e10s profiles),
with no assertions.
Jesse, can you still reproduce this?

Comment 5

[Jason Kratzer [:jkratzer]]

A variant of this issue was found while fuzzing mozilla-central rev 20170316-39607304b774.

ASAN:DEADLYSIGNAL
=================================================================
==20231==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f8637fa3172 bp 0x7ffc50454ed0 sp 0x7ffc50454ed0 T0)
==20231==The signal is caused by a READ memory access.
==20231==Hint: address points to the zero page.
    #0 0x7f8637fa3171 in mozilla::layout::FrameChildListIterator::FrameChildListIterator(nsIFrame const*) /home/worker/workspace/build/src/layout/generic/FrameChildList.cpp:17:11
    #1 0x7f8637ea82df in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:590:31
    #2 0x7f8637ea8481 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:602:7
    #3 0x7f8637ea8481 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:602:7
    #4 0x7f8637ea8481 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:602:7
    #5 0x7f8637ea8481 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:602:7
    #6 0x7f8637ea8481 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:602:7
    #7 0x7f8637ea8481 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:602:7
    #8 0x7f8637ea8481 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsFrameManager.cpp:602:7
    #9 0x7f8637df3a03 in mozilla::PresShell::CaptureHistoryState(nsILayoutHistoryState**) /home/worker/workspace/build/src/layout/base/PresShell.cpp:3868:22
    #10 0x7f863a255b45 in nsDocShell::PersistLayoutHistoryState() /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:12702:19
    #11 0x7f863a262c2b in nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7284:3
    #12 0x7f863a1f92b1 in nsDocShell::CreateContentViewer(nsACString const&, nsIRequest*, nsIStreamListener**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:9265:3
    #13 0x7f863a1f645b in nsDSURIContentListener::DoContent(nsACString const&, bool, nsIRequest*, nsIStreamListener**, bool*) /home/worker/workspace/build/src/docshell/base/nsDSURIContentListener.cpp:128:21
    #14 0x7f863316c436 in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:738:28
    #15 0x7f863316960c in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:416:30

Comment 6

[Jason Kratzer [:jkratzer]]

Attachment: New crashing testcase

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Neither of the Jesse's original two testcases crash/assert for me anymore either. They were fixed by something in the range below:
INFO: First good revision: 1dbe350b57b17ec1ce2887441b79c6f51b429378 (2016-02-05)
INFO: Last bad revision: 03297f8c28a08d2b39a252c7b368524d9e69da69 (2016-02-04)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=03297f8c28a08d2b39a252c7b368524d9e69da69&tochange=1dbe350b57b17ec1ce2887441b79c6f51b429378

However, but Jason's testcase still does crash as shown in bp-bec8db9b-88c5-4d15-a65e-c849b0171019. That was a more recent regression:
INFO: Last good revision: 05c3c56d71a7266d116afbe23fc1b3459217f2c4
INFO: First bad revision: f7b0f3b271228e2f33e0f6ce641060862012f07d
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=05c3c56d71a7266d116afbe23fc1b3459217f2c4&tochange=f7b0f3b271228e2f33e0f6ce641060862012f07d

Not sure which of jfkthame's patches is to blame there.

Part of me thinks we should file a new bug for Jason's testcase since it seems like separate issue from the on this bug was filed for. On the other hand, we could also just repurpose the bug and land Jesse's testcases as ride-alongs if/when this bug gets fixed.

Comment 8

[Jonathan Kew [:jfkthame]]

Looking at Jason's testcase, I don't think it really has any relation to the original issues here, nor to the title of the bug... in particular, there's no use of writing-mode in there AFAICT.

As such, I think we should move it to its own bug, and close this as WFM.

(FWIW, I'm also doubtful whether the regression range in comment 17 is the true cause here; I think it's more likely the changes there just altered behavior slightly such that Jason's testcase tickles a pre-existing bug, but a slightly different testcase would probably have crashed in a similar way prior to those patches.)

Comment 9

[Jonathan Kew [:jfkthame]]

And I guess we can go ahead and land Jesse's testcases here as crashtests, because More Tests = More Better.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Thanks for weighing in, Jonathan. NI myself on the follow-up actions.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I'm landing the two original testcases as crashtests and have filed bug 1411689 for Jason's newer one.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/956c004710ee
Add crashtests. r=me

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/956c004710ee