stylo: Crash in mozilla::ServoStyleSet::AppendSheetOfType

RESOLVED FIXED in Firefox 57

Status

()

P2
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: bradwerth)

Tracking

({crash, regression})

57 Branch
mozilla58
All
Windows
crash, regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 verified, firefox58 verified)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-d9d9e6a3-3f70-48b9-acd0-5ff540170927.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::ServoStyleSet::AppendSheetOfType(mozilla::SheetType, mozilla::ServoStyleSheet*) 	layout/style/ServoStyleSet.cpp:1466
1 	xul.dll 	mozilla::ServoStyleSet::AppendStyleSheet(mozilla::SheetType, mozilla::ServoStyleSheet*) 	layout/style/ServoStyleSet.cpp:715
2 	xul.dll 	nsDocumentViewer::CreateStyleSet(nsIDocument*) 	layout/base/nsDocumentViewer.cpp:2482
3 	xul.dll 	nsDocumentViewer::InitPresentationStuff(bool) 	layout/base/nsDocumentViewer.cpp:698
4 	xul.dll 	nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) 	layout/base/nsDocumentViewer.cpp:959
5 	xul.dll 	nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) 	layout/base/nsDocumentViewer.cpp:676
6 	xul.dll 	nsDocShell::SetupNewViewer(nsIContentViewer*) 	docshell/base/nsDocShell.cpp:9547
7 	xul.dll 	nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) 	docshell/base/nsDocShell.cpp:7366
8 	xul.dll 	nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIURI*, bool, bool) 	docshell/base/nsDocShell.cpp:8240
9 	xul.dll 	nsDocShell::EnsureContentViewer() 	docshell/base/nsDocShell.cpp:8098
10 	xul.dll 	nsDocShell::GetDocument() 	docshell/base/nsDocShell.cpp:4629
11 	xul.dll 	nsPIDOMWindow<mozIDOMWindowProxy>::MaybeCreateDoc() 	dom/base/nsGlobalWindow.cpp:4127
12 	xul.dll 	nsPIDOMWindow<mozIDOMWindowProxy>::GetDoc() 	dom/base/nsPIDOMWindow.h:213
13 	xul.dll 	nsGlobalWindow::WrapObject(JSContext*, JS::Handle<JSObject*>) 	dom/base/nsGlobalWindow.h:319
14 	xul.dll 	XPCConvert::NativeInterface2JSObject(JS::MutableHandle<JS::Value>, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, bool, nsresult*) 	js/xpconnect/src/XPCConvert.cpp:778
15 	xul.dll 	XPCConvert::NativeData2JS(JS::MutableHandle<JS::Value>, void const*, nsXPTType const&, nsID const*, nsresult*) 	js/xpconnect/src/XPCConvert.cpp:345
16 	xul.dll 	nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) 	js/xpconnect/src/XPCWrappedJSClass.cpp:1267
17 	xul.dll 	nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) 	js/xpconnect/src/XPCWrappedJS.cpp:615
18 	xul.dll 	PrepareAndDispatch 	xpcom/reflect/xptcall/md/win32/xptcstubs.cpp:85
19 	xul.dll 	SharedStub 	xpcom/reflect/xptcall/md/win32/xptcstubs.cpp:112
20 	xul.dll 	nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverList.cpp:112

this is showing up as a new signature in low volume after 57.0b was rolled-out to beta users in the last hours.
Brad: it appears we've got a null StyleSheet in the list here. Can you have a look and see how that can happen and how we should plug it? Thx!
Flags: needinfo?(bwerth)
(Assignee)

Comment 2

2 years ago
It's coming from the nsStyleSheetService singleton, from the AGENT_SHEET entry for the Servo backend. I'm still searching for what sets that entry to NULL.
(Assignee)

Comment 3

2 years ago
I've found at least one place where a NULL sheet could be put into that array: http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/layout/base/nsStyleSheetService.cpp#269. This will happen if nsLayoutUtils::StyloSupportedInCurrentProcess() returns false, which is dependent on process state in a way I don't understand well. I'll change the nsStyleSheetService to not push a NULL sheet in that case.
Flags: needinfo?(bwerth)
(Assignee)

Updated

2 years ago
Attachment #8913485 - Flags: review?(xidorn+moz)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8913485 [details]
Bug 1403694 Part 1: Prevent nsStyleSheetService from storing null servo stylesheets.

https://reviewboard.mozilla.org/r/184828/#review189978

::: layout/base/nsStyleSheetService.cpp:254
(Diff revision 1)
>    }
>  
>    nsresult rv;
>  
>    RefPtr<StyleSheet> geckoSheet;
>    RefPtr<StyleSheet> servoSheet;

In this case, I think you can move the variable `servoSheet` into the `if` block as well.

Comment 7

2 years ago
mozreview-review
Comment on attachment 8913485 [details]
Bug 1403694 Part 1: Prevent nsStyleSheetService from storing null servo stylesheets.

https://reviewboard.mozilla.org/r/184828/#review189982

I think this change itself is fine, but I suspect whether this would fix anything...

When `StyloSupportedInCurrentProcess()` returns false, `UpdateStyleBackendType` should never set the backend to Servo, so `nsStyleSheetService` shouldn't ever return array from `mServoSheets`. And given `StyloSupportedInCurrentProcess()` doesn't change its return value in the lifetime of the process (because it simply depends on the process type), this invariant should never change.

`nsIDocument::SetStyleBackendType` can be used to set the backend to something explicitly, but majority of its uses are just for inheriting backend from parent document, so they shouldn't be able to escape from the restriction above. There is one case, though, that the backend can be set to Servo regardless of the process restriction is `gfxSVGGlyphsDocument::ParseDocument` [1]. That can be a problem, but it doesn't seem to be related to this case anyway.

[1] https://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/gfx/thebes/gfxSVGGlyphs.cpp#365-376
Attachment #8913485 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> There is one case, though, that the backend can be set to
> Servo regardless of the process restriction is
> `gfxSVGGlyphsDocument::ParseDocument` [1]. That can be a problem, but it
> doesn't seem to be related to this case anyway.

Oh, wait, this is wrong. It checks `nsLayoutUtils::StyloEnabled()` which is the same check as `UpdateStyleBackendType`, so it is restricted by the process type as well.
That says, I have no idea how this crash can happen at all...
(Assignee)

Comment 10

2 years ago
Thanks for the more detailed analysis in comment 7. I don't know that we actively check the invariant that "backend == servo -> StyloSupportedInCurrentProcess() returns true". Since the patch corrects something that is arguably wrong (even if never called due to an invariant), I'm going to land it.
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f713977f1629
Fix a case where a NULL stylesheet would be added to the nsStyleSheetService if nsLayoutUtils::StyloSupportedInCurrentProcess() returns false. r=xidorn
Backed out

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f713977f16299641ecd8d01ae9393d7ba4541a9c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Push with more failures (ignore the xpcshell netwerk ones): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3c4afc844a6aacc68f1ee2c325143e2a7f569b17&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134078376&repo=autoland

[task 2017-09-29T15:33:51.372Z] 15:33:51     INFO -  87 INFO None88 INFO TEST-START | layout/style/test/test_additional_sheets.html
[task 2017-09-29T15:34:13.655Z] 15:34:13     INFO -  INFO | automation.py | Application ran for: 0:07:54.541903
[task 2017-09-29T15:34:13.656Z] 15:34:13     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmpO7hvojpidlog
[task 2017-09-29T15:34:14.297Z] 15:34:14     INFO -  /data/tombstones does not exist; tombstone check skipped
[task 2017-09-29T15:34:15.156Z] 15:34:15     INFO -  mozcrash Copy/paste: /builds/worker/workspace/build/linux64-minidump_stackwalk /tmp/tmpP5HY4m/3212b427-7ffa-556b-5a33-f26039b70428.dmp /builds/worker/workspace/build/symbols
[task 2017-09-29T15:34:23.231Z] 15:34:23     INFO -  mozcrash Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/3212b427-7ffa-556b-5a33-f26039b70428.dmp
[task 2017-09-29T15:34:23.231Z] 15:34:23     INFO -  mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/3212b427-7ffa-556b-5a33-f26039b70428.extra
[task 2017-09-29T15:34:23.235Z] 15:34:23  WARNING -  PROCESS-CRASH | layout/style/test/test_additional_sheets.html | application crashed [@ nsStyleSheetService::LoadAndRegisterSheet]
[task 2017-09-29T15:34:23.236Z] 15:34:23     INFO -  Crash dump filename: /tmp/tmpP5HY4m/3212b427-7ffa-556b-5a33-f26039b70428.dmp
[task 2017-09-29T15:34:23.236Z] 15:34:23     INFO -  Operating system: Android
[task 2017-09-29T15:34:23.242Z] 15:34:23     INFO -                    0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l
[task 2017-09-29T15:34:23.242Z] 15:34:23     INFO -  CPU: arm
[task 2017-09-29T15:34:23.242Z] 15:34:23     INFO -       ARMv7 ARM Cortex-A8 features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3
[task 2017-09-29T15:34:23.242Z] 15:34:23     INFO -       1 CPU
[task 2017-09-29T15:34:23.242Z] 15:34:23     INFO -  GPU: UNKNOWN
[task 2017-09-29T15:34:23.242Z] 15:34:23     INFO -  Crash reason:  SIGSEGV
[task 2017-09-29T15:34:23.243Z] 15:34:23     INFO -  Crash address: 0x0
[task 2017-09-29T15:34:23.243Z] 15:34:23     INFO -  Process uptime: not available
[task 2017-09-29T15:34:23.243Z] 15:34:23     INFO -  Thread 11 (crashed)
[task 2017-09-29T15:34:23.244Z] 15:34:23     INFO -   0  libxul.so!nsStyleSheetService::LoadAndRegisterSheet [nsStyleSheetService.cpp:f713977f1629 : 181 + 0x0]
[task 2017-09-29T15:34:23.244Z] 15:34:23     INFO -       r0 = 0x000000b6    r1 = 0x67db4702    r2 = 0x67db4702    r3 = 0x67db4702
[task 2017-09-29T15:34:23.245Z] 15:34:23     INFO -       r4 = 0x00000000    r5 = 0x5424e0a0    r6 = 0x00000000    r7 = 0x5299f448
[task 2017-09-29T15:34:23.245Z] 15:34:23     INFO -       r8 = 0x5556f460    r9 = 0x00000002   r10 = 0x00000000   r12 = 0x00000003
[task 2017-09-29T15:34:23.245Z] 15:34:23     INFO -       fp = 0x5299f748    sp = 0x5299f350    lr = 0x5bc05663    pc = 0x5bc0b156
[task 2017-09-29T15:34:23.245Z] 15:34:23     INFO -      Found by: given as instruction pointer in context
[task 2017-09-29T15:34:23.245Z] 15:34:23     INFO -   1  libxul.so!NS_InvokeByIndex [xptcinvoke_arm.cpp:f713977f1629 : 169 + 0x13]
[task 2017-09-29T15:34:23.246Z] 15:34:23     INFO -       r4 = 0x5299f440    r5 = 0x5bc0b04d    r6 = 0x00000003    r7 = 0x5299f448
[task 2017-09-29T15:34:23.246Z] 15:34:23     INFO -       r8 = 0x00000000    r9 = 0x00000002   r10 = 0x00000000    fp = 0x5299f748
[task 2017-09-29T15:34:23.246Z] 15:34:23     INFO -       sp = 0x5299f430    pc = 0x5a799267
[task 2017-09-29T15:34:23.246Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.246Z] 15:34:23     INFO -   2  libxul.so!CallMethodHelper::Call [XPCWrappedNative.cpp:f713977f1629 : 1996 + 0xd]
[task 2017-09-29T15:34:23.246Z] 15:34:23     INFO -       r3 = 0x5299f4c0    r4 = 0x5299f490    r5 = 0x00000002    r6 = 0x5299f468
[task 2017-09-29T15:34:23.246Z] 15:34:23     INFO -       r7 = 0x00000002    r8 = 0x00000000    r9 = 0x00000002   r10 = 0x00000000
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       fp = 0x5299f748    sp = 0x5299f460    pc = 0x5ac57a81
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -   3  libxul.so!XPCWrappedNative::CallMethod [XPCWrappedNative.cpp:f713977f1629 : 1282 + 0x5]
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       r4 = 0x5299f5b0    r5 = 0x00000000    r6 = 0x5299f584    r7 = 0x5299f4b8
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       r8 = 0x5299f4b0    r9 = 0x5299f588   r10 = 0x00000000    fp = 0x5299f748
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       sp = 0x5299f490    pc = 0x5ac57b6f
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -   4  libxul.so!XPC_WN_CallMethod [XPCWrappedNativeJSOps.cpp:f713977f1629 : 929 + 0x7]
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       r4 = 0x00000001    r5 = 0x5299f5b0    r6 = 0x5299f584    r7 = 0x5299f598
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       r8 = 0x5299f5a4    r9 = 0x5299f588   r10 = 0x00000000    fp = 0x5299f748
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       sp = 0x5299f568    pc = 0x5ac5c51b
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -   5  libxul.so!js::CallJSNative [jscntxtinlines.h:f713977f1629 : 293 + 0x3]
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       r4 = 0x5299f6f0    r5 = 0x52c2f800    r6 = 0x5299f750    r7 = 0x00000000
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       r8 = 0x00000001    r9 = 0x5ac5c419   r10 = 0x5820f000    fp = 0x5299f748
[task 2017-09-29T15:34:23.247Z] 15:34:23     INFO -       sp = 0x5299f628    pc = 0x5c51bfc1
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -   6  libxul.so!js::InternalCallOrConstruct [Interpreter.cpp:f713977f1629 : 495 + 0x7]
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -       r4 = 0x52c2f800    r5 = 0x5299f6f0    r6 = 0x00000000    r7 = 0x00000000
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -       r8 = 0x5299f66c    r9 = 0x52cd8000   r10 = 0x5299f700    fp = 0x00000000
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -       sp = 0x5299f660    pc = 0x5c536185
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -   7  libxul.so!js::Call [Interpreter.cpp:f713977f1629 : 559 + 0x3]
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -       r4 = 0x5299f6f0    r5 = 0x00000000    r6 = 0x6ac47200    r7 = 0xffffff8c
[task 2017-09-29T15:34:23.248Z] 15:34:23     INFO -       r8 = 0x5299f770    r9 = 0x5299f6e0   r10 = 0x5299f700    fp = 0x00000000
[task 2017-09-29T15:34:23.249Z] 15:34:23     INFO -       sp = 0x5299f6b8    pc = 0x5c536537
[task 2017-09-29T15:34:23.249Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.249Z] 15:34:23     INFO -   8  libxul.so!js::fun_apply [jsfun.cpp:f713977f1629 : 1302 + 0x3]
[task 2017-09-29T15:34:23.250Z] 15:34:23     INFO -       r3 = 0x5299f6f0    r4 = 0x5299f6e4    r5 = 0x5299f6f0    r6 = 0x00000001
[task 2017-09-29T15:34:23.250Z] 15:34:23     INFO -       r7 = 0x5299fd80    r8 = 0x5299f770    r9 = 0x5299f6e0   r10 = 0x5299f700
[task 2017-09-29T15:34:23.251Z] 15:34:23     INFO -       fp = 0x00000000    sp = 0x5299f6d0    pc = 0x5c8728fd
[task 2017-09-29T15:34:23.251Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.251Z] 15:34:23     INFO -   9  libxul.so!js::CallJSNative [jscntxtinlines.h:f713977f1629 : 293 + 0x3]
[task 2017-09-29T15:34:23.252Z] 15:34:23     INFO -       r4 = 0x5299fb14    r5 = 0x52c2f800    r6 = 0x5299fd98    r7 = 0x00000000
[task 2017-09-29T15:34:23.252Z] 15:34:23     INFO -       r8 = 0x00000001    r9 = 0x5c8726f9   r10 = 0x5820f000    fp = 0x5299fd90
[task 2017-09-29T15:34:23.253Z] 15:34:23     INFO -       sp = 0x5299f9f0    pc = 0x5c51bfc1
[task 2017-09-29T15:34:23.253Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.253Z] 15:34:23     INFO -  10  libxul.so!js::InternalCallOrConstruct [Interpreter.cpp:f713977f1629 : 495 + 0x7]
[task 2017-09-29T15:34:23.254Z] 15:34:23     INFO -       r4 = 0x52c2f800    r5 = 0x5299fb14    r6 = 0x00000000    r7 = 0x00000000
[task 2017-09-29T15:34:23.254Z] 15:34:23     INFO -       r8 = 0x5299fa34    r9 = 0x52cd8000   r10 = 0x00000000    fp = 0x5299fb50
[task 2017-09-29T15:34:23.254Z] 15:34:23     INFO -       sp = 0x5299fa28    pc = 0x5c536185
[task 2017-09-29T15:34:23.255Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.255Z] 15:34:23     INFO -  11  libxul.so!js::jit::DoCallFallback [BaselineIC.cpp:f713977f1629 : 2586 + 0x7]
[task 2017-09-29T15:34:23.255Z] 15:34:23     INFO -       r4 = 0x5299fb74    r5 = 0x52c2f800    r6 = 0x5299fb08    r7 = 0x5299faeb
[task 2017-09-29T15:34:23.256Z] 15:34:23     INFO -       r8 = 0x5299fdc0    r9 = 0x0000004f   r10 = 0x00000000    fp = 0x5299fb50
[task 2017-09-29T15:34:23.256Z] 15:34:23     INFO -       sp = 0x5299fa80    pc = 0x5c5df9e1
[task 2017-09-29T15:34:23.257Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.257Z] 15:34:23     INFO -  12  0x5e66a122
[task 2017-09-29T15:34:23.257Z] 15:34:23     INFO -       r4 = 0x5299fd48    r5 = 0x5299fd60    r6 = 0x5eb81bcc    r7 = 0xffffff8c
[task 2017-09-29T15:34:23.258Z] 15:34:23     INFO -       r8 = 0x00003043    r9 = 0x558f6400   r10 = 0x00000003    fp = 0x5299fd90
[task 2017-09-29T15:34:23.258Z] 15:34:23     INFO -       sp = 0x5299fd38    pc = 0x5e66a124
[task 2017-09-29T15:34:23.258Z] 15:34:23     INFO -      Found by: call frame info
[task 2017-09-29T15:34:23.259Z] 15:34:23     INFO -  13  libxul.so!nexttowardf + 0x177bb51
[task 2017-09-29T15:34:23.259Z] 15:34:23     INFO -       sp = 0x5299fd54    pc = 0x5e657aa0
[task 2017-09-29T15:34:23.259Z] 15:34:23     INFO -      Found by: stack scanning
Flags: needinfo?(bwerth)
Backed out for crashing in Android mochitests, e.g. layout/style/test/test_additional_sheets.html:
https://hg.mozilla.org/integration/autoland/rev/776ded750b88f711f99dc2a658bb8b85f53235ff
(Assignee)

Comment 15

2 years ago
Oh, I'm sorry I didn't test that more thoroughly. More is needed here. nsStyleSheetService::LoadAndRegisterSheet asserts that a successful return from the function changed in Part 1 means that two sheets were added (wrong).
(Assignee)

Updated

2 years ago
Assignee: nobody → bwerth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8913802 - Flags: review?(xidorn+moz)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8913802 - Attachment is obsolete: true
Attachment #8913802 - Flags: review?(xidorn+moz)
(Assignee)

Comment 20

2 years ago
Xidorn, I had to significantly rework the patch, and I wasn't able to clear your review of Part 1. Would you please re-review?
Flags: needinfo?(bwerth) → needinfo?(xidorn+moz)
Status: NEW → ASSIGNED
Priority: -- → P2
Summary: Crash in mozilla::ServoStyleSet::AppendSheetOfType → stylo: Crash in mozilla::ServoStyleSet::AppendSheetOfType

Comment 22

2 years ago
mozreview-review
Comment on attachment 8913485 [details]
Bug 1403694 Part 1: Prevent nsStyleSheetService from storing null servo stylesheets.

https://reviewboard.mozilla.org/r/184828/#review190818

r=me with the new patch

::: layout/base/nsStyleSheetService.cpp:189
(Diff revision 4)
>      nsTArray<nsCOMPtr<nsIPresShell>> toNotify(mPresShells);
>      for (nsIPresShell* presShell : toNotify) {
>        if (presShell->StyleSet()) {
> -        StyleSheet* sheet = presShell->StyleSet()->IsGecko() ? geckoSheet
> -                                                             : servoSheet;
> +        bool isGecko = presShell->StyleSet()->IsGecko();
> +        // We always have a Gecko sheet; we sometimes have a Servo sheet.
> +        if (isGecko || servoSheetWasAdded) {

I'm wondering whether this can just become an assertion, since on a process that stylo is not supported, no presshell with servo backend should be created... But it probably doesn't matter. Let's just leave the check here.

You can consider adding an `else` branch to `MOZ_ASSERT_UNREACHABLE`, though.
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request)

Comment 24

2 years ago
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6fd9d522e93
Part 1: Prevent nsStyleSheetService from storing null servo stylesheets. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/e6fd9d522e93
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: ? → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(FWIW, as I mentioned in comment 7, I don't really believe this would fix anything, but we can see. If the crash doesn't stop, we can try to figure out the reason in a new bug, I guess.)
All the crash reports we've gotten for this signature are on Beta, so should we consider uplifting this there to see if it makes a difference?
Flags: needinfo?(bwerth)
(Assignee)

Comment 28

2 years ago
I think it should be uplifted, but I don't recall which flags to set and to what values. I do have the details:

[Feature/regressing bug #]: Fixes a gap in the implementation of nsStyleService, when it was extended to support Servo sheets.
[User impact if declined]: We have no Steps To Reproduce, but the crash signature shows that this could crash from a NULL dereference in release under some unusual conditions.
[Describe test coverage new/current, TBPL]: No new tests were generated. The fix plugs a theoretical problem that is appearing in crashes in the wild.
[Risks and why]: Low. There's no code that should be relying on the previous behavior, because it would consistently lead to a crash.
[String/UUID change made/needed]: none
Flags: needinfo?(bwerth)
Attachment #8913485 - Flags: approval-mozilla-beta?
Comment on attachment 8913485 [details]
Bug 1403694 Part 1: Prevent nsStyleSheetService from storing null servo stylesheets.

Stylo related crash fix, let's hope this helps, Beta57+

Philipp FYI if you see this signature go away, please mark this as verified on 57. Thanks!
Flags: needinfo?(madperson)
Attachment #8913485 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 30

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8c7645234f86
status-firefox57: affected → fixed
(Reporter)

Comment 31

2 years ago
looks fixed with b6 (no more reports via crash stats during the last days).
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: needinfo?(madperson)
You need to log in before you can comment on or make changes to this bug.