Julian Seward [:jseward] Reporter
	Description • 9 years ago

Running mochitest-chrome on Valgrind (something I should have done much earlier)
I get reports of uninitialised values originating from stack allocation in 
mozilla::a11y::xpcAccessibleTable::GetSelectedRowIndices(unsigned int*, int**)
and also
mozilla::a11y::xpcAccessibleTable::GetSelectedCellIndices(unsigned int*, int**)
and leaking into various parts of the Javascript implementation.

I had a brief look but haven't made sense of what's going on (yet).

	Julian Seward [:jseward] Reporter
	Comment 2 • 9 years ago

I should point out, I patched in (by hand) the fix for the just-landed
bug 1180189 (crash in mozilla::a11y::HTMLTableRowAccessible::GroupPosition())
since that has the feel of being related, but that did not help.

STR:
Test = accessible/tests/mochitest/table/test_sels_listbox.xul

I ran:
G_SLICE=always-malloc MOZ_DISABLE_NONLOCAL_CONNECTIONS=0 ./mach mochitest --debugger=/home/sewardj/VgTRUNK/mozhx/Inst/bin/valgrind  --debugger-args="--fair-sched=yes --smc-check=all-non-file --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc-may2015.supp --error-limit=no --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,*/bin/java --num-transtab-sectors=24 --tool=memcheck --freelist-vol=500000000 --redzone-size=128 --gen-suppressions=no --px-default=allregs-at-mem-access --px-file-backed=unwindregs-at-mem-access --stats=yes --partial-loads-ok=yes --show-mismatched-frees=no --read-inline-info=yes --fullpath-after=-CURR/ --num-callers=16 --track-origins=no"  ./accessible/tests/mochitest/table/test_sels_listbox.xul 2>&1 | tee spew-14-mc

	Marco Zehe (:MarcoZ)
	Comment 3 • 9 years ago

Thanks for finding these, Julian! Bug 1180189 doesn't look related to me, since you're testing XULListboxAccessibles, whereas the crash in bug 1180189 was related to HTMLTableAccessibles.

Alex, can you take a look?

	Trevor Saunders (:tbsaunde)
	Comment 4 • 9 years ago

> I had a brief look but haven't made sense of what's going on (yet).

yeah at a first glance that being the true origin doesn't make sense to me either.  However maybe this is an after effect of bug 1153620 which iirc is the side effect of an ancient bug where something claims to be a nsIDOMNodeList, but isn't

	Julian Seward [:jseward] Reporter
	Comment 5 • 9 years ago

It would be nice to get this fixed.  I investigated more.  I wonder if it is
fallout from bug 1076816 ("segregate XPCOM tree").  What I see is:

The uninitialised values are created on the stack, in
xpcAccessibleTable::GetSelectedRowIndices (accessible/xpcom/xpcAccessibleTable.cpp)
as the elements of |rowsArray|.  IIUC, this creates an array on
the stack with XPC_TABLE_DEFAULT_SIZE as-yet not-filled-in items.

  nsAutoTArray<uint32_t, XPC_TABLE_DEFAULT_SIZE> rowsArray;

This then calls 

  Intl()->SelectedRowIndices(&rowsArray);

to fill the items in.  That winds up calling

XULListboxAccessible::SelectedRowIndices (accessible/xul/XULListboxAccessible.cpp)

and I don't really understand the logic in there for resizing the array
|*aRows| into which the row indices are to be written.  But I suspect
that something isn't right here.  In particular the caller creates an array
of size XPC_TABLE_DEFAULT_SIZE, but ::SelectedRowIndices can return without
writing anything into the array and without resizing it to zero:

  if (!selectedItems)
    return;

and

  if (!rowCount)
    return;

and even if it does resize it ..

  aRows->SetCapacity(rowCount);
  aRows->AppendElements(rowCount);

there's no guarantee that all the elements will get filled in

      int32_t itemIdx = -1;
      control->GetIndexOfItem(item, &itemIdx);
      if (itemIdx >= 0)
        aRows->ElementAt(rowIdx) = itemIdx;

Having said that my understanding of TArray isn't good.  So maybe I missed
something here.

Can someone who understands this code have a look, please?

	Julian Seward [:jseward] Reporter
	Comment 6 • 9 years ago

>       int32_t itemIdx = -1;
>       control->GetIndexOfItem(item, &itemIdx);
>       if (itemIdx >= 0)
>         aRows->ElementAt(rowIdx) = itemIdx;

In fact it is exactly to do with this loop.  If I put this immediately
before it

+  for (uint32_t i = 0; i < rowCount; i++)
+    aRows->ElementAt(i) = 0;

then Valgrind stops complaining.  But this doesn't really seem like a
good fix to me, since it merely returns zeroes where it would
previously have left holes in the output array.  Would it be better
instead to incrementally write into the output array, so there are no
gaps (logical or actual) in it?

The same problem occurs in, and the same not-really-a-fix works for
XULListboxAccessible::SelectedCellIndices.

	Trevor Saunders (:tbsaunde)
	Comment 8 • 9 years ago

(In reply to Julian Seward [:jseward] from comment #5)
> It would be nice to get this fixed.  I investigated more.  I wonder if it is
> fallout from bug 1076816 ("segregate XPCOM tree").  What I see is:

no, its a dup of bug  1153620.

> The uninitialised values are created on the stack, in
> xpcAccessibleTable::GetSelectedRowIndices
> (accessible/xpcom/xpcAccessibleTable.cpp)
> as the elements of |rowsArray|.  IIUC, this creates an array on
> the stack with XPC_TABLE_DEFAULT_SIZE as-yet not-filled-in items.
> 
>   nsAutoTArray<uint32_t, XPC_TABLE_DEFAULT_SIZE> rowsArray;
> 
> This then calls 
> 
>   Intl()->SelectedRowIndices(&rowsArray);
> 
> to fill the items in.  That winds up calling
> 
> XULListboxAccessible::SelectedRowIndices
> (accessible/xul/XULListboxAccessible.cpp)
> 
> and I don't really understand the logic in there for resizing the array
> |*aRows| into which the row indices are to be written.  But I suspect
> that something isn't right here.  In particular the caller creates an array
> of size XPC_TABLE_DEFAULT_SIZE, but ::SelectedRowIndices can return without
> writing anything into the array and without resizing it to zero:

no, it is an array with that capacity and size 0.

> and even if it does resize it ..
> 
>   aRows->SetCapacity(rowCount);
>   aRows->AppendElements(rowCount);
> 
> there's no guarantee that all the elements will get filled in

I'd say in the absence of bugs in the called functions they should all be filled in.  Which means those ifs really should be asserts.

	Julian Seward [:jseward] Reporter
	Comment 9 • 9 years ago

Fix, as discussed over irc with tbsaunde.  Note, this adds
assertions to SelectedCellIndices() and SelectedRowIndices()
that the arrays passed from the callers are empty, since, 
as far as I can see, the code does assume that.

	alexander :surkov (:asurkov)
	Comment 10 • 9 years ago

no needinfo from me I guess since Trevor is up on this

	Trevor Saunders (:tbsaunde)
	Comment 11 • 9 years ago

Sorry about the lag! I kept meaning to comment here and getting distracted.  I think i've gotten the root issue fixed, would you mind retesting this?

	Julian Seward [:jseward] Reporter
	Comment 12 • 9 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
With m-c and without my patch, I cannot reproduce the problem now.
So it seems good to me.

Out of curiousity, can you point me at the bug/changeset that
fixes it?

	Trevor Saunders (:tbsaunde)
	Comment 13 • 9 years ago

f

	Trevor Saunders (:tbsaunde)
	Comment 14 • 9 years ago

fixed by bug 120684