112494 - crash in nsAccessibilityService::CreateIFrameAccessible

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Robert Ginda]

Another null pointer dereference in accessible code, triggered by opening the
window object in the venkman object inspector.  See also bug 103903 and bug 110620.

I spoke with jgaunt about smoketesting future accessible changes with the
debugger, I'd really appreciate it if ya'll would do that.  These types of
crashes reflect bad on the debugger, because it's typically the first thing new
users try.  When it crashes on them this easily, they get turned off to the
debugger and go back to dump() statements, leaving me without a job.

The smoketest procedure is simple...
1. Start mozilla with -venkman.
2. Launch navigator from the venkman tasks menu.
3. Click the stop button (a red X) in the venkman toolbar.
4. Focus navigator, venkman should react by stopping execution.
5. Open the top stack frame and the "this" object, which should be of type Window.
6. If you don't crash, everything is OK, otherwise, something is wrong.

The deal here is that the accessibility getter in browser.xml is calling out to
CreateIFrameAccessible, which crashes.  It is possible that the above test could
be reduced to a xul file which fetches |window.accessible|, though I havn't tested.

In general, any scriptable method, such as CreateIFrameAccessible, should to be
robust enough to deal with bad parameters without crashing.  If the caller
passes null, or an unexpected object, the method should throw a meaningful
error, instead of assert and crash.  Asserts are best left for conditions that
"can't" happen.  When you're scriptable, anything can happen (as far as
parameters are concerned.)

Comment 1

[Robert Ginda]

Attachment: proposed patch

Comment 2

[John Gaunt (redfive)]

Comment on attachment 59593 [details] [diff] [review]
proposed patch

r=jgaunt

Comment 3

[John Gaunt (redfive)]

adding brendan for sr=

This is horking the js debugger, trying to get quick turnaround.

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 59593 [details] [diff] [review]
proposed patch

Blech on NS_WARN_IF_FALSE, but I'll assent nonetheless: sr=shaver.

Long live venkman!

Comment 5

[Robert Ginda]

Attachment: new proposed patch

Sorry, that last patch was stale when I posted it.  This new one adds a null
pointer check for the argument too.

Comment 6

[John Gaunt (redfive)]

Comment on attachment 59626 [details] [diff] [review]
new proposed patch

r=jgaunt

still looks good to me

Comment 7

[Robert Ginda]

checked in.