707564 - force the properties onto navigator in the enumerate hook

Status: RESOLVED FIXED

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

Description

[[:fabrice] Fabrice Desré]

navigator properties that are added with the JavaScript-navigator-property category should be enumerated on the navigator object

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Fix TCPSocket initialization to not throw exceptions on non-b2g.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a way to enumerate navigator names on the script namespace manager.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Teach WebIDL Xrays about the GetOwnProperties methods on WebIDL objects that have NewResolve.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ce48e9675973

Comment 6

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

Comment on attachment 783666 [details] [diff] [review]
part 3.  Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties.

I don't understand this.
Why do we want those properties to be own properties?
Shouldn't they be getters(/setters?) in prototype and that way enumerable?

Clearing r? for now.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 783666 [details] [diff] [review]
part 3.  Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties.

We just want to be consistent.  The resolve hook resolves these as own properties, so the enumerate hook should enumerate them.

Long-term we do want the navigator category stuff to be on the proto somehow, but we just haven't figured out how.  At the point at which we manage to do that, we'll almost certainly no longer need this code or a resolve hook at all: setting up Navigator.prototype would just enumerate the prop names and set up accessors.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 783667 [details] [diff] [review]
part 4.  Teach WebIDL Xrays about the GetOwnProperties methods on WebIDL objects that have NewResolve.

Again, this is just being consistent with the resolve hook's behavior.

Comment 9

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

Thanks for explaining. I'll try to review these still today.

Comment 10

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

Comment on attachment 783666 [details] [diff] [review]
part 3.  Give WebIDL bindings with NewResolve hooks Enumerate hooks as well, so enumerating them correctly resolves all the properties.

nsScriptNameSpaceManager *nameSpaceManager ->
nsScriptNameSpaceManager* nameSpaceManager

In the test could you try also for-in.

Comment 11

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

Comment on attachment 783667 [details] [diff] [review]
part 4.  Teach WebIDL Xrays about the GetOwnProperties methods on WebIDL objects that have NewResolve.


>+class CGEnumerateOwnPropertiesViaGetOwnPropertyNames(CGAbstractBindingMethod):
>+    """
>+    An implementation of Xray EnumerateOwnProperties stuff for things
>+    that have a newresolve hook.
>+    """
>+    def __init__(self, descriptor):
>+        args = [Argument('JSContext*', 'cx'),
>+                Argument('JS::Handle<JSObject*>', 'wrapper'),
>+                Argument('JS::Handle<JSObject*>', 'obj'),
>+                Argument('JS::AutoIdVector&', 'props')]
Not about this bug, but the generated code, and I believe even the codegen itself would be
easier to read if aFoo style was used for parameters.

>+        CGAbstractBindingMethod.__init__(self, descriptor,
>+                                         "EnumerateOwnPropertiesViaGetOwnPropertyNames",
>+                                         args, getThisObj="",
>+                                         callArgs="", returnType="bool")
>+    def generate_code(self):
>+        return CGIndenter(CGGeneric(
>+                "nsAutoTArray<nsString, 8> names;\n"
>+                "ErrorResult rv;\n"
>+                "self->GetOwnPropertyNames(cx, names, rv);\n"
>+                "rv.WouldReportJSException();\n"
>+                "if (rv.Failed()) {\n"
>+                '  return ThrowMethodFailedWithDetails<true>(cx, rv, "%s", "enumerate");\n'
>+                "}\n"
>+                '// OK to pass "obj" as "proxy" because it\'s ignored if\n'
>+                "// shadowPrototypeProperties is true\n"
>+                "return DOMProxyHandler::AppendNamedPropertyIds(cx, JS::NullPtr(), names, true, nullptr, props);"))

well, you don't pass obj, and nullptr makes more sense anyway. Remove the comment.

>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=707564">Mozilla Bug 707564</a>
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+<iframe id="t" src="http://example.org/tests/dom/bindings/test/file_bug707564.html"></iframe>
>+</div>
>+<pre id="test">
>+<script type="application/javascript">
>+
>+/** Test for Bug 775543 **/
>+function test()
>+{
>+  var props = Object.getOwnPropertyNames(XPCNativeWrapper(document.getElementById("t").contentWindow.navigator));
>+  isnot(props.indexOf("mozApps"), -1,
>+        "Should enumerate a mozApps property on navigator xray");
Could you test for-in, and perhaps also some expando property.



I need to still understand how CGEnumerateOwnPropertiesViaGetOwnPropertyNames is actually about XRay

Comment 12

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

Ah, http://mxr.mozilla.org/mozilla-central/source/dom/bindings/DOMJSClass.h#126 helps

Comment 13

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

Comment on attachment 783667 [details] [diff] [review]
part 4.  Teach WebIDL Xrays about the GetOwnProperties methods on WebIDL objects that have NewResolve.

Please file a bug about making Navigator stuff to use prototype.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> In the test could you try also for-in.

Will do.

> the generated code, and I believe even the codegen itself would be
> easier to read if aFoo style was used for parameters.

Let's take that up with peterv when he gets back.  ;)

> well, you don't pass obj, and nullptr makes more sense anyway. Remove the comment.

Done; it was left over from a previous patch version.

> Could you test for-in, and perhaps also some expando property.

I can test for/in.  For the expando bit you mean test that it's not present?

Comment 15

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/ddad646a1d86
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e13fb5ddfae
   https://hg.mozilla.org/integration/mozilla-inbound/rev/db968c9c8831
   https://hg.mozilla.org/integration/mozilla-inbound/rev/54e8477f4415

Comment 16

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/aab25110bbc3 to deal with jsapi merge bustage.

Comment 17

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/0868beaeab71 to deal with push notifications code not following the JS-implemented webidl contract.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ddad646a1d86
https://hg.mozilla.org/mozilla-central/rev/5e13fb5ddfae
https://hg.mozilla.org/mozilla-central/rev/db968c9c8831
https://hg.mozilla.org/mozilla-central/rev/54e8477f4415
https://hg.mozilla.org/mozilla-central/rev/aab25110bbc3
https://hg.mozilla.org/mozilla-central/rev/0868beaeab71