Last Comment Bug 654999 - Valgrind complains about unitialized values created by stack allocation in nsHyperTextAccessible::GetTextHelper
: Valgrind complains about unitialized values created by stack allocation in ns...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All Linux
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Fernando Herrera
:
Mentors:
: 633831 (view as bug list)
Depends on:
Blocks: texta11y 652459
  Show dependency treegraph
 
Reported: 2011-05-05 06:49 PDT by Mike Hommey [:glandium]
Modified: 2011-05-21 00:13 PDT (History)
7 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
valgrind log (704.33 KB, text/plain)
2011-05-05 06:49 PDT, Mike Hommey [:glandium]
no flags Details

Description Mike Hommey [:glandium] 2011-05-05 06:49:34 PDT
Created attachment 530299 [details]
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] 2011-05-12 15:16:14 PDT
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 2011-05-13 02:26:58 PDT
(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 2011-05-13 02:28:40 PDT
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 2011-05-13 02:35:32 PDT
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 2011-05-13 03:22:12 PDT
(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) 2011-05-13 05:34:40 PDT
(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] 2011-05-13 06:32:34 PDT
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) 2011-05-13 20:39:24 PDT
(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] 2011-05-13 23:59:46 PDT
(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] 2011-05-15 02:29:02 PDT
This should work better:

   if (aType == eGetBefore) {
-    endOffset = aOffset;
+    finalEndOffset = aOffset;
   }
Comment 11 Mike Hommey [:glandium] 2011-05-15 03:10:30 PDT
(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 12 Trevor Saunders (:tbsaunde) 2011-05-16 00:10:08 PDT
*** Bug 633831 has been marked as a duplicate of this bug. ***
Comment 13 Bob Clary [:bc:] 2011-05-16 03:28:01 PDT
I don't know why I bother to file bugs.
Comment 14 David Bolter [:davidb] ***PTO until 29th*** 2011-05-16 09:01:49 PDT
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] ***PTO until 29th*** 2011-05-16 09:08:09 PDT
(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 2011-05-18 09:54:56 PDT
(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 2011-05-21 00:08:53 PDT
landed - http://hg.mozilla.org/mozilla-central/rev/5ebbe0f78c74 (the patch was attached to bug 652459).

Note You need to log in before you can comment on or make changes to this bug.