582553 - nsSelectionState::IsEqual ignores error return from CompareBoundaryPoints

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Julian Seward [:jseward]]

m-c from 27 July 2010 (yesterday), x86_64-linux

TEST_PATH=editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html


nsSelectionState::IsEqual(nsSelectionState *aSelState) calls
myRange->CompareBoundaryPoints(...), and assumes it writes a
result to out parameter 'compResult':

163:    PRInt16 compResult;
164:    myRange->CompareBoundaryPoints(nsIDOMRange::START_TO_START, itsRange,
        &compResult);
165:    if (compResult) return PR_FALSE;
166:    myRange->CompareBoundaryPoints(nsIDOMRange::END_TO_END, itsRange,
        &compResult);
167:    if (compResult) return PR_FALSE;
(in editor/libeditor/base/nsSelectionState.cpp)

However, in this test myRange->CompareBoundaryPoints(...) sometimes
returns NS_ERROR_NOT_INITIALIZED, and other non-NS_OK values that
I could not identify.  In such cases it does not write a value to
&compResult, so the code above branches on garbage when that happens.
(same problem at 166/167).

From examination of
nsRange::CompareBoundaryPoints(PRUint16 aHow, nsIDOMRange* aOtherRange,
                               PRInt16* aCmpRet)
in
content/base/src/nsRange.cpp, which I presume is what it is calling,
it is clear that NS_ERROR_DOM_INVALID_STATE_ERR, NS_ERROR_NOT_INITIALIZED,
NS_ERROR_ILLEGAL_VALUE and NS_ERROR_DOM_WRONG_DOCUMENT_ERR can be returned
with no assignment to the out parameter.



Valgrind complains accordingly:

Conditional jump or move depends on uninitialised value(s)
   at 0x5B712C2: nsSelectionState::IsEqual(nsSelectionState*)
                 (nsSelectionState.cpp:165)
   by 0x5B71CD7: PlaceholderTxn::StartSelectionEquals(nsSelectionState*, int*)
                 (PlaceholderTxn.cpp:259)
   by 0x5B72045: PlaceholderTxn::Merge(nsITransaction*, int*)
                 (PlaceholderTxn.cpp:210)
   by 0x5F16937: nsTransactionManager::EndTransaction()
                 (nsTransactionManager.cpp:1037)
   by 0x5F15317: nsTransactionManager::DoTransaction(nsITransaction*)
                 (nsTransactionManager.cpp:126)
   by 0x5B6315C: nsEditor::DoTransaction(nsITransaction*) (nsEditor.cpp:644)
   by 0x5B63298: nsEditor::DoTransaction(nsITransaction*) (nsEditor.cpp:591)
   by 0x5B683F9: nsEditor::CreateNode(nsAString_internal const&, nsIDOMNode*,
                 int, nsIDOMNode**) (nsEditor.cpp:1307)
   by 0x5D627CB: nsHTMLEditor::CreateBRImpl(nsCOMPtr<nsIDOMNode>*, int*,
                 nsCOMPtr<nsIDOMNode>*, short) (nsHTMLEditor.cpp:1563)
   by 0x5D5F2B4: nsHTMLEditor::CreateBR(nsIDOMNode*, int, nsCOMPtr<nsIDOMNode>*,
                 short) (nsHTMLEditor.cpp:1600)
   by 0x5D62369: nsHTMLEditor::InsertBR(nsCOMPtr<nsIDOMNode>*)
                 (nsHTMLEditor.cpp:1629)
   by 0x5D5CCB7: nsHTMLEditor::TypedText(nsAString_internal const&, int)
                 (nsHTMLEditor.cpp:1425)

 Uninitialised value was created by a stack allocation
   at 0x5B71210: nsSelectionState::IsEqual(nsSelectionState*)
                 (nsSelectionState.cpp:150)

and an identical complaint 2 lines later, at nsSelectionState.cpp:167.

Comment 1

[(no longer active)]

Julain, thanks for catching this.  Did you figure this out only by running the tests under valgrind?

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Comment 3

[Julian Seward [:jseward]]

(In reply to comment #1)
> Did you figure this out only by running the tests under valgrind?

Yes.  I periodically (attempt to) run all of Mochitests through
Valgrind, and found this one in the latest run.  I haven't seen it before.

If you have a build on Linux (or Mac) you can probably reproduce this
with a command along these lines:

TEST_PATH=editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html DISPLAY=:3.0 make -C ff-opt mochitest-plain EXTRA_TEST_ARGS='--close-when-done --debugger=valgrind --debugger-args="--tool=memcheck --suppressions=/home/sewardj/MOZ/mochitest-mc.supp --error-limit=no --stats=no --vex-guest-chase-cond=yes --trace-children=yes --child-silent-after-fork=yes '--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel' --track-origins=yes"'

Comment 4

[Julian Seward [:jseward]]

I should add that I believe there is a similar problem in
nsSelectionState::IsCollapsed, just above:

  nsSelectionState::IsCollapsed()
  {
    if (1 != mArray.Length()) return PR_FALSE;
    nsCOMPtr<nsIDOMRange> range;
    mArray[0].GetRange(address_of(range));
    NS_ENSURE_TRUE(range, PR_FALSE);
    PRBool bIsCollapsed;
    range->GetCollapsed(&bIsCollapsed);
    return bIsCollapsed;
  }

If ->GetCollapsed(...) can return without writing to bIsCollapsed
then IsCollapsed will return garbage.  I think this is the cause of
the error below, which is also shown by
TEST_PATH=editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html
(look at the last 2 lines of the error)

Conditional jump or move depends on uninitialised value(s)
   at 0x5B71C9A: PlaceholderTxn::StartSelectionEquals
                 (PlaceholderTxn.cpp:254)
   by 0x5B72015: PlaceholderTxn::Merge (PlaceholderTxn.cpp:210)
   by 0x5F16907: nsTransactionManager::EndTransaction
                 (nsTransactionManager.cpp:1037)
   by 0x5F152E7: nsTransactionManager::DoTransaction
                 (nsTransactionManager.cpp:126)
   by 0x5B6315C: nsEditor::DoTransaction (nsEditor.cpp:644)
   by 0x5B63298: nsEditor::DoTransaction (nsEditor.cpp:591)
   by 0x5B683F9: nsEditor::CreateNode (nsEditor.cpp:1307)
   by 0x5D6279B: nsHTMLEditor::CreateBRImpl (nsHTMLEditor.cpp:1563)
   by 0x5D5F284: nsHTMLEditor::CreateBR (nsHTMLEditor.cpp:1600)
   by 0x5D62339: nsHTMLEditor::InsertBR (nsHTMLEditor.cpp:1629)
   by 0x5D5CC87: nsHTMLEditor::TypedText (nsHTMLEditor.cpp:1425)
   by 0x5D65F35: nsHTMLEditor::HandleKeyPressEvent (nsHTMLEditor.cpp:713)
 Uninitialised value was created by a stack allocation
   at 0x5B704D0: nsSelectionState::IsCollapsed (nsSelectionState.cpp:138)

Comment 5

[(no longer active)]

Attachment: Part 2

(In reply to comment #4)
> I should add that I believe there is a similar problem in
> nsSelectionState::IsCollapsed, just above:

Let's fix that!

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Under what conditions is CompareBoundaryPoints actually failing? Or GetCollapsed?

Comment 7

[Julian Seward [:jseward]]

Attachment: combined version of the previous 2 patches

Verified as memcheck-clean.

Comment 8

[Julian Seward [:jseward]]

(In reply to comment #6)
> Under what conditions is CompareBoundaryPoints actually failing?

It sometimes returns NS_ERROR_NOT_INITIALISED, and also another
non-NS_OK value.  Or is that not what you meant?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I mean *why* is that happening?

Comment 10

[(no longer active)]

For example, if one of the ranges in comparison is detached or not yet positioned.  And also on error conditions.

Comment 11

[(no longer active)]

roc: ping?

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/517874d6eece
http://hg.mozilla.org/mozilla-central/rev/cba4071f3551