Uninitialised value use in nsTextFrame::GetChildFrameContainingOffset

RESOLVED FIXED in mozilla14

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jseward, Assigned: Ehsan)

Tracking

Trunk
mozilla14
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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)
(Reporter)

Comment 1

6 years ago
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...
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
(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.
Created attachment 608774 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #608774 - Flags: review?(roc)
https://tbpl.mozilla.org/?tree=Try&rev=caf8d1cc31ab
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/integration/mozilla-inbound/rev/d3f94293896d
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/d3f94293896d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.