528025 - Invalid read happen in view because of Device context is not referred properly

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Oleg Romashin (:romaxa)]

mContext in nsViewManager and nsPresContext are the same pointers, but it is not referred properly when DocumentViewer creating nsIDeviceContext.

And when nsPresContext destructor releasing and destroying mDevicecontext, mContext in nsViewManager getting invalid pointer.


==30066== Invalid read of size 4
==30066==    at 0x4F15BD8: nsViewManager::ViewToWidget(nsView*, nsView*, nsRect const&) const (nsIDeviceContext.h:284)
==30066==    by 0x4F16A19: nsViewManager::UpdateWidgetArea(nsView*, nsIWidget*, nsRegion const&, nsView*) (nsViewManager.cpp:813)
==30066==    by 0x4F16E67: nsViewManager::UpdateView(nsIView*, nsRect const&, unsigned int) (nsViewManager.cpp:844)
==30066==    by 0x4F13769: nsViewManager::UpdateView(nsIView*, unsigned int) (nsViewManager.cpp:602)
==30066==    by 0x4F1430F: nsViewManager::RemoveChild(nsIView*) (nsViewManager.cpp:1420)
==30066==    by 0x4F10C75: nsView::~nsView() (nsView.cpp:224)
==30066==    by 0x4F0F895: nsIView::Destroy() (nsView.cpp:294)
==30066==    by 0x4F14EDF: nsViewManager::~nsViewManager() (nsViewManager.cpp:180)
==30066==    by 0x4F14927: nsViewManager::Release() (nsViewManager.cpp:226)
==30066==    by 0x5489504: nsCOMPtr_base::~nsCOMPtr_base() (nsCOMPtr.cpp:81)
==30066==    by 0x4BD2D9C: DocumentViewerImpl::~DocumentViewerImpl() (nsCOMPtr.h:469)
==30066==    by 0x4BCE2D7: DocumentViewerImpl::Release() (nsDocumentViewer.cpp:570)
==30066==    by 0x5489504: nsCOMPtr_base::~nsCOMPtr_base() (nsCOMPtr.cpp:81)
==30066==    by 0x4BD382D: DocumentViewerImpl::Show() (nsCOMPtr.h:469)
==30066==    by 0x4BE293B: nsPresContext::EnsureVisible() (nsPresContext.cpp:1583)
==30066==    by 0x4BE8213: PresShell::UnsuppressAndInvalidate() (nsPresShell.cpp:4619)
==30066==    by 0x4BE9A58: PresShell::UnsuppressPainting() (nsPresShell.cpp:4661)
==30066==    by 0x4BECCB1: PresShell::sPaintSuppressionCallback(nsITimer*, void*) (nsPresShell.cpp:2726)
==30066==    by 0x54CFE59: nsTimerImpl::Fire() (nsTimerImpl.cpp:427)
==30066==    by 0x54CFEF8: nsTimerEvent::Run() (nsTimerImpl.cpp:519)
==30066==    by 0x54CC2C1: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527)
==30066==    by 0x548F8FC: NS_ProcessPendingEvents_P(nsIThread*, unsigned int) (nsThreadUtils.cpp:189)
==30066==    by 0x53CFE5E: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:121)
==30066==    by 0x53B908D: nsAppShell::EventProcessorCallback(_GIOChannel*, GIOCondition, void*) (nsAppShell.cpp:70)
==30066==    by 0x41336C6: g_io_unix_dispatch (giounix.c:162)
==30066==    by 0x40FEE3B: g_main_context_dispatch (gmain.c:1836)
==30066==    by 0x41023C4: g_main_context_iterate (gmain.c:2467)
==30066==    by 0x41026B7: g_main_loop_run (gmain.c:2675)
==30066==    by 0x42ABCF8: gtk_main (gtkmain.c:1200)
==30066==    by 0x5AD8DFB: (below main) (in /targets/fr2009x86/lib/libc-2.5.so)
==30066==  Address 0x7d27d64 is 4 bytes inside a block of size 80 free'd
==30066==    at 0x4020449: operator delete(void*) (in /targets/fr2009x86/usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==30066==    by 0x53E4309: nsThebesDeviceContext::~nsThebesDeviceContext() (nsThebesDeviceContext.cpp:351)
==30066==    by 0x53E3957: nsThebesDeviceContext::Release() (nsThebesDeviceContext.cpp:295)
==30066==    by 0x4BE6B40: nsPresContext::~nsPresContext() (nsPresContext.cpp:309)
==30066==    by 0x4BE22C7: nsPresContext::Release() (nsPresContext.cpp:322)
==30066==    by 0x54895A9: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:456)
==30066==    by 0x4BD2C4C: DocumentViewerImpl::Destroy() (nsCOMPtr.h:640)
==30066==    by 0x4BD36CE: DocumentViewerImpl::Show() (nsDocumentViewer.cpp:1914)
==30066==    by 0x4BE293B: nsPresContext::EnsureVisible() (nsPresContext.cpp:1583)
==30066==    by 0x4BE8213: PresShell::UnsuppressAndInvalidate() (nsPresShell.cpp:4619)
==30066==    by 0x4BE9A58: PresShell::UnsuppressPainting() (nsPresShell.cpp:4661)
==30066==    by 0x4BECCB1: PresShell::sPaintSuppressionCallback(nsITimer*, void*) (nsPresShell.cpp:2726)
==30066==    by 0x54CFE59: nsTimerImpl::Fire() (nsTimerImpl.cpp:427)
==30066==    by 0x54CFEF8: nsTimerEvent::Run() (nsTimerImpl.cpp:519)
==30066==    by 0x54CC2C1: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527)
==30066==    by 0x548F8FC: NS_ProcessPendingEvents_P(nsIThread*, unsigned int) (nsThreadUtils.cpp:189)
==30066==    by 0x53CFE5E: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:121)
==30066==    by 0x53B908D: nsAppShell::EventProcessorCallback(_GIOChannel*, GIOCondition, void*) (nsAppShell.cpp:70)
==30066==    by 0x41336C6: g_io_unix_dispatch (giounix.c:162)
==30066==    by 0x40FEE3B: g_main_context_dispatch (gmain.c:1836)
==30066==    by 0x41023C4: g_main_context_iterate (gmain.c:2467)
==30066==    by 0x41026B7: g_main_loop_run (gmain.c:2675)
==30066==    by 0x42ABCF8: gtk_main (gtkmain.c:1200)

Comment 1

[Oleg Romashin (:romaxa)]

Here we are taking mDeviceContext pointer from PresContext without addref, and when PresContext destroying itself, then ViewManager having problems...

http://mxr.mozilla.org/mozilla1.9.2/source/layout/base/nsDocumentViewer.cpp#2280

Comment 2

[Oleg Romashin (:romaxa)]

Also if this line is removed:
http://hg.mozilla.org/releases/mozilla-1.9.2/file/4ca2f1951447/layout/base/nsDocumentViewer.cpp#l1604

Then Invalid read not appear anymore.

I think we should or give own reference for DeviceContext in nsViewManager, or  destroy mViewManager in nsDocumentViewer before destroying mDeviceContext

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Proposed fix.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm...  That's really fragile; what if someone else holds a strong ref to the viewmanager at that point?

Is there a reason the viewmanager can't just hold a strong ref to the device context?  Or that we can't null out the device context in the view manager at this point?

Comment 5

[Oleg Romashin (:romaxa)]

> Is there a reason the viewmanager can't just hold a strong ref to the device

I think it was so before this patch landed
https://bug229371.bugzilla.mozilla.org/attachment.cgi?id=140408
> context?  Or that we can't null out the device context in the view manager at
> this point?

What should I do? increase reference in nsViewManager->Init or increase refcount in after presContext->DeviceContext() call ?

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Keep refcount in nsViewManager

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I think it was so before this patch landed

That was a strong ref on the stack, not in the viewmanager...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 412149 [details] [diff] [review]
Keep refcount in nsViewManager

roc's a better reviewer for this (but at the very least, use nsCOMPtr?).

roc, should this be leak-safe?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 412149 [details] [diff] [review]
Keep refcount in nsViewManager

the nsIDeviceContext shouldn't be holding a (direct or indirect) reference all the way back to the view manager, certainly.

Comment 10

[Oleg Romashin (:romaxa)]

Pushed in:
http://hg.mozilla.org/mozilla-central/rev/83737f97719d

Comment 11

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

Do we want this on 1.9.2?  This valgrind warning is one of our more common; I could imagine it leading to crashes in some cases.

Comment 12

[Boris Zbarsky [:bzbarsky]]

romaxa, what happened to the "use nsCOMPtr" bit?

Comment 13

[Oleg Romashin (:romaxa)]

(In reply to comment #12)
> romaxa, what happened to the "use nsCOMPtr" bit?

Do you mean inside nsViewManager? use comPtr instead of raw pointer?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Do you mean inside nsViewManager? use comPtr instead of raw pointer?

Yes.

Comment 15

[Oleg Romashin (:romaxa)]

Yep, this change also fixing the problem.
---  nsIDeviceContext  *mContext;
+++  nsCOMPtr<nsIDeviceContext>  mContext;

Comment 16

[Boris Zbarsky [:bzbarsky]]

That's a much better solution than manual refcounting.  Much much better.

Comment 17

[Oleg Romashin (:romaxa)]

Should I create another bug? or attach new patch to this bug and re-fix current bug?

Comment 18

[Boris Zbarsky [:bzbarsky]]

The latter is fine, imo (and equivalent for branches; you'll want to request branch approval on the branch patch).

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Fix with comPtr

Comment 20

[Oleg Romashin (:romaxa)]

Adding checkin-needed keyword, not sure that I'm able to catch boxes state today/tomorrow...

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: Patch for 1.9.2 branch

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/daa0cd237513

Thanks

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c9e6759c6bb1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/18a793947bec