838146 - Convert Navigator to WebIDL

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I can't think of a reason not to, and it would make a lot of the conditional stuff it does right now simpler, I think.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

How about the LookupNavigatorName stuff?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  Could those not become prefcontrolled normal properties on navigator?

Comment 3

[Justin Lebar (not reading bugmail)]

I'm not sure what you guys are referring to; perhaps you meant to cc someone else?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm not sure who the right person to cc is.

What we're talking about are all the things in http://mxr.mozilla.org/mozilla-central/search?string=JavaScript-navigator-property&find=manifest and how we determine whether they're exposed to web content or not.

Comment 5

[Justin Lebar (not reading bugmail)]

Ah, I see.  Thanks for clarifying.

I'm still not sure I have something interesting to contribute here; I don't see why we couldn't control those properties by prefs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That counts as an interesting contribution in my book.  If there's no obvious reason it can't work, then we should just try it.  ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm going to poke at this....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 765171 [details] [diff] [review]
Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.

This is for bug 885171.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: WIP, just so it's backed up

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Implement WebIDL API on Navigator for the parts that are specified or are in nsIDOMNavigator.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Implement WebIDL API on Navigator for all our nonstandard extensions that are not behind build-time flags.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Set up preprocessing of Navigator.webidl, since we want to make some stuff on Navigator build-time-conditional.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to remove some cruft

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 now including GLOBAL_DEPS

Comment 16

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

Comment on attachment 770593 [details] [diff] [review]
part 1.  Implement WebIDL API on Navigator for the parts that are specified or are in nsIDOMNavigator.


>@@ -714,53 +756,79 @@ Navigator::AddIdleObserver(nsIIdleObserv
>   }
> 
>   NS_ENSURE_ARG_POINTER(aIdleObserver);
> 
>   if (!CheckPermission("idle")) {
>     return NS_ERROR_DOM_SECURITY_ERR;
>   }
> 
>-  if (NS_FAILED(mWindow->RegisterIdleObserver(aIdleObserver))) {
>+  AddIdleObserver(*aIdleObserver);
>+  return NS_OK;
>+}
>+
>+void
>+Navigator::AddIdleObserver(nsIIdleObserver& aIdleObserver)
>+{
>+  if (NS_FAILED(mWindow->RegisterIdleObserver(&aIdleObserver))) {
>     NS_WARNING("Failed to add idle observer.");
>   }
>+}
> 
>-  return NS_OK;
>+void
>+Navigator::AddIdleObserver(MozIdleObserver& aIdleObserver, ErrorResult& aRv)
>+{
>+  if (!mWindow) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return;
>+  }
>+  CallbackObjectHolder<MozIdleObserver, nsIIdleObserver> holder(&aIdleObserver);
>+  nsCOMPtr<nsIIdleObserver> obs = holder.ToXPCOMCallback();
>+  return AddIdleObserver(*obs);
> }

I think HasIdleSupport should be somewhere close here. Same thing with other Has*
And perhaps also some comment that it is used for the webidl case.
The setup is a bit error prone. C++ caller gets different results depending on the
which version of the method it is calling. Could we at least add
MOZ_ASSERT to the methods for webidl that permission is enabled.

Also, some b2g dev should comment whether the permission may change



>+  if (aPattern.Length() > sMaxVibrateListLen) {
>+    // XXXbz this should be returning false instead
>+    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>+    return;
>+  }
>+
>+  for (size_t i = 0; i < aPattern.Length(); ++i) {
>+    if (aPattern[i] > sMaxVibrateMS) {
>+      // XXXbz this should be returning false instead
>+      aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>+      return;
>+    }
>+  }
Could you say in XXX something like: per spec this should be returning false instead.
Perhaps add even the bug number 884935

>+'Navigator': {
>+    'register': False,
>+},
Er, why false? What is still missing? ...or perhaps some other patch enables it.

>+callback interface MozIdleObserver {
>+  // Time is in seconds and is read only when idle observers are added
>+  // and removed.
>+  readonly attribute unsigned long time;
>+  void onidle();
>+  void onactive();
>+};
ugh. UGH.

Comment 17

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

Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

In this patch too, the methods for WebIDL should have MOZ_ASSERT for
permission. Or at least NS_WARNING


>+Navigator::MozSetMessageHandler(const nsAString& aType,
>+                                systemMessageCallback* aCallback,
oddly named type. Should be capital S


>+// nsIDOMNavigatorDeviceStorage
>+partial interface Navigator {
>+  // XXXbz can this be hidden outside "certified apps"?
>+  // Or just controlled by "device.storage.enabled"?  sicking says yes.
Sicking says yes to what? to the first question? Perhaps ask dougt too, and if the answer to the
first one is yes, this could be changed. But I guess Pref is ok for now.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Could we at least add MOZ_ASSERT to the methods for webidl that permission is enabled.

That would actually not be a valid assert.  In the WebIDL setup if you grab the method from a window where the permission _is_ enabled and .call() it on an object that comes from a window where the permsission is _not_ enabled, it will actually work. I think this is perfectly ok, frankly.

> Also, some b2g dev should comment whether the permission may change

I was told no.

> Er, why false? What is still missing?

Well, part 2, and all the ifdeffed stuff and the resolve hook and actually using SetIsDOMBinding().  ;)  The register:False will go away in the same patch that adds the SetIsDOMBinding() call, when all the rest of this stuff is done.

> ugh. UGH.

Yeah, indeed.  Maybe we should file a bug on that API.  :(

> oddly named type.

That's what the spec, such as it is, calls it.  I agree the spec is a bit odd; take that up with Mounir?

> Sicking says yes to what?

Er, sicking says yes to just having the property not be there based on the conditions that would cause it to fail in today's world (which is when the pref is unset).  I removed that comment; it was a note to myself to make sure to check how stuff should work here....

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 with better non-clobber build handling

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I think HasIdleSupport should be somewhere close here. Same thing with other Has*

Hmm.  Just in the .cpp or in the header too?  I was trying to put actual implementations of WebIDL stuff together and the miscellaneous helpers separately from those....

Comment 21

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

Or just add a comment to the methods which webidl uses that they rely on the pref check in webidl.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Implement WebIDL API on Navigator for the WebTelephony.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Implement remaining MOZ_B2G_RIL-conditional WebIDL APIs on Navigator.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Implement gamepad, bluetooth, time, audiochannel WebIDL APIs on Navigator.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Implement the MOZ_MEDIA_NAVIGATOR WebIDL APIs on Navigator.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Switch the Navigator resolve hook over to a more WebIDL-like API.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Turn on WebIDL bindings for Navigator.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 9 now with some test fixes to deal with the new setup

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 9 with more test changes

Comment 30

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

Comment on attachment 773275 [details] [diff] [review]
part 8.  Switch the Navigator resolve hook over to a more WebIDL-like API.

>+Navigator::DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
>+                        JS::Handle<jsid> aId, unsigned aFlags,
>+                        JS::MutableHandle<JSObject*> aObjp)
>+{
>+  if (!JSID_IS_STRING(aId)) {
>+    return true;
>+  }
>+
>+  nsScriptNameSpaceManager *nameSpaceManager =
nsScriptNameSpaceManager* nameSpaceManager


So this was mostly just moving code.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> So this was mostly just moving code.

Yes, exactly.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 770593 [details] [diff] [review]
part 1.  Implement WebIDL API on Navigator for the parts that are specified or are in nsIDOMNavigator.

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

sr=me on Navigator.webidl

Comment 33

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

Comment on attachment 773472 [details] [diff] [review]
Part 9 with more test changes

>+JSObject*
>+Navigator::WrapObject(JSContext* cx, JS::Handle<JSObject*> scope)
aCx, aScope

>+  virtual JSObject* WrapObject(JSContext* cx,
>+                               JS::Handle<JSObject*> scope) MOZ_OVERRIDE;
ditto



r+ assuming jonas is ok with the permission handling changes (which are in the most of the patches).

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 773272 [details] [diff] [review]
part 5.  Implement remaining MOZ_B2G_RIL-conditional WebIDL APIs on Navigator.

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

::: dom/base/Navigator.cpp
@@ +2058,5 @@
> +                                      JSObject* aGlobal)
> +{
> +  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> +  return win && CheckPermission(win, "mobileconnection") &&
> +    CheckPermission(win, "mobilenetwork");

This should be

win && (CheckPermission("mobileconnection") || CheckPermission("mobilenetwork))

Comment 35

[Boris Zbarsky [:bzbarsky]]

> win && (CheckPermission("mobileconnection") || CheckPermission("mobilenetwork))

Oh, good catch!  It worries me a bit that this passed tests.  ;)

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Oh, good catch!  It worries me a bit that this passed tests.  ;)

Good news: this does in fact cause one test failure on B2G, in test_mobileconnection.html.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

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

Getting deviceStorage is definitely a privileged thing. Ping me on irc if you need help with finding the security checks.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Getting deviceStorage is definitely a privileged thing.

In a vanilla trunk build on Mac, if I set the "device.storage.enabled" preference to true, navigator.getDeviceStorage("videos") in a web page returns a DeviceStorage object.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

Per comment 39....

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 772243 [details] [diff] [review]
part 4.  Implement WebIDL API on Navigator for the WebTelephony.

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

This looks fine but I think it should change now that we decided not to do the tri-state (present, null, or valid value):

::: dom/base/Navigator.cpp
@@ +1443,2 @@
>      NS_ENSURE_STATE(mWindow);
> +    if (!telephony::Telephony::CheckPermission(mWindow)) {

Now that we don't define the property unless permission is granted is there any reason not to just assert this?

@@ +1452,5 @@
> +  return rv.ErrorCode();
> +}
> +
> +nsIDOMTelephony*
> +Navigator::GetMozTelephony(ErrorResult& aRv)

Hm, I don't see the benefit of this second method. Can't it just be combined into the other one?

@@ +1462,5 @@
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +    mTelephony = telephony::Telephony::Create(mWindow, aRv);
> +    // Note: mTelephony can be null here, even if aRv is success

This shouldn't be true any longer.

::: dom/telephony/Telephony.cpp
@@ +142,4 @@
>    telephony->mListener = new Listener(telephony);
>  
> +  // XXXbz Should failures in EnumerateCalls and RegisterTelephonyMsg
> +  // become exceptions instead of a null return?

Yes, definitely, and same for the other things that can fail above and below. We should communicate the error result if we can.

::: dom/telephony/Telephony.h
@@ +9,5 @@
>  
>  #include "TelephonyCommon.h"
> +// Need to include TelephonyCall.h because we have inline methods that
> +// assume they see the definition of TelephonyCall.
> +#include "TelephonyCall.h"

Wait, really? How is this not failing to compile right now?

@@ +58,5 @@
>                                                     Telephony,
>                                                     nsDOMEventTargetHelper)
>  
>    static already_AddRefed<Telephony>
> +    Create(nsPIDOMWindow* aOwner, ErrorResult& aRv);

Nit, no indent here. And add a newline after :)

Comment 42

[Boris Zbarsky [:bzbarsky]]

> Now that we don't define the property unless permission is granted is there any reason
> not to just assert this?

Yes.  We don't define the _webidl_ property, but the check you're looking at is in the _xpidl_ method.  I'll remove all this xpidl stuff in a followup, but wanted to make it easier to revert this change for now as needed.

> Hm, I don't see the benefit of this second method.

This is the one the WebIDL bindings call.

> This shouldn't be true any longer.

Given your comments about Create(), agreed.

> Yes, definitely, and same for the other things that can fail above and below.

Done.

> Wait, really? How is this not failing to compile right now?

The only places that include Telephony.h right now are TelephonyCall.cpp, Telephony.cpp, and nsDOMClassInfo.cpp.  All three also include TelephonyCall.h.

> Nit, no indent here. And add a newline after :)

Done.

Thank you for the review!

Comment 43

[Peter Van der Beken [:peterv]]

Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

sr+ing to unblock this. The patch maintains the status quo, we can come up with a better solution in a followup which bz will file.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Pushed:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/483fbc6878a8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc7f2167503
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b135e58350
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c3d84965cf
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f06b420e0dfa
   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3a62dd3910
   https://hg.mozilla.org/integration/mozilla-inbound/rev/7cddfebc261c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/fddccc5fce6e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/955a14e3c181

I filed bug 893003 on devicestorage bits and bug 893004 on removing the XPCOM bits from Navigator.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/33fb417caccf

https://tbpl.mozilla.org/php/getParsedLog.php?id=25244444&tree=Mozilla-Inbound

Comment 46

[Boris Zbarsky [:bzbarsky]]

Ugh.  There are orange Mn on my try push now too that weren't there when I last looked at it.  :(

I guess I need to fix those tests as well, because they do dynamic permissions stuff.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

(In reply to Boris Zbarsky (:bz) from comment #46)
> Ugh.  There are orange Mn on my try push now too that weren't there when I
> last looked at it.  :(

As a quick aside, can you please post a link to your last Try run when you push to inbound? It probably would have saved us a lot of time trying to track this down yesterday. Especially since B2G full-stack builds + tests have a roughly 2.5 hour turnaround time.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Xrays play nice with WebIDL resolve hooks

Comment 49

[Boris Zbarsky [:bzbarsky]]

Attachment: Xray+resolve, with an obvious bug fixed

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: Xray+newresolve, without a boolean outparam

Comment 51

[Bobby Holley (:bholley)]

Comment on attachment 775951 [details] [diff] [review]
Xray+newresolve, without a boolean outparam

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

I only superficial follow some of the mechanical Codegen changes, but it all looks reasonable and you know what you're doing. r=bholley

::: dom/base/nsDOMClassInfo.cpp
@@ +4660,5 @@
>  {
>    JS::Rooted<JSObject*> obj(cx, aObj);
>    JS::Rooted<jsid> id(cx, aId);
>    nsCOMPtr<nsIDOMNavigator> navigator = do_QueryWrappedNative(wrapper);
> +  JS::Rooted<JS::Value> value(cx);

Given the calling convention here, I think that per-EIBTI we should explicitly pass UndefinedValue() here, though I could be convinced otherwise.

::: dom/bindings/Codegen.py
@@ +85,5 @@
>          if self.descriptor.concrete and self.descriptor.proxy:
>              resolveOwnProperty = "ResolveOwnProperty"
>              enumerateOwnProperties = "EnumerateOwnProperties"
> +        elif self.descriptor.interface.getExtendedAttribute("NeedNewResolve"):
> +            resolveOwnProperty = "ResolveOwnPropertyViaNewresolve"

Would we ever have both a NewResolve hook _and_ other own properties that might be resolved some other way? Or are they orthogonal (proxies vs non-proxies)? If they might overlap, this should probably be called 'ResolveOwnPropertyWithNewResolve' or something.

@@ +6678,5 @@
> +                "if (!self->DoNewResolve(cx, obj, id, &value)) {\n"
> +                "  return false;\n"
> +                "}\n"
> +                "if (!value.isUndefined()) {\n"
> +                "  FillPropertyDescriptor(desc, wrapper, value, false);\n"

Please add /* readOnly = */ before the |false|.

Comment 52

[Boris Zbarsky [:bzbarsky]]

I can't think of any case where something would both have a named getter and a resolve hook except for Window, and Window is all sorts of special snowflake anyway.

Will fix the other bits.  Thanks for the review!

Comment 53

[Boris Zbarsky [:bzbarsky]]

And if course I forgot to make those changes before pushing, so had to push a followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7ef42d1fe1
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd89d97430c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a33453d3cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0fe0a7cc385
https://hg.mozilla.org/integration/mozilla-inbound/rev/738f332df9f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fcca099a7c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/97c9600b2f9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/325c7efdbcf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0249e847f1c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c9e64a03dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e32217fa899

The try run for this was https://tbpl.mozilla.org/?tree=Try&rev=4f8716f30119 for all the good it will do given the current state of try.  :(

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c7ef42d1fe1
https://hg.mozilla.org/mozilla-central/rev/dd89d97430c5
https://hg.mozilla.org/mozilla-central/rev/88a33453d3cb
https://hg.mozilla.org/mozilla-central/rev/e0fe0a7cc385
https://hg.mozilla.org/mozilla-central/rev/738f332df9f3
https://hg.mozilla.org/mozilla-central/rev/8fcca099a7c3
https://hg.mozilla.org/mozilla-central/rev/97c9600b2f9d
https://hg.mozilla.org/mozilla-central/rev/325c7efdbcf5
https://hg.mozilla.org/mozilla-central/rev/0249e847f1c4
https://hg.mozilla.org/mozilla-central/rev/90c9e64a03dc
https://hg.mozilla.org/mozilla-central/rev/0e32217fa899

Comment 55

[Boris Zbarsky [:bzbarsky]]

I've updated https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#NeedNewResolve to document the new setup.

Comment 57

[Adam Moore]

https://hg.mozilla.org/mozilla-central/rev/7c7ef42d1fe1 caused a regression.  general.appname.override no longer is honored due to the removal of the return:

   1.714 -nsresult
   1.715 +void
   1.716  NS_GetNavigatorAppName(nsAString& aAppName)
   1.717  {
   1.718    if (!nsContentUtils::IsCallerChrome()) {
   1.719      const nsAdoptingString& override =
   1.720        mozilla::Preferences::GetString("general.appname.override");
   1.721  
   1.722      if (override) {
   1.723        aAppName = override;
   1.724 -      return NS_OK;
   1.725      }
   1.726    }
   1.727  
   1.728    aAppName.AssignLiteral("Netscape");
   1.729 -  return NS_OK;
   1.730  }

Comment 58

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Adam, please file a new bug and make it block this one.