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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the easiest fix here is to add to mRoots early in nsStyleContext::nsStyleContext rather than waiting for nsStyleSet::GetContext to do it.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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/

Comment 3

[Cameron McCormack (:heycam)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eeebc83a2b986fbf072e86ba4e8fc8d003b6c45
Bug 1256500 - Root style contexts before calling ApplyStyleFixups.  r=heycam

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1eeebc83a2b9

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b157d05eb98e

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ef62e642b5d1

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The beta backport needed a followup:
https://hg.mozilla.org/releases/mozilla-beta/rev/e6d9411d4d0c

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.