The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla6

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: Fernando Herrera)

Tracking

(Blocks: 1 bug)

Trunk
mozilla6
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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)

Updated

6 years ago
Blocks: 368895
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

6 years ago
(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

6 years ago
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?
(Assignee)

Comment 4

6 years ago
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

6 years ago
(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.
(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
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.
(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.
(Reporter)

Comment 9

6 years ago
(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
(Reporter)

Comment 10

6 years ago
This should work better:

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

Comment 11

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

Looks like that gets more test failures, too.
Duplicate of this bug: 633831

Comment 13

6 years ago
I don't know why I bother to file bugs.
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.
(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 :)
(Assignee)

Comment 16

6 years ago
(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

6 years ago
landed - http://hg.mozilla.org/mozilla-central/rev/5ebbe0f78c74 (the patch was attached to bug 652459).
Assignee: nobody → fherrera
Blocks: 652459
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6

Updated

6 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.