1319456 - [e10s] Crash in std::_Hash<T>::equal_range

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-8a500d05-e87e-4aad-a2d2-0c0592161122.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	std::_Hash<std::_Uset_traits<mozilla::gfx::DrawTargetD2D1*, std::_Uhash_compare<mozilla::gfx::DrawTargetD2D1*, std::hash<mozilla::gfx::DrawTargetD2D1*>, std::equal_to<mozilla::gfx::DrawTargetD2D1*> >, std::allocator<mozilla::gfx::DrawTargetD2D1*>, 0> >::equal_range(mozilla::gfx::DrawTargetD2D1* const&) 	vs2015u2/VC/include/xhash:641
1 	xul.dll 	mozilla::gfx::DrawEventRecorderPrivate::RemoveStoredObject(mozilla::gfx::ReferencePtr) 	gfx/2d/DrawEventRecorder.h:42
2 	xul.dll 	nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::ShiftData<nsTArrayInfallibleAllocator>(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int) 	obj-firefox/dist/include/nsTArray-inl.h:261
3 	xul.dll 	mozilla::gfx::PathRecording::`scalar deleting destructor'(unsigned int) 	
4 	xul.dll 	nsTArray_Impl<mozilla::dom::CanvasRenderingContext2D::ContextState, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) 	obj-firefox/dist/include/nsTArray.h:1905
5 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::`scalar deleting destructor'(unsigned int) 	
6 	xul.dll 	SnowWhiteKiller::~SnowWhiteKiller() 	xpcom/base/nsCycleCollector.cpp:2685
7 	xul.dll 	nsCycleCollector::FreeSnowWhite(bool) 	xpcom/base/nsCycleCollector.cpp:2859
8 	xul.dll 	nsCycleCollector_doDeferredDeletion() 	xpcom/base/nsCycleCollector.cpp:4134
9 	xul.dll 	AsyncFreeSnowWhite::Run() 	js/xpconnect/src/XPCJSRuntime.cpp:142
10 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1076
11 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:290
12 	xul.dll 	nsPrintingProxy::ShowPrintDialog(mozIDOMWindowProxy*, nsIWebBrowserPrint*, nsIPrintSettings*) 	embedding/components/printingui/ipc/nsPrintingProxy.cpp:113
13 	xul.dll 	nsPrintEngine::DoCommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*) 	layout/printing/nsPrintEngine.cpp:616
14 	xul.dll 	nsPrintEngine::CommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*) 	layout/printing/nsPrintEngine.cpp:405
15 	xul.dll 	nsPrintEngine::Print(nsIPrintSettings*, nsIWebProgressListener*) 	layout/printing/nsPrintEngine.cpp:788
16 	xul.dll 	nsDocumentViewer::Print(nsIPrintSettings*, nsIWebProgressListener*) 	layout/base/nsDocumentViewer.cpp:3727
17 	xul.dll 	nsGlobalWindow::PrintOuter(mozilla::ErrorResult&) 	dom/base/nsGlobalWindow.cpp:7388
18 	xul.dll 	nsGlobalWindow::Print(mozilla::ErrorResult&) 	dom/base/nsGlobalWindow.cpp:7414
19 	xul.dll 	mozilla::dom::WindowBinding::print 	obj-firefox/dom/bindings/WindowBinding.cpp:2527
20 	xul.dll 	mozilla::dom::WindowBinding::genericMethod 	obj-firefox/dom/bindings/WindowBinding.cpp:13778
21 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:453
22 		@0xfffffff5 	
23 		@0xfffffff5 	
24 		@0xfffffff5 	
25 	kernel32.dll 	InterlockedExchangeAdd 	
26 	xul.dll 	mozilla::detail::ThreadLocal<mozilla::dom::ScriptSettingsStackEntry*>::set(mozilla::dom::ScriptSettingsStackEntry* const) 	obj-firefox/dist/include/mozilla/ThreadLocal.h:203
27 	xul.dll 	nsWrapperCache::GetWrapper() 	dom/base/nsWrapperCacheInlines.h:19
28 	xul.dll 	mozilla::dom::ToJSValue<mozilla::dom::EventTarget>(JSContext*, mozilla::dom::EventTarget&, JS::MutableHandle<JS::Value>) 	obj-firefox/dist/include/mozilla/dom/ToJSValue.h:154
29 	xul.dll 	mozilla::dom::Event::GetEventPopupControlState(mozilla::WidgetEvent*, nsIDOMEvent*) 	dom/events/Event.cpp:712
30 		@0x2 	
31 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp:1566
32 	winmm.dll 	timeGetTime 	
33 	xul.dll 	mozilla::detail::RunnableMethodImpl<bool ( mozilla::ipc::MessageChannel::*)(void), 0, 1>::Run() 	obj-firefox/dist/include/nsThreadUtils.h:764
34 	xul.dll 	mozilla::ipc::MessageChannel::DequeueTask::Run() 	obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:569
35 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1076

crashes in the content process with this signature are spiking since firefox 50 builds on windows. currently they account for around 1.2% of all content process crashes in firefox 50.0.

Correlations for Firefox Release:
(100.0% in signature vs 36.07% overall) dom_ipc_enabled = 1
(100.0% in signature vs 37.04% overall) reason = EXCEPTION_ACCESS_VIOLATION_READ
(94.97% in signature vs 42.29% overall) "D2D1.1-" in app_notes = true

Comment 1

[Astley Chen (inactive)]

Bob, the crash seemed to be related to printing recording canvas, do you have any insight about the root cause or pointer to progress this bug ?

Comment 2

[Bob Owen (:bobowen) PTO until 8th Oct]

Attachment: Hold DrawEventRecorders in the correct structure in PathRecording

MozReview-Commit-ID: 1bJxJnKXzVc

Comment 3

[Astley Chen (inactive)]

Thanks for taking prompt feedback and support in this bug.

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording

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

Will this not cause a reference cycle?

Comment 5

[Bob Owen (:bobowen) PTO until 8th Oct]

(In reply to Bas Schouten (:bas.schouten) from comment #4)
> Comment on attachment 8813557 [details] [diff] [review]
> Hold DrawEventRecorders in the correct structure in PathRecording
> 
> Review of attachment 8813557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Will this not cause a reference cycle?

I don't think so.
The DrawEventRecorderPrivate only holds raw pointers in a map as a unique id for objects during playback. It relies on the Creation and Destruction events to keep track of them.

It doesn't hold a reference to anything else as far as I can see.
Also, the other *Recording types hold references to the recorder, so we'd presumably see cycles there if there was a problem.

Does this reasoning sound correct?

Comment 6

[Bob Owen (:bobowen) PTO until 8th Oct]

(In reply to Bob Owen (:bobowen) from comment #5)
> (In reply to Bas Schouten (:bas.schouten) from comment #4)

> I don't think so.
> The DrawEventRecorderPrivate only holds raw pointers in a map as a unique id
> for objects during playback. It relies on the Creation and Destruction
> events to keep track of them.

Erm, this should have said "as a unique ID to know what objects it has already recorded.".

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Given that this affects multiple branches, it'll need a sec rating of moderate or less or sec-approval before it can land.

Comment 8

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The issue is pretty obvious from this patch, because it changes from holding raw pointers to RefPtrs.
I'm not sure how easy this would be to exploit given that the potential use after free happens later on during GC or CC.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Patch is a one line change and the check-in comment if fairly vague.

Which older supported branches are affected by this flaw?
I believe that the code landing in 18, but it wasn't actually used in release until Fx50.

If not all supported branches, which bug introduced the flaw?
The code landed as part of bug 792207.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch applies cleanly to release (50).

How likely is this patch to cause regressions; how much testing does it need?
I think the only possible issue would be reference cycles, but I don't think that could happen. Waiting on Bas's opinion.

Comment 9

[Gerry Chang [:gchang]]

Track 51+ as crashes in 51 beta.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording

sec-approval+ for trunk. We should backport to Aurora and Beta, at least, as well so please nominate patches for those branches.

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Bob Owen (:bobowen) from comment #5)
> (In reply to Bas Schouten (:bas.schouten) from comment #4)
> > Comment on attachment 8813557 [details] [diff] [review]
> > Hold DrawEventRecorders in the correct structure in PathRecording
> > 
> > Review of attachment 8813557 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Will this not cause a reference cycle?
> 
> I don't think so.
> The DrawEventRecorderPrivate only holds raw pointers in a map as a unique id
> for objects during playback. It relies on the Creation and Destruction
> events to keep track of them.
> 
> It doesn't hold a reference to anything else as far as I can see.
> Also, the other *Recording types hold references to the recorder, so we'd
> presumably see cycles there if there was a problem.
> 
> Does this reasoning sound correct?

I think you're right, yes, thanks!

Comment 12

[Bob Owen (:bobowen) PTO until 8th Oct]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b57b890350b0e607b4d9b4ca266a761d20fae15e
Bug 1319456: Hold DrawEventRecorders in the correct structure in PathRecording. r=bas

Comment 13

[Bob Owen (:bobowen) PTO until 8th Oct]

Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording

Requesting for release as well, in case there is something else that drives a dot release.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 792207 originally, but not actually triggered until bug 1156742.

[User impact if declined]:
They will continue experiencing content process crashes after printing in some circumstances.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
No

[Needs manual test from QE? If yes, steps to reproduce]:
Yes - The crashes occur during GC or CC after printing in some circumstances, unfortunately we don't have steps to reproduce.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No 

[Why is the change risky/not risky?]:
Very simple change to hold RefPtrs instead of raw pointers. Only slight concern was that this could introduce a reference cycle, but we're confident that's not the case.

[String changes made/needed]:
None

Comment 14

[Bob Owen (:bobowen) PTO until 8th Oct]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b57b890350b0e607b4d9b4ca266a761d20fae15e

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b57b890350b0

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording

Fix a crash. Beta51+ and Aurora52+. Should be in 51 beta 7.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/11e76531aa74

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Kamil Jozwiak [:kjozwiak]]

This looks like it's going to be difficult to test without any STR/test cases. I went through the comments that have been submitted along side the crash reports, but I couldn't find any STR either. I went through some of the crash reports and attempted to reproduce the issue with some of the links that have been submitted alongside the crash reports but couldn't reproduce.

Looking at the crashes, I don't see this happening in any of the builds that have the fix from this patch. However, I didn't see any crashes occurring in anything other than early versions of 51beta and the current released versions [1] so I'm not sure that's the best indicator in telling us that this has been fixed.

If anyone has any idea's on how we can make sure this has been fixed, please let me know!

[1] https://crash-stats.mozilla.com/signature/?product=Firefox&signature=std%3A%3A_Hash%3CT%3E%3A%3Aequal_range#summary