1315447 - Crash in mozilla::layers::ClientLayerManager::GetRemoteRenderer

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-6bbff2c3-cf58-4daa-85ac-7c0ae2161103.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::layers::ClientLayerManager::GetRemoteRenderer() 	gfx/layers/client/ClientLayerManager.cpp:393
1 	xul.dll 	mozilla::layers::ClientLayerManager::HandleMemoryPressure() 	gfx/layers/client/ClientLayerManager.cpp:748
2 	xul.dll 	mozilla::layers::ClientLayerManager::MemoryPressureObserver::Observe(nsISupports*, char const*, char16_t const*) 	gfx/layers/client/ClientLayerManager.cpp:62
3 	xul.dll 	nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverList.cpp:112
4 	xul.dll 	nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) 	xpcom/ds/nsObserverService.cpp:305
5 	xul.dll 	nsThread::DoMainThreadSpecificProcessing(bool) 	xpcom/threads/nsThread.cpp:1314
6 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1030
7 	xul.dll 	mozilla::dom::U2F::cycleCollection::ClassName() 	obj-firefox/dist/include/mozilla/dom/U2F.h:121
8 	xul.dll 	nsInProcessTabChildGlobal::FireUnloadEvent() 	dom/base/nsInProcessTabChildGlobal.cpp:210
9 	nss3.dll 	_MD_CURRENT_THREAD 	nsprpub/pr/src/md/windows/w95thred.c:317
10 		@0x500080a1

this signature is regressing since firefox 50 builds (though one single occurrence has been recorded for 46: https://crash-stats.mozilla.com/report/index/b36dc625-464f-4027-a49e-63f6c2160526 ).
overall it's rather low volume though with 0.06% of browser crashes on 50.0b last week.

marking the bug as security-sensitive as a precaution.

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

We reviewed this in platform triage meeting today. This doesn't seem release blocking. 

The crash volume about ~50 crashes a week, seems low and not release blocking. Let's keep it around in case we want to fix in 50.1.0.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Appears to be win32 only from the crash reports. I see that the crashing ClientLayerManager.cpp line in the second frame of the submitted reports was added in bug 1176011. Any chance it's related, George?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

UAF on ClientLayerManager?

Comment 4

[George Wright (:gw280) (needinfo me!)]

It does look like it's caused by my patch. Basically what's going on here is that mWidget on ClientLayerManager is getting deleted from somewhere else. ClientLayerManager only holds a raw pointer to mWidget and we never clear it, so there's an assumption that mWidget will always outlive the ClientLayerManager.

I'm not sure where mWidget is being deleted, but it's basically getting deleted elsewhere (probably related to this memory pressure event?) and we're trying to deref it in GetRemoteRenderer(). Before my patch, we never tried to call GetRemoteRenderer() in HandleMemoryPressure() so we kind of avoided the issue.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 6

[Marcia Knous [:marcia]]

Tracking 53+ for this sec-high bug.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Unless this signature spikes in the next few weeks, I see only ~5 instances of this on 50 in the past 24 hrs. It is too low volume a crash signature, wontfix for 50 now.

Comment 9

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1315447-Ensure-we-null-out-ClientLayerManager-s-.patch

Speculative fix. We already null out mWidget in BasicLayerManager on nsBaseWidget's destruction, so let's do the same for ClientLayerManager. I think this should fix it, but as I can't reproduce it this is speculative.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8813370 [details] [diff] [review]
0001-Bug-1315447-Ensure-we-null-out-ClientLayerManager-s-.patch

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

I think this is already fixed by bug 1319213.

Comment 11

[George Wright (:gw280) (needinfo me!)]

I agree. It looks functionally identical, as Destroy() is called in the same location. Will close this bug as a dupe of that other bug (should the other be marked sec?)

Comment 13

[Marco Castelluccio [:marco]]

Should we uplift to 51 and 52?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Dave, can you request Aurora/Beta approval on your patch in bug 1319213?

Comment 15

[Gerry Chang [:gchang]]

Track 51+ as sec-high and crash.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Done.

Comment 17

[[:philipp]]

hi, the uplift in bug 1315447 didn't work - could you try to rebase the patch there?
(the signature is accounting for 0.5% of browser crashes on 51.0b7 at the moment)

Comment 18

[[:philipp]]

sorry, comment #17 should have read: the uplift in bug 1319213 ...

Comment 19

[Julien Cristau [:jcristau]]

Maybe George could help with the rebase since he worked on a similar patch for this bug?

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

bug 1305829 has to be uplifted as well for bug 1319213 to be uplifted. But maybe we can just check mWidget and not DidComposite in ClientLayerManager's destructor? I can post a new patch if we don't want to uplift the other bug.

Comment 21

[[:philipp]]

hi liz, how would you feel about uplifting bug 1305829 to beta as well (see comment #20), in order to fix this regressing crash on 51?

on 51.0b10 the signature is currently the #11 browser crash accounting for 0.66% of all crashes of the browser process there...

Comment 22

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

dvander, please let us know what you would like to uplift. 
 
Andrew, do you have advice here or an opinion on the correct way to fix this? We will go to build for beta 11 next Monday.

Comment 23

[Andrew McCreight [:mccr8]]

I don't know, sorry.

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

I requested uplift on bug 1305829, since it's harmless and better to have the same code than write a new patch.

Comment 25

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

OK. Let's make sure to land the uplifts (from bug 1305829 and bug 1319213) for  beta 12.

Comment 26

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

Bug 1305829 landed on beta, does that mean the 51 status here should be marked "fixed"?

https://hg.mozilla.org/releases/mozilla-beta/rev/743472d4fc92212031be5faeea0ef4ea2c3a9c54

Comment 27

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

Looks like patches from both bugs landed on beta for 51 and this should be fixed in beta 12.
Let's check back next week to make sure the crash signature isn't showing up.

Comment 28

[Marco Castelluccio [:marco]]

No crashes on 51.0b12 or 52.0a2 since this landed.

Comment 29

[Petruta Horea [:phorea], Desktop QA]

We couldn't reproduce this on several OS's using different video players on facebook.com, youtube.com and other websites mentioned in comments.

Considering that there are no new reports in Socorro after the fixed landed, I'm marking this as verified and I'm removing the qe-verify+ flag.