Closed Bug 1416544 Opened 2 years ago Closed 2 years ago

Crash in nsPlaceholderFrame::RenumberFrameAndDescendants

Categories

(Core :: Layout: Floats, defect, critical)

58 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: bc, Assigned: mats)

References

(Blocks 2 open bugs, )

Details

(Keywords: crash, csectype-nullptr, testcase)

Crash Data

Attachments

(3 files)

1. http://www.mellowship.nu/menu
2. Crash Beta/57, Nightly/58
   This and related crashes involving  nsFontFaceUtils::MarkDirtyForFontChange occur on Windows and Linux.

This bug was filed from the Socorro interface and is
report bp-7fbf4d13-ebe2-4b13-812a-02b6e0171112.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsPlaceholderFrame::RenumberFrameAndDescendants layout/generic/nsPlaceholderFrame.h:159
1 xul.dll nsBlockFrame::RenumberChildFrames layout/generic/nsBlockFrame.cpp:7167
2 xul.dll nsContainerFrame::RenumberFrameAndDescendants layout/generic/nsContainerFrame.cpp:1921
3 xul.dll nsBlockFrame::RenumberChildFrames layout/generic/nsBlockFrame.cpp:7167
4 xul.dll nsContainerFrame::RenumberFrameAndDescendants layout/generic/nsContainerFrame.cpp:1906
5 xul.dll nsBlockFrame::RenumberChildFrames layout/generic/nsBlockFrame.cpp:7167
6 xul.dll nsContainerFrame::RenumberList layout/generic/nsContainerFrame.cpp:1829
7 xul.dll nsBlockFrame::Reflow layout/generic/nsBlockFrame.cpp:1191
8 xul.dll nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:934
9 xul.dll nsColumnSetFrame::ReflowChildren layout/generic/nsColumnSetFrame.cpp:810

=============================================================

Nightly/58 on Linux produced bp-9aec4739-87d9-4bf1-87b5-dcd750171112
[@ nsFontFaceUtils::MarkDirtyForFontChange ]

0 	libxul.so 	nsFontFaceUtils::MarkDirtyForFontChange 	mfbt/RefPtr.h:287
1 	libxul.so 	nsFontFaceLoader::OnStreamComplete 	layout/style/nsFontFaceLoader.cpp:283
2 	libxul.so 	mozilla::net::nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	
3 	libxul.so 	nsCORSListenerProxy::OnStopRequest(nsIRequest*, nsISupports*, nsresult) 	
4 	libxul.so 	mozilla::net::HttpChannelChild::DoOnStopRequest 	netwerk/protocol/http/HttpChannelChild.cpp:1206
5 	libxul.so 	mozilla::net::HttpChannelChild::OnStopRequest 	netwerk/protocol/http/HttpChannelChild.cpp:1099
6 	libxul.so 	mozilla::net::ChannelEventQueue::FlushQueue 	netwerk/ipc/ChannelEventQueue.cpp:93
7 	libxul.so 	mozilla::net::ChannelEventQueue::ResumeInternal()::CompleteResumeRunnable::Run() 	
8 	libxul.so 	mozilla::SchedulerGroup::Runnable::Run 	xpcom/threads/SchedulerGroup.cpp:396
9 	libxul.so 	nsThread::ProcessNextEvent(bool, bool*) 	
10 	libxul.so 	NS_ProcessNextEvent(nsIThread*, bool)

  
debug asan gives:
=================================================================
==10898==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x7f67e8aba86c bp 0x7ffdd43c4d90 sp 0x7ffdd43c4a60 T0)
==10898==The signal is caused by a READ memory access.
==10898==Hint: address points to the zero page.
    #0 0x7f67e8aba86b in get /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:287:27
    #1 0x7f67e8aba86b in operator nsStyleContext * /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:300
    #2 0x7f67e8aba86b in StyleContext /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:786
    #3 0x7f67e8aba86b in PresContext /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:641
    #4 0x7f67e8aba86b in FrameUsesFont /builds/worker/workspace/build/src/layout/style/nsFontFaceUtils.cpp:52
    #5 0x7f67e8aba86b in nsFontFaceUtils::MarkDirtyForFontChange(nsIFrame*, gfxUserFontEntry const*) /builds/worker/workspace/build/src/layout/style/nsFontFaceUtils.cpp:131
    #6 0x7f67e8a0e991 in nsFontFaceLoader::OnStreamComplete(nsIStreamLoader*, nsISupports*, nsresult, unsigned int, unsigned char const*) /builds/worker/workspace/build/src/layout/style/nsFontFaceLoader.cpp:283:14
    #7 0x7f67e1f794fd in mozilla::net::nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/base/nsStreamLoader.cpp:108:30
    #8 0x7f67e274051c in nsCORSListenerProxy::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /builds/worker/workspace/build/src/netwerk/protocol/http/nsCORSListenerProxy.cpp:638:27
    #9 0x7f67e26d3bbc in mozilla::net::HttpChannelChild::DoOnStopRequest(nsIRequest*, nsresult, nsISupports*) /builds/worker/workspace/build/src/netwerk/protocol/http/HttpChannelChild.cpp:1206:16
    #10 0x7f67e26dff3a in mozilla::net::HttpChannelChild::OnStopRequest(nsresult const&, mozilla::net::ResourceTimingStruct const&) /builds/worker/workspace/build/src/netwerk/protocol/http/HttpChannelChild.cpp:1099:5
Attached file Testcase #1
Component: Layout → Layout: Floats
Keywords: testcase
OS: Windows 7 → All
Hardware: Unspecified → All
Attached file frame dump
When we first reflow the "Block(div)(1)@7ff86927f018" (lime) it pushes
a float (red) and the frame that follows (blue).  The problem occurs
on the next reflow we pull up the in-flow frame (blue) and signal
that the reflow is complete.  When deleting the continuations we
also delete the pushed float, which triggers various assertions:

###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file layout/base/nsLayoutUtils.cpp, line 7840
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file layout/base/nsLayoutUtils.cpp, line 7840
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 7854
###!!! 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 760
###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow', file layout/generic/nsPlaceholderFrame.h, line 183
Assignee: nobody → mats
It appears to always be a null-pointer crash for me.
I'm guessing the worst case is we try to use the deleted float frame,
which is covered by frame-poisoning so it should be non-exploitable.
This patch amends an existing workaround, but as the NOTE there says,
we should have pulled up those floats and reflowed them somewhere
(and pushed them again potentially, and then we wouldn't be COMPLETE).
It's unclear to me where that pull-up is supposed to happen though.
Attachment #8927995 - Flags: review?(dholbert)
Comment on attachment 8927995 [details] [diff] [review]
Don't report COMPLETE reflow status when we still have pushed floats on some next-in-flow frame

Review of attachment 8927995 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8927995 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff1b6740768
Don't report COMPLETE reflow status when we still have pushed floats on some next-in-flow frame.  r=dholbert
https://hg.mozilla.org/mozilla-central/rev/9ff1b6740768
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1418543
Hi :mats,
Since this bug also affects 58, do you think it's worth uplifting to 58 if this patch is not too risky?
Flags: needinfo?(mats)
Sure, it's a very low risk wallpaper.
Flags: needinfo?(mats)
Comment on attachment 8927995 [details] [diff] [review]
Don't report COMPLETE reflow status when we still have pushed floats on some next-in-flow frame

Approval Request Comment
[Feature/Bug causing the regression]:no
[User impact if declined]:crash in rare multicolumn layouts containing floats
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]:simple change to amend an already present wallpaper with one more case
[String changes made/needed]:none
Attachment #8927995 - Flags: approval-mozilla-beta?
Comment on attachment 8927995 [details] [diff] [review]
Don't report COMPLETE reflow status when we still have pushed floats on some next-in-flow frame

Fix a crash. Beta58+.
Attachment #8927995 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.