crash in [@ nsStyleContext::StyleDisplay] creating ::viewport style context during printing, due to rule tree GC rooting failure

RESOLVED FIXED in Firefox 46

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({crash, topcrash})

Trunk
mozilla48
Unspecified
Windows NT
crash, topcrash
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed, firefox47 fixed, firefox48 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-9d5fb770-f029-4c61-896a-35b642160314.
=============================================================

There are a decent number of crashes of this form during printing:

0 	xul.dll 	nsStyleContext::StyleDisplay() 	obj-firefox/dist/include/nsStyleStructList.h
1 	xul.dll 	nsStyleContext::ApplyStyleFixups(bool) 	layout/style/nsStyleContext.cpp
2 	xul.dll 	NS_NewStyleContext(nsStyleContext*, nsIAtom*, mozilla::CSSPseudoElementType, nsRuleNode*, bool) 	layout/style/nsStyleContext.cpp
3 	xul.dll 	nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) 	layout/style/nsStyleSet.cpp
4 	xul.dll 	nsStyleSet::ResolveAnonymousBoxStyle(nsIAtom*, nsStyleContext*, unsigned int) 	layout/style/nsStyleSet.cpp
5 	xul.dll 	nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) 	layout/base/nsCSSFrameConstructor.cpp
6 	xul.dll 	nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) 	layout/base/nsCSSFrameConstructor.cpp
7 	xul.dll 	nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool, nsCSSFrameConstructor::RemoveFlags, nsIContent**) 	layout/base/nsCSSFrameConstructor.cpp
8 	xul.dll 	nsCSSFrameConstructor::ReconstructDocElementHierarchy() 	layout/base/nsCSSFrameConstructor.cpp
9 	xul.dll 	PresShell::ReconstructFrames() 	layout/base/nsPresShell.cpp
10 	xul.dll 	nsPrintEngine::ReconstructAndReflow(bool) 	layout/printing/nsPrintEngine.cpp
11 	xul.dll 	nsPrintEngine::SetupToPrintContent() 	layout/printing/nsPrintEngine.cpp
12 	xul.dll 	nsPrintEngine::DocumentReadyForPrinting() 	layout/printing/nsPrintEngine.cpp
13 	xul.dll 	nsPrintEngine::FinishPrintPreview() 	layout/printing/nsPrintEngine.cpp
14 	xul.dll 	nsPrintEngine::AfterNetworkPrint(bool) 	layout/printing/nsPrintEngine.cpp
15 	xul.dll 	nsPrintEngine::InitPrintDocConstruction(bool) 	layout/printing/nsPrintEngine.cpp
16 	xul.dll 	nsPrintEngine::DoCommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*) 	layout/printing/nsPrintEngine.cpp
17 	xul.dll 	nsPrintEngine::CommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*) 	layout/printing/nsPrintEngine.cpp
18 	xul.dll 	nsPrintEngine::PrintPreview(nsIPrintSettings*, mozIDOMWindowProxy*, nsIWebProgressListener*) 	layout/printing/nsPrintEngine.cpp
19 	xul.dll 	nsDocumentViewer::PrintPreview(nsIPrintSettings*, mozIDOMWindowProxy*, nsIWebProgressListener*) 	layout/base/nsDocumentViewer.cpp
20 	xul.dll 	XPTC__InvokebyIndex 	xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm


I debugged the minidump for this crash, and the crash seems to be happening because the rule node for the style context that we're creating has the frame poison pattern.  In other words, we're dealing with a style context with a destroyed rule node.

There's a decent explanation for how this can happen, too.

In this case, we're creating the style context for the viewport:
https://hg.mozilla.org/mozilla-central/annotate/d1d47ba19ce9/layout/style/nsStyleContext.cpp#l604
In this chunk of code, we enter the if() and we create and destroy a style context.  When the RefPtr goes out of scope, we call nsStyleContext::Release -> nsStyleContext::~nsStyleContext -> nsStyleSet::NotifyStyleContextDestroyed.  1/300 of the time, this then triggers a rule tree GC.

Since NS_NewStyleContext hasn't yet returned to nsStyleSet::GetContext (which is responsible for adding the new style context to mRoots if it has a null parent, which is the case here since the caller is https://hg.mozilla.org/mozilla-central/annotate/d1d47ba19ce9/layout/base/nsCSSFrameConstructor.cpp#l2341 ), we haven't added the new style context to mRoots.  This means if we trigger a rule tree GC on line 618 (end of the RefPtr's scope), we can crash when we call StyleDisplay() on line 622.

I believe that's what's happening in this case, although I haven't tested.  I suspect it's probably relatively reproducable by changing kGCInterval to 1.

This is a regression from https://hg.mozilla.org/mozilla-central/rev/5d4b602cf88f .

I believe it's not a security risk thanks to frame poisoning.
(Assignee)

Comment 1

2 years ago
I think the easiest fix here is to add to mRoots early in nsStyleContext::nsStyleContext rather than waiting for nsStyleSet::GetContext to do it.
(Assignee)

Updated

2 years ago
Has Regression Range: --- → yes
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
(Assignee)

Updated

2 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8730486 [details]
MozReview Request: Bug 1256500 - Root style contexts before calling ApplyStyleFixups.  r?heycam

Review commit: https://reviewboard.mozilla.org/r/39931/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39931/
Attachment #8730486 - Flags: review?(cam)
Comment on attachment 8730486 [details]
MozReview Request: Bug 1256500 - Root style contexts before calling ApplyStyleFixups.  r?heycam

https://reviewboard.mozilla.org/r/39931/#review36495

::: layout/style/nsStyleContext.cpp:129
(Diff revision 1)
> +  if (!mParent) {
> +    // Add as a root before ApplyStyleFixups, since ApplyStyleFixups
> +    // can trigger rule tree GC.
> +    nsStyleSet* styleSet =
> +      mRuleNode->PresContext()->PresShell()->StyleSet()->GetAsGecko();
> +    styleSet->AddStyleContextRoot(this);

Wrap the AddStyleContextRoot call in an |if (styleSet)|, since null will be returned when we're using a ServoStyleSet.
Attachment #8730486 - Flags: review?(cam) → review+

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1eeebc83a2b9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 6

2 years ago
Comment on attachment 8730486 [details]
MozReview Request: Bug 1256500 - Root style contexts before calling ApplyStyleFixups.  r?heycam

Approval Request Comment
[Feature/regressing bug #]: bug 817406 (Firefox 41)
[User impact if declined]: occasional crashes, perhaps only when printing, or maybe in other cases
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low-risk crash fix for crash I found looking at topcrashes in crash-stats
[String/UUID change made/needed]: no
Attachment #8730486 - Flags: approval-mozilla-beta?
Attachment #8730486 - Flags: approval-mozilla-aurora?
Comment on attachment 8730486 [details]
MozReview Request: Bug 1256500 - Root style contexts before calling ApplyStyleFixups.  r?heycam

Fix for a topcrash, taking it for aurora and beta. 
Let's see if we can get this into beta 2.
Attachment #8730486 - Flags: approval-mozilla-beta?
Attachment #8730486 - Flags: approval-mozilla-beta+
Attachment #8730486 - Flags: approval-mozilla-aurora?
Attachment #8730486 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 11

2 years ago
I looked through some of the crashes with this signature in 46.0b9, and couldn't find any with the stack in comment 0 that goes through printing, which suggests that this particular problem is fixed, although I don't actually know how to query on things lower down on the stack to know for sure.
You need to log in before you can comment on or make changes to this bug.