388665 - Javascript interferes with STATE_EDITABLE effective 5th July build

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: test case

Steps to reproduce:

1. Open the attached test case and launch Accerciser
2. Examine each entry using Accerciser's Interface viewer

Expected results:  Each of the four entries would have "editable" among their states.

Actual results:  With the 5th July trunk build (14), the second entry lacks this state.  Beginning with the 6th July trunk build, the first and the second entry lack this state.

For some reason, the presence of certain Javascript scripts associated with a form can prevent editable entries and password text in that form from exposing STATE_EDITABLE.

Comment 1

[Ginn Chen]

I think it's caused by the contentEditable changes.

The first textfield get focused by javascript, so we creates accessible for first 2 textfields immediately after the page loaded.
GetEditor fails at that time for some reason.
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLFormControlAccessible.cpp#561

Comment 2

[Ginn Chen]

We hit NS_ERROR_DOM_SECURITY_ERR

http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#3841

Comment 3

[Ginn Chen]

interesting,

the regression of first text field happened between July 3 to July 4
the regression of second text field happened between July 5 to July 6

Comment 4

[Ginn Chen]

actually I think it's a little random.

Comment 5

[Ginn Chen]

The summary of this bug is very nice!

javascript focus() caused nsCaretAccesible::NotifySelectionChanged be called, and then we creates accessible objects for the textfields.
because the js is on JSStack, we're denied to get the editor.

Is it a false denial?

Comment 6

[Boris Zbarsky [:bzbarsky]]

If there's untrusted JS on the stack, the denial is correct.  You probably want to push a null JSContext on the stack before doing the GetEditor() call.

Not sure why contentEditable would affect this.

Comment 7

[Ginn Chen]

Maybe contentEditable is not related to this.

Comment 8

[Ginn Chen]

Attachment: patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273538 [details] [diff] [review]
patch

>Index: nsHTMLFormControlAccessible.cpp
>+  // Override UniversalBrowserWrite capability by pushing a null jscontext 

That's not what you're doing.  You're making sure that you're not restricted by the permissions of whatever script is currently running.

>+    stack->Push(nsnull);

That call can fail, and if it does you need to deal with that.  You do NOT want to pop if pushing failed.

Comment 10

[Ginn Chen]

Attachment: patch v2

Looks better now?

BTW: the last patch is copied from extensions/webservices/security/src/nsWebScriptsAccess.cpp#767
which is an example in http://wiki.mozilla.org/Breaking_the_grip_JS_has_on_the_DOM

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273549 [details] [diff] [review]
patch v2

Much better, thank you!

Comment 12

[Aaron Leventhal]

Comment on attachment 273549 [details] [diff] [review]
patch v2

Ginn, can you provide a more complete description of what is going on? I don't understand it. Why is JS involved here at all? Please add the description in the comments when you check in.

Comment 13

[Aaron Leventhal]

One thing I want to understand is why we don't have this problem in other areas of mozilla/accessible, not just when dealing with the editor, but for any calls we make.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Most calls you make probably don't do explicit security checks like GetEditor() does.

Comment 15

[alexander :surkov (:asurkov)]

Theoretically we can get similar things everywhere where untrusted js is a cause of events we handle if I get right.

Comment 16

[Ginn Chen]

I guess there're somewhere else in mozilla/accessible need a similar patch,
I think we can find out later.

Comment 17

[Ginn Chen]

committed