Crash in mozilla::layers::ClientLayerManager::GetRemoteRenderer

VERIFIED FIXED in Firefox 51

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: philipp, Assigned: dvander)

Tracking

(4 keywords)

50 Branch
mozilla53
x86
Windows
Points:
---

Firefox Tracking Flags

(firefox49 unaffected, firefox-esr45 unaffected, firefox50- wontfix, firefox51+ verified, firefox52+ verified, firefox53+ verified)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
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.
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?
Flags: needinfo?(gwright)
UAF on ClientLayerManager?
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.
Flags: needinfo?(gwright)
Group: core-security → gfx-core-security
[Tracking Requested - why for this release]:
Tracking 53+ for this sec-high bug.
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.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1318894
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.
Assignee: nobody → gwright
Attachment #8813370 - Flags: review?(matt.woodrow)
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.
Attachment #8813370 - Flags: review?(matt.woodrow)
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?)
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1319213
Resolution: DUPLICATE → FIXED
Should we uplift to 51 and 52?
Flags: needinfo?(gwright)
Group: gfx-core-security → core-security-release
Dave, can you request Aurora/Beta approval on your patch in bug 1319213?
Assignee: gwright → dvander
Depends on: 1319213
Flags: needinfo?(gwright) → needinfo?(dvander)
Target Milestone: --- → mozilla53
Track 51+ as sec-high and crash.
Done.
Flags: needinfo?(dvander)
(Reporter)

Comment 17

3 years ago
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)
Flags: needinfo?(dvander)
(Reporter)

Comment 18

2 years ago
sorry, comment #17 should have read: the uplift in bug 1319213 ...
Maybe George could help with the rebase since he worked on a similar patch for this bug?
Flags: needinfo?(gwright)
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.
Flags: needinfo?(dvander)
(Reporter)

Comment 21

2 years ago
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...
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(dvander)
Flags: needinfo?(continuation)
I don't know, sorry.
Flags: needinfo?(continuation)
I requested uplift on bug 1305829, since it's harmless and better to have the same code than write a new patch.
Flags: needinfo?(dvander)
OK. Let's make sure to land the uplifts (from bug 1305829 and bug 1319213) for  beta 12.
Flags: needinfo?(lhenry)
Flags: needinfo?(gwright)
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
Flags: needinfo?(dvander)
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.
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Flags: needinfo?(dvander)
No crashes on 51.0b12 or 52.0a2 since this landed.
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.