Last Comment Bug 726592 - Uninitialised value use in nsTextFrame::GetChildFrameContainingOffset
: Uninitialised value use in nsTextFrame::GetChildFrameContainingOffset
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-13 07:38 PST by Julian Seward [:jseward]
Modified: 2012-03-27 05:10 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (12.22 KB, patch)
2012-03-23 11:08 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review

Description Julian Seward [:jseward] 2012-02-13 07:38:49 PST
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)
Comment 1 Julian Seward [:jseward] 2012-03-23 06:11:20 PDT
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
Comment 2 :Ehsan Akhgari 2012-03-23 10:05:38 PDT
Hmm, I suspect this is the nsPeekOffsetStruct::mContentOffset which is not initialized by calling SetData...
Comment 3 Julian Seward [:jseward] 2012-03-23 10:27:20 PDT
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.
Comment 4 Julian Seward [:jseward] 2012-03-23 10:30:40 PDT
(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?
Comment 5 :Ehsan Akhgari 2012-03-23 10:58:42 PDT
(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.
Comment 6 :Ehsan Akhgari 2012-03-23 11:08:39 PDT
Created attachment 608774 [details] [diff] [review]
Patch (v1)
Comment 7 :Ehsan Akhgari 2012-03-23 11:12:19 PDT
https://tbpl.mozilla.org/?tree=Try&rev=caf8d1cc31ab
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-25 15:37:25 PDT
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 :-)
Comment 10 Marco Bonardo [::mak] 2012-03-27 05:10:53 PDT
https://hg.mozilla.org/mozilla-central/rev/d3f94293896d

Note You need to log in before you can comment on or make changes to this bug.