654999 - Valgrind complains about unitialized values created by stack allocation in nsHyperTextAccessible::GetTextHelper

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mike Hommey [:glandium]]

Attachment: valgrind log

When running test_singleline.html for bug 652459, I got these errors reported by valgrind. (JIT supposedly disabled ; the few first complaints are not related)

Comment 1

[Julian Seward [:jseward]]

I can't reproduce this.  OTOH I'm not convinced I'm actually running
anything:

TEST_PATH=accessible/tests/mochitest/text/test_singleline.html \
   make -C ff-opt mochitest-plain

gets me a 404 Not Found:
/tests/accessible/tests/mochitest/text/test_singleline.html was not
found.

What gives?  TEST_PATH= has worked reliably in the past.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)

> gets me a 404 Not Found:
> /tests/accessible/tests/mochitest/text/test_singleline.html was not
> found.
> 
> What gives?  TEST_PATH= has worked reliably in the past.

This test was disabled for a some time, I think it should be enabled now. You should try trunk again.

Comment 3

[alexander :surkov (:asurkov)]

Btw, GetTextHelper is going away, I think Fernando on this way. So maybe we shouldn't care about this bug too much? What's impact of this bug?

Comment 4

[Fernando Herrera]

The problem is really at GetPosAndText (the one calling GetRenderedText). And we are not only using GetPosAndText for GetTextHelper, but also for stuff like GetRangeExtents, HypertextOffsetsToDOMRange etc...

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> The problem is really at GetPosAndText (the one calling GetRenderedText).
> And we are not only using GetPosAndText for GetTextHelper, but also for
> stuff like GetRangeExtents, HypertextOffsetsToDOMRange etc...

it doesn't sound like we need GetPosAndText for HypertextOffsetsToDOMRange (need to fix it too). GetRangeExtents maybe a special case, but iirc they said it somehow broken so we probably need to change it implementation. Let's see.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to comment #1)
> I can't reproduce this.  OTOH I'm not convinced I'm actually running
> anything:
> 
> TEST_PATH=accessible/tests/mochitest/text/test_singleline.html \
>    make -C ff-opt mochitest-plain
> 
> gets me a 404 Not Found:
> /tests/accessible/tests/mochitest/text/test_singleline.html was not
> found.
> 
> What gives?  TEST_PATH= has worked reliably in the past.

for one thing a11y tests are in mochitest-a11y not mochitest-plain, and for another the paths are a little weird, I think the path you actually want is TEST_PATH=accessible/text/test_singleline.html

Comment 7

[Julian Seward [:jseward]]

Seems like the problem is in nsHyperTextAccessible::GetTextHelper,
in nsHyperTextAccessible.cpp:

We have

1002  PRInt32 finalStartOffset, finalEndOffset;

finalEndOffset is uninitialised.

1016  if (aType == eGetBefore) {

assume this is taken.  Then we arrive at

1067  GetPosAndText(finalStartOffset, finalEndOffset, &aText);

without any assignment to finalEndOffset.  In GetPosAndText is

282  if (aEndOffset == nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT) {

and at that point Valgrind complains, since that's the first place
the undefinedness affects control flow.  The undefinedness seems to
seem into a lot of places -- it is reported at 450 different stack
traces when running test_singleline.html.

Changing 1002 like this

1002  PRInt32 finalStartOffset, finalEndOffset = 0;

makes Valgrind stop complaining.  It unfortunately makes a whole
bunch of the tests fail, though.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to comment #3)
> Btw, GetTextHelper is going away, I think Fernando on this way. So maybe we
> shouldn't care about this bug too much? What's impact of this bug?

well, I'm not sure but I wouldn't be completely suprised if this is what's causing the permaoranges in 652495, so unless fer will have the text stuff done rsn I think we need to fix this.  (it went permaorange again tonight with a tm merge)
(In reply to comment #7)
> Seems like the problem is in nsHyperTextAccessible::GetTextHelper,
> in nsHyperTextAccessible.cpp:
> 
> We have
> 
> 1002  PRInt32 finalStartOffset, finalEndOffset;
> 
> finalEndOffset is uninitialised.
> 
> 1016  if (aType == eGetBefore) {
> 
> assume this is taken.  Then we arrive at
> 
> 1067  GetPosAndText(finalStartOffset, finalEndOffset, &aText);
> 
> without any assignment to finalEndOffset.  In GetPosAndText is
> 
> 282  if (aEndOffset == nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT) {
> 
> and at that point Valgrind complains, since that's the first place
> the undefinedness affects control flow.  The undefinedness seems to
> seem into a lot of places -- it is reported at 450 different stack
> traces when running test_singleline.html.
> 
> Changing 1002 like this
> 
> 1002  PRInt32 finalStartOffset, finalEndOffset = 0;
> 
> makes Valgrind stop complaining.  It unfortunately makes a whole
> bunch of the tests fail, though.

If we assume the tests are correct then there must be some value that finalEndOffset has a lot of the time that makes things work correctly, I wonder what said value is.

Comment 9

[Mike Hommey [:glandium]]

(In reply to comment #8)
> well, I'm not sure but I wouldn't be completely suprised if this is what's
> causing the permaoranges in 652495

I guess you meant 652459

Comment 10

[Mike Hommey [:glandium]]

This should work better:

   if (aType == eGetBefore) {
-    endOffset = aOffset;
+    finalEndOffset = aOffset;
   }

Comment 11

[Mike Hommey [:glandium]]

(In reply to comment #10)
> This should work better:
> 
>    if (aType == eGetBefore) {
> -    endOffset = aOffset;
> +    finalEndOffset = aOffset;
>    }

Looks like that gets more test failures, too.

Comment 13

[Bob Clary [:bc] (inactive)]

I don't know why I bother to file bugs.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Good news - this aligns with one of our a11y goals!

We should prioritize this so that we aren't blocking anyone. I chatted with Fer today and it sounds like the following needs to happen:

(Correct me if I'm wrong)

We need to remove all calls to GetPosAndText. This requires we finish and land Fer's "GetText rewrite WIP" on bug 630001. We need to handle the "lines" case, over on bug 634202. We need a patch for that. Fernando estimates this line boundary patch will be done today-ish.

Alexander we need your direction on the word boundary WIP, and an estimate if possible.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #13)
> I don't know why I bother to file bugs.

Yeah, you were first. Sorry that bug didn't get more attention -- resources and priorities -- we're working on both :)

Comment 16

[Fernando Herrera]

(In reply to comment #11)
> (In reply to comment #10)
> > This should work better:
> > 
> >    if (aType == eGetBefore) {
> > -    endOffset = aOffset;
> > +    finalEndOffset = aOffset;
> >    }
> 
> Looks like that gets more test failures, too.

Oh, sorry, I missed that. Those are not real tet failures. It actually fixed some cases where we were failing and were marked as kTodo.

Comment 17

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/5ebbe0f78c74 (the patch was attached to bug 652459).