Closed
Bug 726592
Opened 12 years ago
Closed 12 years ago
Uninitialised value use in nsTextFrame::GetChildFrameContainingOffset
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: jseward, Assigned: ehsan.akhgari)
Details
Attachments
(1 file)
12.22 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•12 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
Assignee | ||
Comment 2•12 years ago
|
||
Hmm, I suspect this is the nsPeekOffsetStruct::mContentOffset which is not initialized by calling SetData...
Reporter | ||
Comment 3•12 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, ¤tOffset); 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•12 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?
Assignee | ||
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Comment 7•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f94293896d
Target Milestone: --- → mozilla14
Comment 10•12 years ago
|
||
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.
Description
•