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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

1.9.2 Branch
Other
Linux
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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)
(Assignee)

Comment 1

9 years ago
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
(Assignee)

Updated

9 years ago
(Assignee)

Comment 2

9 years ago
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
(Assignee)

Comment 3

9 years ago
Created attachment 411941 [details] [diff] [review]
Proposed fix.
Attachment #411941 - Flags: review?(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?
(Assignee)

Comment 5

9 years ago
> 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 ?
(Assignee)

Comment 6

9 years ago
Created attachment 412149 [details] [diff] [review]
Keep refcount in nsViewManager
Attachment #411941 - Attachment is obsolete: true
Attachment #412149 - Flags: review?(bzbarsky)
Attachment #411941 - Flags: review?(bzbarsky)
> I think it was so before this patch landed

That was a strong ref on the stack, not in the viewmanager...
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?
Attachment #412149 - Flags: review?(bzbarsky) → review?(roc)
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.
(Assignee)

Comment 10

9 years ago
Pushed in:
http://hg.mozilla.org/mozilla-central/rev/83737f97719d
Assignee: nobody → romaxa
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
Flags: wanted1.9.2?
romaxa, what happened to the "use nsCOMPtr" bit?
(Assignee)

Comment 13

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

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

Yes.
(Assignee)

Comment 15

9 years ago
Yep, this change also fixing the problem.
---  nsIDeviceContext  *mContext;
+++  nsCOMPtr<nsIDeviceContext>  mContext;
That's a much better solution than manual refcounting.  Much much better.
(Assignee)

Comment 17

9 years ago
Should I create another bug? or attach new patch to this bug and re-fix current bug?
The latter is fine, imo (and equivalent for branches; you'll want to request branch approval on the branch patch).
(Assignee)

Comment 19

9 years ago
Created attachment 412671 [details] [diff] [review]
Fix with comPtr
Attachment #412671 - Flags: review?(bzbarsky)
Attachment #412671 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

9 years ago
Adding checkin-needed keyword, not sure that I'm able to catch boxes state today/tomorrow...
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
(Assignee)

Comment 21

9 years ago
Created attachment 412676 [details] [diff] [review]
Patch for 1.9.2 branch
Flags: wanted1.9.2? → blocking1.9.2+
http://hg.mozilla.org/mozilla-central/rev/daa0cd237513

Thanks
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Whiteboard: [needs 192 landing]
Duplicate of this bug: 515799
You need to log in before you can comment on or make changes to this bug.