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?
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.
[Tracking Requested - why for this release]:
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.
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
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.
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
Should we uplift to 51 and 52?
Dave, can you request Aurora/Beta approval on your patch in bug 1319213?
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)
Maybe George could help with the rebase since he worked on a similar patch for this bug?
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.
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...
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.
I don't know, sorry.
I requested uplift on bug 1305829, since it's harmless and better to have the same code than write a new patch.
OK. Let's make sure to land the uplifts (from bug 1305829 and bug 1319213) for beta 12.
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
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.
2 years ago
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.
You need to log in before you can comment on or make changes to this bug.