Closed Bug 1245650 Opened 4 years ago Closed 4 years ago

Consider removing JavaScript-navigator-property

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: peterv)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #8719529 - Flags: review?(bzbarsky)
As I said, I don't think we should keep this. Feel free to r- if you agree.
Attachment #8719532 - Flags: review?(bzbarsky)
Comment on attachment 8719529 [details] [diff] [review]
v1

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

Need to figure out a crash first.
Attachment #8719529 - Flags: review?(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.
Attachment #8719532 - Flags: review?(bzbarsky) → review-
Attached patch v1.1Splinter Review
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).
Attachment #8719529 - Attachment is obsolete: true
Attachment #8719532 - Attachment is obsolete: true
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 :-/.
Attachment #8724124 - Flags: review?(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...
Flags: needinfo?(peterv)
Attachment #8724124 - Flags: review?(bzbarsky) → review+
Attached file 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());
}
Flags: needinfo?(peterv)
Attachment #8736309 - Flags: feedback?(bzbarsky)
Comment on attachment 8736309 [details]
NavigatorBinding.cpp

That looks great, thanks!
Attachment #8736309 - Flags: feedback?(bzbarsky) → feedback+
Attached patch v1.1 part 2Splinter Review
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.
Attachment #8737234 - Flags: review?(bzbarsky)
Comment on attachment 8737234 [details] [diff] [review]
v1.1 part 2

r=me
Attachment #8737234 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/ce2b18491b16
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.