1245650 - Consider removing JavaScript-navigator-property

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1090382 comment 5: the only consumer we know of now is a single extension which is also using it to return an API object to the page that uses __exposedProps__ and the like...  The extension in question is https://addons.mozilla.org/en-US/firefox/addon/nulltxt/?src=search

The question is whether we can give this extension some other way to do what it's doing.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

The nulltxt extension hasn't been updated since 2013. We can't really justify keeping all this code for that one use case imo.

This adds partial Navigator interfaces with an attribute per NavigatorProperty in the parser. I tried doing this just in Codegen.py, but it looks fairly hard (everything expects an IDLAttribute and creating one from scratch correctly isn't trivial). Let me know if you have a better idea.

This removes support for dom.navigator-property.disable.*. I have a separate patch that adds it back in the new world, but I'm not sure we should use it. It looks pretty weird that we support this on some navigator properties but not others. For example there's an addon that tries to do "dom.navigator-property.disable.userAgent", which we have never supported afaict.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Keep support for dom.navigator-property.disable.* v1

As I said, I don't think we should keep this. Feel free to r- if you agree.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 8719529 [details] [diff] [review]
v1

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

Need to figure out a crash first.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8719532 [details] [diff] [review]
Keep support for dom.navigator-property.disable.* v1

I thought this was added for Tor or something, but looks like it was just added for bug 863704.

Given that we have a way of doing that in Web IDL already via Pref annotations, I don't think we should add more complexity here.  I did check over addons MXR and none of them are doing anything that looks like it would be seriously broken by nixing this stuff.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.1

I had to clean up the meaning of the static argument to CGPerSignatureCall.__init__ a bit, and move out the changes we make to a the method name to the callers, so that CGNavigatorGetterCall works correctly. CGNavigatorGetterCall wants to call a static method, but for a normal property. Before those changes passing True for the static argument would also assume that the getter was for a static property.

I've also made the SettingsManager interface global property exposure obey settings-api-read|write permissions. We used to only make the navigator property obey those, but doing one without the other doesn't really make much sense imo (the interface has no static methods or properties and the only way to get an instance of it is through the navigator property).

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 8724124 [details] [diff] [review]
v1.1

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

Hmmm, that's why you didn't review the new patch yet, I forgot to ask :-/.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8724124 [details] [diff] [review]
v1.1

>-        if not self.descriptor.interface.isJSImplemented():
>-            raise TypeError("Only JS-implemented classes are currently supported "
>-                            "on navigator. See bug 856820.")

This check and exception got lost?

>+    A class to generate a native object getter call for a an IDL getter for a

s/a an/an/

>+                    self.extendedAttributes['all'].setdefault(m.identifier.name, []).append('implicitJSContext')

This bit could use a comment explaining why.

>+                partialInterface = IDLPartialInterface(iface.location,

The could use documentation too, since you're not _doing_ anything with "partialInterface", just relying on the side-effects from the constructor.

Would it make sense to add the IDLPartialInterface to self._productions, just to make it simpler to see that this is really ok?

I'm not super-happy about having this interface-specific hack in here, but I guess I can live with it.

>+    document.body.removeChild(iframe);

  iframe.remove();

r=me, but I would like to see the generated NavigatorBinding with this patch; I tried to just do that myself, but it doesn't apply on tip for me...

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: NavigatorBinding.cpp

Example for mozAlarms:

From AlarmsManagerBinding.cpp:

already_AddRefed<AlarmsManager>
ConstructNavigatorObject(JSContext* cx, JS::Handle<JSObject*> obj, ErrorResult& aRv)
{
  GlobalObject global(cx, obj);
  if (global.Failed()) {
    aRv.Throw(NS_ERROR_FAILURE);
    return nullptr;
  }
  JS::Rooted<JSObject*> jsImplObj(cx);
  nsCOMPtr<nsIGlobalObject> globalHolder =
    ConstructJSImplementation("@mozilla.org/alarmsManager;1", global, &jsImplObj, aRv);
  if (aRv.Failed()) {
    return nullptr;
  }
  // Build the C++ implementation.
  RefPtr<AlarmsManager> impl = new AlarmsManager(jsImplObj, globalHolder);
  return impl.forget();
}

From NavigatorBinding.cpp:

static bool
get_mozAlarms(JSContext* cx, JS::Handle<JSObject*> obj, mozilla::dom::Navigator* self, JSJitGetterCallArgs args)
{
  // Have to either root across the getter call or reget after.
  JS::Rooted<JSObject*> reflector(cx);
  // Safe to do an unchecked unwrap, since we've gotten this far.
  // Also make sure to unwrap outer windows, since we want the
  // real DOM object.
  reflector = IsDOMObject(obj) ? obj : js::UncheckedUnwrap(obj, /* stopAtWindowProxy = */ false);
  {
    // Scope for cachedVal
    JS::Value cachedVal = js::GetReservedSlot(reflector, (DOM_INSTANCE_RESERVED_SLOTS + 4));
    if (!cachedVal.isUndefined()) {
      args.rval().set(cachedVal);
      // The cached value is in the compartment of reflector,
      // so wrap into the caller compartment as needed.
      return MaybeWrapValue(cx, args.rval());
    }
  }

  ErrorResult rv;
  auto result(StrongOrRawPtr<mozilla::dom::AlarmsManager>(AlarmsManagerBinding::ConstructNavigatorObject(cx, reflector, rv)));
  if (MOZ_UNLIKELY(rv.MaybeSetPendingException(cx))) {
    return false;
  }
  MOZ_ASSERT(!JS_IsExceptionPending(cx));
  { // Make sure we wrap and store in the slot in reflector's compartment
    JSAutoCompartment ac(cx, reflector);
    do { // block we break out of when done wrapping
      if (!GetOrCreateDOMReflector(cx, result, args.rval())) {
        MOZ_ASSERT(true || JS_IsExceptionPending(cx));
        return false;
      }
      break;
    } while (0);
    js::SetReservedSlot(reflector, (DOM_INSTANCE_RESERVED_SLOTS + 4), args.rval());
    PreserveWrapper(self);
  }
  // And now make sure args.rval() is in the caller compartment
  return MaybeWrapValue(cx, args.rval());
}

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8736309 [details]
NavigatorBinding.cpp

That looks great, thanks!

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v1.1 part 2

Sigh, sorry for the followup review request but there was a failing test for mozAlarms on Android. Apparently we are exposing AlarmsManager but not Navigator.mozAlarms on Android (by not packaging the JS component for it). We really shouldn't do stuff like that, so I opted for not exposing AlarmsManager either.

While debugging this I discovered that the generated IsFeatureDetectible code for Navigator.hasFeature contained strings like

    "Navigator.'<unresolved scope>::AlarmsManager' attribute '<unresolved scope>::mozAlarms'",

in addition to

    "Navigator.mozAlarms",

This is fixed by removing the special-cased code for Navigator in dom/bindings/Configuration.py (getNavigatorProperty now returns a IDLAttribute instead of just the property name). Since we now generate normal properties we can just rely on the existing code for those instead of special-casing Navigator.

And I removed the code for Navigator.mozContacts from Navigator::HasFeature, since it's already generated in IsFeatureDetectible.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8737234 [details] [diff] [review]
v1.1 part 2

r=me

Comment 12

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce2b18491b16