549236 - valgrind: nsDocShell::InternalLoad creates undefined nscoords

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Julian Seward [:jseward]]

There is a path through docshell/base/nsDocShell.cpp function
nsDocShell::InternalLoad which causes an undefined pair of nscoods
(constituting a ScrollPosition) to be inserted into a nsISHEntry
object.  This eventually surfaces in various parts of the system,
including jsengine, possibly because of running js chrome.

HOW TO REPRO: run dom/tests/mochitest/whatwg/test_bug500328.html
on Memcheck as described in bug 549224.

As per comments in the source, this may or may not have something
to do with the very-ancient bug 59744.

Some of the resulting errors are shown below this text.


ANALYSIS: problem begins at nsDocShell.cpp:7899 

7899:        nscoord cx, cy;

Control reaches 7953:

7953:                mOSHE->SetScrollPosition(cx, cy);

so the undefined values cx, cy are incorporated into the mOSHE object.
At line 8004 the undefined values are pulled back out, and passed to
SetCurScrollPosEx:

8001:   /* restore previous position of scroller(s), if we're moving
8002:    * back in history (bug 59774)
8003:    */
8004:   if (mOSHE && (aLoadType == LOAD_HISTORY || aLoadType == LOAD_RELOAD_NORMAL))
8005:   {
8006:       nscoord bx, by;
8007:       mOSHE->GetScrollPosition(&bx, &by);
8008:       SetCurScrollPosEx(bx, by);
8009:   }

A cascade of errors results.


The analysis is verified by setting cx and cy to silly values, and
observing that bx and by have those same values immediately prior to
the call to SetCurrScrollPosEx:

7899:        nscoord cx = 12345, cy = 67890;
...
8007:                mOSHE->GetScrollPosition(&bx, &by);
8008:                printf("bx = %d, by = %d\n", bx, by);
8009:                SetCurScrollPosEx(bx, by);

produces

  bx = 12345, by = 67890

and a vast mass of uninitialised-value errors disappear.

Trivial fix (in lieu of figuring out the complex control flow to
guarantee that all uses of cx/cy are dominated by their defs):
initialise cx and cy to zero.




Resulting errors (a few of many):

Conditional jump or move depends on uninitialised value(s)
   at 0x57A5481: nsGfxScrollFrameInner::ClampScrollPosition(nsPoint const&) const (nsGfxScrollFrame.cpp:1303)
   by 0x57A9DBF: nsGfxScrollFrameInner::ScrollTo(nsPoint, nsIScrollableFrame::ScrollMode) (nsGfxScrollFrame.cpp:1363)
   by 0x57ABD8B: nsHTMLScrollFrame::ScrollTo(nsPoint, nsIScrollableFrame::ScrollMode) (nsGfxScrollFrame.h:415)
   by 0x5D86F63: nsDocShell::SetCurScrollPosEx(int, int) (nsDocShell.cpp:4759)
   by 0x5D9FDB9: nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, unsigned short const*, char const*, nsIInputStr
eam*, nsIInputStream*, unsigned int, nsISHEntry*, int, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:8008)
   by 0x5D9702E: nsDocShell::LoadHistoryEntry(nsISHEntry*, unsigned int) (nsDocShell.cpp:9649)
   by 0x5D93232: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, int) (nsDocShell.cpp:1274)
   by 0x5DC6EB8: nsSHistory::InitiateLoad(nsISHEntry*, nsIDocShell*, long) (nsSHistory.cpp:1317)
   by 0x5DC6FAE: nsSHistory::CompareFrames(nsISHEntry*, nsISHEntry*, nsIDocShell*, long, int*) (nsSHistory.cpp:1257)
   by 0x5DC7179: nsSHistory::CompareFrames(nsISHEntry*, nsISHEntry*, nsIDocShell*, long, int*) (nsSHistory.cpp:1292)
   by 0x5DC77D3: nsSHistory::LoadEntry(int, long, unsigned int) (nsSHistory.cpp:1214)
   by 0x5DC6256: nsSHistory::GoForward() (nsSHistory.cpp:735)
 Uninitialised value was created by a stack allocation
   at 0x5D9E2B0: nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, unsigned short const*, char const*, nsIInputStr
eam*, nsIInputStream*, unsigned int, nsISHEntry*, int, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:7552)


Conditional jump or move depends on uninitialised value(s)
   at 0x5552FB2: XPCConvert::NativeData2JS(XPCLazyCallContext&, long*, void const*, nsXPTType const&, nsID const*, JSObject*, unsigned int*) (xpcconvert.cpp:257)
   by 0x556DC7B: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (xpcprivate.h:2997)
   by 0x5573497: XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) (xpcwrappednativejsops.cpp:1770)
   by 0x6B17D56: js_Invoke (jsinterp.cpp:1370)
   by 0x6B0807F: js_Interpret (jsops.cpp:2277)
   by 0x6B17FF7: js_Invoke (jsinterp.cpp:1378)
   by 0x6AFF851: js_fun_call (jsfun.cpp:1989)
   by 0x6B0E4F3: js_Interpret (jsops.cpp:2243)
   by 0x6B17FF7: js_Invoke (jsinterp.cpp:1378)
   by 0x5566DD9: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (xpcwrappedjsclass.cpp:1696)
   by 0x6102FB4: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:153)
   by 0x610247A: SharedStub (in /home/sewardj/MOZ/MC-25-02-2010/ff-opt/toolkit/library/libxul.so)
 Uninitialised value was created by a stack allocation
   at 0x5D9E2B0: nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, unsigned short const*, char const*, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, int, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:7552)

Conditional jump or move depends on uninitialised value(s)
   at 0x6B0AA5B: js_Interpret (jsops.cpp:366)
   by 0x6B17FF7: js_Invoke (jsinterp.cpp:1378)
   by 0x6AFF851: js_fun_call (jsfun.cpp:1989)
   by 0x6B0E4F3: js_Interpret (jsops.cpp:2243)
   by 0x6B17FF7: js_Invoke (jsinterp.cpp:1378)
   by 0x5566DD9: nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) (xpcwrappedjsclass.cpp:1696)
   by 0x6102FB4: PrepareAndDispatch (xptcstubs_x86_64_linux.cpp:153)
   by 0x610247A: SharedStub (in /home/sewardj/MOZ/MC-25-02-2010/ff-opt/toolkit/library/libxul.so)
   by 0x60F64C5: nsTimerImpl::Fire() (nsTimerImpl.cpp:435)
   by 0x60F659F: nsTimerEvent::Run() (nsTimerImpl.cpp:519)
   by 0x60F31FF: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527)
   by 0x60B9168: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
 Uninitialised value was created by a stack allocation
   at 0x5D9E2B0: nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, unsigned int, unsigned short const*, char const*, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, int, nsIDocShell**, nsIRequest**) (nsDocShell.cpp:7552)

Comment 1

[Julian Seward [:jseward]]

Attachment: proposed fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/73a103a1a7a4