376884 - Crashing site reported -- with disability access

Status: RESOLVED DUPLICATE of bug 376924

(Firefox :: Disability Access, defect)

Description

[Tim Miao]

Attachment: stacktrace on solaris

This site will crash the firefox.
Steps to reproduce:
1. Launch orca2.19.0 and firefox 20070331 nightly trunk build.
2. Press Tab key to browse the page.

Bug observation:
Firefox crashed if user keeps on tab browsing.

Comment 1

[Tim Miao]

crashing site url: https://www.bankofamerica.com/index.jsp

Comment 2

[Aaron Leventhal]

It's in nsHyperTextAccessible::GetText

Is there a talkback report? Providing a link to a talkback report makes this much, much easier.

Comment 3

[Mike Pedersen]

Hi Aaron, it was me that pointed this out to Tim and Evan however as talkback is inaccessible I can't get you this information.  Hopefully you'll be able to reproduce the problem with accessibility running.

Comment 4

[Aaron Leventhal]

Evan, can you look at this?

Comment 5

[Evan Yan]

sure, I'll work on this.

Comment 6

[Evan Yan]

The cause:

When tabbing in the web page which "URI" points to, there will be a dynamically showing link, right before the top left "View demo" link. When you tabbed into that link, it shows dynamically; and when you tabbed out of it, the link will be hidden.

So when we tab quickly, the frame may have been destroyed when we access it.
It then will crash at
http://mxr.mozilla.org/seamonkey/source/accessible/src/html/nsHyperTextAccessible.cpp#341
frame->GetContent() returns a pointer to illegal address.

Aaron,

How can we know whether a frame has been destroyed? Can it be checked by checking presShell === nsnull?

The frame is got from accessNode->GetFrame(), which has already checked presShell.

Comment 7

[Aaron Leventhal]

The thing is that text frames are cached by nsHTMLTextAccessible. See how GetFrame() is overridden in that method and we use mFrame. The mFrame is supposed to get set to nsnull in nsHTMLTextAccessible::Shutdown(). And the accessible should be shut down whenever the frame is destroyed.

One thing to test is this. If you remove the GetFrame() method in nsHTMLTextAccessible so that we no longer cache the frame for those accessibles, it will fall back on the base GetFrame() impl (which uses GetPrimaryFrameFor), does it still crash?

There is no test for a dangling pointer that we can use which isn't hacky. The right thing is to find out why the accessible is not getting shutdown if it no longer exists. Why are we still trying to use it?

Comment 8

[Evan Yan]

(In reply to comment #7)
> There is no test for a dangling pointer that we can use which isn't hacky. The
> right thing is to find out why the accessible is not getting shutdown if it no
> longer exists. Why are we still trying to use it?
> 
Can an accessible be notified and then shutdown when the frame is destroyed?
I mean, we may hold an accessible whose frame has gone.

Why we cache mFrame in some accessible? for performance reason?

Comment 9

[Aaron Leventhal]

(In reply to comment #8)
> Can an accessible be notified and then shutdown when the frame is destroyed?
> I mean, we may hold an accessible whose frame has gone.
It is. Layout notifies nsAccessibilityService::InvalidateSubtreeFor()
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#1601

> Why we cache mFrame in some accessible? for performance reason?
Yes, because GetPrimaryFrameFor() must add extra items to primary frame hash table whenever request comes in for a frame. Since we already have the frame when accessible is created it seems like extra work to do that. However, it may not really be a big improvement. Maybe getting rid of that optimization is okay. 

Comment 10

[Evan Yan]

(In reply to comment #7)
> One thing to test is this. If you remove the GetFrame() method in
> nsHTMLTextAccessible so that we no longer cache the frame for those
> accessibles, it will fall back on the base GetFrame() impl (which uses
> GetPrimaryFrameFor), does it still crash?
> 
it won't crash any more.

(In reply to comment #9)
> It is. Layout notifies nsAccessibilityService::InvalidateSubtreeFor()
> http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#1601
> 
nsAccessibilityService::InvalidateSubtreeFor() is called by nsFrameManager::ReResolveStyleContext().
I added some prints in nsFrame::Destroy(), nsFrameManager::ReResolveStyleContext(), and nsHyperTextAccessible::GetPosAndText(), to print the value of the frame.

I found that, the frame which caused crash has already been destroyed, but nsFrameManager::ReResolveStyleContext() is not called for it. So the accessible can't be notified about this.

Comment 11

[Evan Yan]

I'm not clear about the layout code. Does nsFrameManager::ReResolveStyleContext() guaranteed to be called before the frame is destroyed?

Comment 12

[Aaron Leventhal]

See bug 376924.

Comment 13

[Aaron Leventhal]

Does the patch in bug 376924 fix this?

Comment 14

[Evan Yan]

yes, it does.
should we dupe this bug to bug 376924?