1324460 - implement AOM get() method

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[alexander :surkov (:asurkov)]

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

Review of attachment 8819919 [details] [diff] [review]:
-----------------------------------------------------------------

it crashes, when get() is called:

#0	0x00000001162d2af9 in js::GetObjectCompartment(JSObject*) [inlined] at /Users/heh/mozilla/inbound/obj-x86_64-apple-darwin16.3.0/dist/include/jsfriendapi.h:675
#1	0x00000001162d2af2 in mozilla::dom::MaybeWrapObjectValue(JSContext*, JS::MutableHandle<JS::Value>) [inlined] at /Users/heh/mozilla/inbound/obj-x86_64-apple-darwin16.3.0/dist/include/mozilla/dom/BindingUtils.h:753
#2	0x00000001162d2ade in mozilla::dom::MaybeWrapObjectOrNullValue(JSContext*, JS::MutableHandle<JS::Value>) [inlined] at /Users/heh/mozilla/inbound/obj-x86_64-apple-darwin16.3.0/dist/include/mozilla/dom/BindingUtils.h:777
#3	0x00000001162d2aa4 in mozilla::dom::AccessibleNodeBinding::get(JSContext*, JS::Handle<JSObject*>, mozilla::dom::AccessibleNode*, JSJitMethodCallArgs const&) at /Users/heh/mozilla/inbound/obj-x86_64-apple-darwin16.3.0/dom/bindings/AccessibleNodeBinding.cpp:240
#4	0x0000000116f594b2 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) at /Users/heh/mozilla/inbound/dom/bindings/BindingUtils.cpp:2914

the 'obj' arg of JSObject* type in GetObjectCompartment is:

obj	JSObject *	0x1410c2388	0x00000001410c2388
group_	js::GCPtrObjectGroup	
js::WriteBarrieredBase<js::ObjectGroup *>	js::WriteBarrieredBase<js::ObjectGroup *>	
js::BarrieredBase<js::ObjectGroup *>	js::BarrieredBase<js::ObjectGroup *>	
value	js::ObjectGroup *	0x400000021	0x0000000400000021
clasp_	const js::Class *	NULL	
proto_	js::GCPtr<js::TaggedProto>	
compartment_	JSCompartment *	NULL	
flags_	js::ObjectGroupFlags	
addendum_	void *	NULL	
propertySet	js::ObjectGroup::Property **	NULL	

and it looks like it cannot be casted to shadow::Object*.

Olli, any ideas what I do wrong?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Does get() really need to return object? ?
Would it be possibly to return some dictionary or perhaps some union?
union of DOMString and some dictionary? The less we deal with raw JSAPI stuff outside JS engine, the better. And it usually makes also APIs easier to understand.


Hmm, isn't the issue that you have a string, and not an object, and you do jsval.toObject() with such value.
If you can't use webidl unions or such, perhaps return 'any' in webidl. That maps to JS::Value in C++, and returning string becomes possible.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Does get() really need to return object? ?
> Would it be possibly to return some dictionary or perhaps some union?
> union of DOMString and some dictionary?
> The less we deal with raw JSAPI
> stuff outside JS engine, the better. And it usually makes also APIs easier
> to understand.
> 

I think the primitive types as strings and numbers are expected, at least I can't think of a real use case where a real objects like DOMNode or something would be needed, but on the other hand it probably shouldn't be forbidden. The idea is to allow the web author to store data in attributes, and then retrieve it back. IAccessible2 API has similar concept allowing to return a VARIANT as attribute value (http://git.linuxfoundation.org/?p=a11y/ia2.git;a=blob;f=api/Accessible2_2.idl;h=e90c2a348d48828488ce7e657d82ee446c598f7f;hb=6903f01e8164bcbb9a5ae8eed033bdff631d754d#l83)

> 
> Hmm, isn't the issue that you have a string, and not an object, and you do
> jsval.toObject() with such value.

not sure, it seems there are examples where string is converted to js object, https://dxr.mozilla.org/mozilla-central/source/dom/animation/KeyframeEffectReadOnly.cpp#1044

> If you can't use webidl unions or such, perhaps return 'any' in webidl. That
> maps to JS::Value in C++, and returning string becomes possible.

I guess I could go with DOMString simply for now and reveal that later, since it would work at least until we are getting close to 'set' part. However if 'any' makes a trick, then I would stick with it I think, as it's closer to the original proposal.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to alexander :surkov from comment #3)
 
> not sure, it seems there are examples where string is converted to js
> object,
> https://dxr.mozilla.org/mozilla-central/source/dom/animation/
> KeyframeEffectReadOnly.cpp#1044

That doesn't convert string to object. It converts a C++ string to JS::Value and stores the result in a property.
No toObject() involved.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

So, if you expect various JS primitives to be returned, use 'any', not 'object'

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

>diff --git a/accessible/aom/AccessibleNode.cpp b/accessible/aom/AccessibleNode.cpp
>--- a/accessible/aom/AccessibleNode.cpp
>+++ b/accessible/aom/AccessibleNode.cpp
>@@ -2,16 +2,17 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "AccessibleNode.h"
> #include "mozilla/dom/AccessibleNodeBinding.h"
> #include "mozilla/dom/BindingDeclarations.h"
> #include "mozilla/dom/DOMStringList.h"
>+#include "nsIPersistentProperties2.h"
> 
> #include "Accessible-inl.h"
> #include "nsAccessibilityService.h"
> #include "DocAccessible.h"
> 
> using namespace mozilla;
> using namespace mozilla::a11y;
> using namespace mozilla::dom;
>@@ -98,13 +99,29 @@ AccessibleNode::Is(const Sequence<nsStri
>   for (const auto& flavor : aFlavors) {
>     if (!flavor.Equals(role) && !mStates->Contains(flavor)) {
>       return false;
>     }
>   }
>   return true;
> }
> 
>+void
>+AccessibleNode::Get(JSContext* aCX, const nsAString& aAttribute,
>+                    JS::MutableHandle<JS::Value> aValue)
>+{
>+  if (!mIntl) {
>+    return;
I think the get() in .webidl should have [Throws], add ErrorResult& aRv argument and then here call
aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); and return


>+  }
>+  nsCOMPtr<nsIPersistentProperties> attrs = mIntl->Attributes();
>+  nsAutoString value;
>+  attrs->GetStringProperty(NS_ConvertUTF16toUTF8(aAttribute), value);
>+  JS::Rooted<JS::Value> jsval(aCX);
>+  if (ToJSValue(aCX, value, &jsval)) {
>+    aValue.set(jsval);
>+  }
If ToJSValue() fails, aRv should throws, so perhaps
else {
  aRv.Throws(NS_ERROR_UNEXPECTED);
}


>+++ b/dom/webidl/AccessibleNode.webidl
>@@ -7,9 +7,10 @@
> [Pref="accessibility.AOM.enabled"]
> interface AccessibleNode {
>   readonly attribute DOMString role;
>   [Frozen, Cached, Pure]
>   readonly attribute sequence<DOMString> states;
>   readonly attribute Node? DOMNode;
> 
>   boolean is(DOMString... states);
>+  any get(DOMString attribute);
So, add [Throws] before the method.

If my explanation isn't clear enough, happy to do a new review.
But those changes done, r+

Comment 8

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16e05131fbeb

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

+  JS::Rooted<JS::Value> jsval(aCX);
+  if (!ToJSValue(aCX, value, &jsval)) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+  }
+
+  aValue.set(jsval);
That looks wrong. Why you set aValue to jsval even after aRv.Throw()?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d426fe6431f0
implement AOM get() method, r=smaug

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failure of test aom/test_general.html on first push after it landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/da6f3eb57c7800df35868f3a52bb04a0caccf97e

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40841998&repo=mozilla-inbound

[task 2016-12-20T22:28:50.040111Z] 22:28:50     INFO - TEST-START | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html
[task 2016-12-20T22:28:50.055926Z] 22:28:50     INFO - ++DOMWINDOW == 55 (0x7f1d044bb800) [pid = 1237] [serial = 55] [outer = 0x7f1d0542b000]
[task 2016-12-20T22:28:50.153193Z] 22:28:50     INFO - ++DOCSHELL 0x7f1d04687000 == 11 [pid = 1237] [id = {4e64cfda-6ed7-4308-981e-83af4c420d11}]
[task 2016-12-20T22:28:50.153355Z] 22:28:50     INFO - ++DOMWINDOW == 56 (0x7f1d04687800) [pid = 1237] [serial = 56] [outer = (nil)]
[task 2016-12-20T22:28:50.155700Z] 22:28:50     INFO - ++DOMWINDOW == 57 (0x7f1d04688000) [pid = 1237] [serial = 57] [outer = 0x7f1d04687800]
[task 2016-12-20T22:28:50.217292Z] 22:28:50     INFO - TEST-INFO | started process screentopng
[task 2016-12-20T22:28:50.690578Z] 22:28:50     INFO - TEST-INFO | screentopng: exit 0
[task 2016-12-20T22:28:50.693908Z] 22:28:50     INFO - Buffered messages logged at 22:28:50
[task 2016-12-20T22:28:50.695664Z] 22:28:50     INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | DOM document has accessible node 
[task 2016-12-20T22:28:50.695723Z] 22:28:50     INFO - Buffered messages finished
[task 2016-12-20T22:28:50.697836Z] 22:28:50     INFO - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | correct role of a document accessible node - got "application", expected "document"
[task 2016-12-20T22:28:50.697960Z] 22:28:50     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:271:5
[task 2016-12-20T22:28:50.698057Z] 22:28:50     INFO - checkImplementation@chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html:49:5
[task 2016-12-20T22:28:50.698345Z] 22:28:50     INFO - promise callback*@chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html:15:3
[task 2016-12-20T22:28:50.699642Z] 22:28:50     INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | correct DOM Node of a document accessible node 
[task 2016-12-20T22:28:50.700789Z] 22:28:50     INFO - Not taking screenshot here: see the one that was previously logged
[task 2016-12-20T22:28:50.703240Z] 22:28:50     INFO - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | readonly state is expected at 0th index - got "busy", expected "readonly"
[task 2016-12-20T22:28:50.703381Z] 22:28:50     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:271:5
[task 2016-12-20T22:28:50.703708Z] 22:28:50     INFO - checkImplementation@chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html:76:9
[task 2016-12-20T22:28:50.703904Z] 22:28:50     INFO - promise callback*@chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html:15:3
[task 2016-12-20T22:28:50.704013Z] 22:28:50     INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | focusable state is expected at 1th index 
[task 2016-12-20T22:28:50.704809Z] 22:28:50     INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | opaque state is expected at 2th index 
[task 2016-12-20T22:28:50.705788Z] 22:28:50     INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | enabled state is expected at 3th index 
[task 2016-12-20T22:28:50.706838Z] 22:28:50     INFO - TEST-PASS | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | sensitive state is expected at 4th index 
[task 2016-12-20T22:28:50.707754Z] 22:28:50     INFO - Not taking screenshot here: see the one that was previously logged
[task 2016-12-20T22:28:50.708835Z] 22:28:50     INFO - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html | correct role and state on an accessible node 
[task 2016-12-20T22:28:50.709484Z] 22:28:50     INFO - checkImplementation@chrome://mochitests/content/a11y/accessible/tests/mochitest/aom/test_general.html:80:5

Comment 12

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30a2bc507707

Comment 13

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fea620fed80a

Comment 14

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab01590fdbd8

Comment 15

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0719102099c09ee924b0e4eeda362ab0b9f37acf
 Bug 1324460 - implement AOM get() method, r=smaug

Comment 16

[alexander :surkov (:asurkov)]

relanded, should be good now

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0719102099c0