1293985 - (CVE-2016-9905) Crash in FlushLayoutRecursive

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-4329e93a-6811-4fc5-ab7b-1ca962160810.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 		@0x5 	
1 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
2 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
3 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
4 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
5 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
6 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
7 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
8 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
9 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
10 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
11 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
12 	xul.dll 	nsDocument::EnumerateSubDocuments(bool (*)(nsIDocument*, void*), void*) 	dom/base/nsDocument.cpp:8845
13 	xul.dll 	FlushLayoutRecursive(nsIDocument*, void*) 	layout/base/nsPresShell.cpp:3653
14 	xul.dll 	PresShell::DispatchSynthMouseMove(mozilla::WidgetGUIEvent*, bool) 	layout/base/nsPresShell.cpp:3685
15 	xul.dll 	PresShell::ProcessSynthMouseMoveEvent(bool) 	layout/base/nsPresShell.cpp:5627
16 	xul.dll 	nsCOMPtr_base::~nsCOMPtr_base() 	obj-firefox/dist/include/nsCOMPtr.h:295
17 	xul.dll 	PresShell::nsSynthMouseMoveEvent::WillRefresh(mozilla::TimeStamp) 	layout/base/nsPresShell.h:667
18 	xul.dll 	mozilla::IMEStateManager::SetInputContext(nsIWidget*, mozilla::widget::InputContext const&, mozilla::widget::InputContextAction const&) 	dom/events/IMEStateManager.cpp:1085
19 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:508

this crash signature is regressing since firefox 49 and seems to occur in the code of the changes from bug 881832.
it's currently the #35 crasher on 449.0b1 making up 0.27% of all crashes.

Comment 1

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

Matt or Tim, can you take a look at this new crash in beta? Thanks!

Comment 2

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

I took a look at bp-9fc65d48-5d23-4c6b-a890-a24b12160812, and it looks like FlushLayoutRecursive is being called with aDocument == 0xE5E5E5E5E5E5E5E5.  That one was amd64, where the "Crash Address" field tends to be less reliable.  On x86 crashes, most crashes appear to have a crash address of 0xe5e5e5e5.

That's apparently how jemalloc marks freed memory.

Comment 3

[Mats Palmgren (inactive)]

> aDocument == 0xE5E5E5E5E5E5E5E5

That implies a deallocated shell, which is a bit strange since the only caller
of DispatchSynthMouseMove holds a strong ref on that shell on the stack:
https://hg.mozilla.org/mozilla-unified/annotate/6cf0089510fa/layout/base/nsPresShell.cpp#l5858
and DispatchSynthMouseMove calls FlushLayoutRecursive with mDocument
(line 3718) which is a nsCOMPtr member on that shell.
https://hg.mozilla.org/mozilla-unified/annotate/6cf0089510fa/layout/base/nsPresShell.cpp#l3687

It seems possible that aDocument might not be alive after line 3691 though,
in case mDocument changed as a result of line 3691, but then it seems like
it should indicate line 3692 as the crash site.

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix?

It seems like we should fix this anyway.
Let's land it and see if the crashes go away.

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8781751 [details] [diff] [review]
fix?

If the list of subdocuments that EnumerateSubDocuments is iterating over changes while iterating over it (because we flush in a callback that causes it to change) then we'd also have a problem, no?

Comment 6

[Matt Woodrow (:mattwoodrow)]

I had a look at the minidump, it's aDocument itself that is 0xe5e5e5e5, not the contents of the document.

That would suggest that the SubDocMapEntry (referenced in EnumerateSubDocuments) that has been deleted and poisoned.

Comment 7

[Mats Palmgren (inactive)]

Yeah, that seems more likely.  I assumed we use something there that's resilient to
entries being added/removed but we're not - we're using PLDHashTable::Iterator.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.

Comment 9

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73160480c6d1&selectedJob=25880129

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd6f2742f7b
part 1 - Hold a strong ref on aDocument.  r=tn
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e93a5912b99
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.  r=tn

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0bd6f2742f7b
https://hg.mozilla.org/mozilla-central/rev/0e93a5912b99

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.

Approval Request Comment
[Feature/regressing bug #]: bug 881832
[User impact if declined]: crash, see comment 2
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 13

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

Comment on attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.

Fix for a new crash in beta, please uplift. This should make it into beta 6 or 7.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba8075cd3fb9
https://hg.mozilla.org/releases/mozilla-aurora/rev/44330fe94f7b

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7f2fa6e2e0bb
https://hg.mozilla.org/releases/mozilla-beta/rev/e454d5a9aa40

Comment 16

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

Mats, turns out we need this for 45.5.0esr. Can you request esr45 uplift?  Does this need any sort of rebasing for esr? Thanks.

Comment 17

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

Comment on attachment 8782077 [details] [diff] [review]
part 2 - Make EnumerateSubDocuments resilient to adding/removing sub-documents.

Actually, let's just go ahead and try landing this on esr45. Dan and I discussed it and we have some time to respond if there are issues.

Comment 18

[Mats Palmgren (inactive)]

I think both patches should apply as is, let me know there are problems.

Comment 19

[Carsten Book [:Tomcat]]

has problem to apply to esr45

grafting 363087:7f2fa6e2e0bb "Bug 1293985 part 1 - Hold a strong ref on aDocument.  r=tn a=lizzard"
merging layout/base/nsPresShell.cpp
warning: conflicts while merging layout/base/nsPresShell.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue'

Comment 20

[Mats Palmgren (inactive)]

I can't produce patches for esr45 because bug 881832 etc needs to land on esr45 first.
I suspect that this will apply without conflicts once they have.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/aebd3687e05e
https://hg.mozilla.org/releases/mozilla-esr45/rev/63d8e5cd27cb