Closed Bug 726592 Opened 12 years ago Closed 12 years ago

Uninitialised value use in nsTextFrame::GetChildFrameContainingOffset

Categories

(Core :: DOM: Editor, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jseward, Assigned: ehsan.akhgari)

Details

Attachments

(1 file)

TEST_PATH=editor/libeditor/html/tests/browserscope/test_richtext2.html

(DISPLAY=:3.0 make -C ff-opt mochitest-plain TEST_PATH=editor/libeditor/html/tests/browserscope/test_richtext2.html EXTRA_TEST_ARGS='--close-when-done --debugger=/home/sewardj/VgTRUNK/merge/Inst/bin/valgrind --debugger-args="--smc-check=all-non-file --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc.supp --error-limit=no --stats=yes --trace-children=yes --child-silent-after-fork=yes '--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep' --tool=memcheck --track-origins=yes --stats=yes"') 2>&1 | cat

produces the complaint below, plus a bunch of related other ones

Conditional jump or move depends on uninitialised value(s)
   at 0x62EA8B8: nsTextFrame::GetChildFrameContainingOffset(int, bool, int*, nsIFrame**) (nsTextFrameThebes.cpp:6062)
   by 0x62DAC85: nsFrameSelection::GetFrameForNodeOffset(nsIContent*, int, nsFrameSelection::HINT, int*) const (nsSelection.cpp:2118)
   by 0x62E4755: nsFrameSelection::MoveCaret(unsigned int, bool, nsSelectionAmount, bool) (nsSelection.cpp:1258)
   by 0x62E4C2C: nsTypedSelection::Modify(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&) (nsSelection.cpp:5866)
   by 0x6DA1CB7: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:195)
   by 0x69242BA: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2898)
   by 0x692A5CF: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1539)
   by 0x70ABB76: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x70A588D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2772)
   by 0x70AB7F8: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:657)
   by 0x70BBBFF: EvalKernel(JSContext*, js::CallArgs const&, EvalType, js::StackFrame*, JSObject&) (jsobj.cpp:1088)
   by 0x70A5E5B: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2742)

 Uninitialised value was created by a stack allocation
   at 0x62E4280: nsFrameSelection::MoveCaret(unsigned int, bool, nsSelectionAmount, bool) (nsSelection.cpp:1110)
Whilst chasing this I noticed what looks like some type confusion ..

layout/generic/nsTextFrameThebes.cpp:6035 ish
nsTextFrame::GetChildFrameContainingOffset(PRInt32   aContentOffset,
                                           bool      aHint,
                                           PRInt32*  aOutOffset,
                                           nsIFrame**aOutFrame)
is called from
layout/generic/nsSelection.cpp:2115
  returnFrame->GetChildFrameContainingOffset(*aReturnOffset, aHint,
                                             &aOffset, &returnFrame);
but second arg (aHint) has type HINT, not bool
Hmm, I suspect this is the nsPeekOffsetStruct::mContentOffset which is not initialized by calling SetData...
I tracked this down to nsFrameSelection::MoveCaret in nsSelection.cpp.
Here's what I have.  Anybody know how to fix it cleanly?

Line numbers might be not quite right.

1188  nsPeekOffsetStruct pos;

All fields uninitialised, including mAttachForward.
Then we set some but not all fields of pos, by calling PeekOffset

1234  if (NS_SUCCEEDED(result = frame->PeekOffset(&pos)) && pos.mResultContent)

and at this point mAttachForward is still uninitialised (I verified that)
but is passed (now as tHint) to GetFrameForNodeOffset, which eventually
uses it, resulting the the errors reported originally:

1255      tHint = (HINT)pos.mAttachForward;
1256      theFrame = GetFrameForNodeOffset(pos.mResultContent, pos.mContentOffset,
1257                                       tHint, &currentOffset);

Setting pos.mAttachForward = false immediately following its
declaration at line 1188 makes all the valgrind errors go away,
confirming this analysis.


The comment at line 1241 seems related.  It is however within the
then- arm of an if, and the control flow in this case visits the
else- arm instead.  Perhaps the comment applies to both arms?

1241  // For left/right, PeekOffset() sets pos.mResultFrame correctly, but does not set pos.mAttachForward,
1242  // so determine the hint here based on the result frame and offset:
1243  // If we're at the end of a text frame, set the hint to HINTLEFT to indicate that we
1244  // want the caret displayed at the end of this frame, not at the beginning of the next one.
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> Hmm, I suspect this is the nsPeekOffsetStruct::mContentOffset which is not
> initialized by calling SetData...

Seems more like nsPeekOffsetStruct::mAttachForward is not initialised by the call
to SetData.  Could that be the case?
(In reply to Julian Seward from comment #4)
> (In reply to Ehsan Akhgari [:ehsan] from comment #2)
> > Hmm, I suspect this is the nsPeekOffsetStruct::mContentOffset which is not
> > initialized by calling SetData...
> 
> Seems more like nsPeekOffsetStruct::mAttachForward is not initialised by the
> call
> to SetData.  Could that be the case?

Yes.  I have a patch which adds a proper ctor to nsPeekOffsetStruct to ensure that all instances of this class will always have all of their fields initialized.
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #608774 - Flags: review?(roc)
Comment on attachment 608774 [details] [diff] [review]
Patch (v1)

Review of attachment 608774 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsIFrame.h
@@ +2704,4 @@
>    // The above methods have been migrated from nsIBox and are in the process of
>    // being refactored. DO NOT USE OUTSIDE OF XUL.
>  
> +  struct CaretPositionInformation {

Call it CaretPosition? "Information" is sort of meaningless here, all data is information :-)
Attachment #608774 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/d3f94293896d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: