Closed Bug 1256500 Opened 4 years ago Closed 4 years ago

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

Categories

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

Unspecified
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- affected
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

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.
I think the easiest fix here is to add to mRoots early in nsStyleContext::nsStyleContext rather than waiting for nsStyleSet::GetContext to do it.
Has Regression Range: --- → yes
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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+
https://hg.mozilla.org/mozilla-central/rev/1eeebc83a2b9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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+
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.