Fix the include situation for nsIDOM* when NeedsGeneratedHasInstance

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox40 affected, firefox41 fixed)

Details

Attachments

(1 attachment)

Our generated hasinstance method uses an nsIDOM* interface and QIs to it.  We include the relevant header like so:

        hasInstanceIncludes = set("nsIDOM" + d.interface.identifier.name + ".h" for d
                                  in descriptors if
                                  NeedsGeneratedHasInstance(d) and
                                  d.interface.hasInterfacePrototypeObject())

But we only need a hasInstance at all if hasInterfaceObject(), so we should check for that here too.
(Assignee)

Comment 1

4 years ago
Created attachment 8597990 [details] [diff] [review]
Don't try to include stuff for a generated hasInstance hook if we have no interface object, since in that case we don't need the include

Not that this matters much, since the only things that are NeedsGeneratedHasInstance() and have an proto object are EventTarget (due to xponnect impls which we should really kill) and XPathEvaluator (which we should consider just breaking the hasinstance for, maybe....)
Attachment #8597990 - Flags: review?(peterv)

Comment 2

4 years ago
This webidl triggers the include of "nsIDOMCredential.h".
https://github.com/AxelNennker/firefox_credentials/blob/master/Credentials.webidl

dictionary CredentialData {
  DOMString id;
  DOMString name;
  USVString iconURL;
};


interface Credential {
  readonly attribute DOMString id;
  readonly attribute DOMString type;
};

dictionary PasswordCredentialData : CredentialData {
  DOMString password;
};

[Constructor(optional PasswordCredentialData data), Exposed=Window]
interface PasswordCredential {
  Promise<Response> send(USVString url);
};
PasswordCredential implements Credential;

dictionary CredentialRequestOptions {
  sequence<USVString> federations;
};
  
dictionary CredentialRequest {
  CredentialRequestOptions options;
  boolean suppressUI = false;
  sequence<DOMString> types;
};

[JSImplementation="@mozilla.org/credentials/manager;1", NavigatorProperty="credentials", NoInterfaceObject]
interface CredentialsContainer {
  Promise<Credential?> get(optional CredentialRequest request);
  Promise<Credential> store(optional Credential credential);
  Promise<void> requireUserMediation();
};

 9:17.76 In file included from /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings3.cpp:62:0:
 9:17.76 /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/CredentialsBinding.cpp:15:30: fatal error: nsIDOMCredential.h: No such file or directory
 9:17.76  #include "nsIDOMCredential.h"
 9:17.76                               ^
 9:17.76 compilation terminated.
 9:20.01 Unified_cpp_webrtc_modules2.o
 9:22.03 Unified_cpp_webrtc_modules0.o
 9:22.03 Unified_cpp_webrtc_modules1.o
 9:22.24 
 9:22.24 In the directory  /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings
 9:22.24 The following command failed to execute properly:
 9:22.24 /usr/bin/ccache c++ -o UnifiedBindings3.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/ignisvulpis/mozilla/firefox/config/gcc_hidden.h -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DMOZ_GLUE_IN_PROGRAM -DAB_CD=en-US -DNO_NSPR_10_SUPPORT -I/home/ignisvulpis/mozilla/firefox/dom/bindings -I. -I../../dist/include/mozilla/dom -I/home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/ipc/ipdl/_ipdlheaders -I/home/ignisvulpis/mozilla/firefox/dom/base -I/home/ignisvulpis/mozilla/firefox/dom/battery -I/home/ignisvulpis/mozilla/firefox/dom/bluetooth -I/home/ignisvulpis/mozilla/firefox/dom/camera -I/home/ignisvulpis/mozilla/firefox/dom/canvas -I/home/ignisvulpis/mozilla/firefox/dom/geolocation -I/home/ignisvulpis/mozilla/firefox/dom/html -I/home/ignisvulpis/mozilla/firefox/dom/indexedDB -I/home/ignisvulpis/mozilla/firefox/dom/media/webaudio -I/home/ignisvulpis/mozilla/firefox/dom/media/webspeech/recognition -I/home/ignisvulpis/mozilla/firefox/dom/svg -I/home/ignisvulpis/mozilla/firefox/dom/workers -I/home/ignisvulpis/mozilla/firefox/dom/xbl -I/home/ignisvulpis/mozilla/firefox/dom/xml -I/home/ignisvulpis/mozilla/firefox/dom/xslt/base -I/home/ignisvulpis/mozilla/firefox/dom/xslt/xpath -I/home/ignisvulpis/mozilla/firefox/dom/xul -I/home/ignisvulpis/mozilla/firefox/js/xpconnect/src -I/home/ignisvulpis/mozilla/firefox/js/xpconnect/wrappers -I/home/ignisvulpis/mozilla/firefox/layout/style -I/home/ignisvulpis/mozilla/firefox/layout/xul/tree -I/home/ignisvulpis/mozilla/firefox/media/mtransport -I/home/ignisvulpis/mozilla/firefox/media/webrtc/ -I/home/ignisvulpis/mozilla/firefox/media/webrtc/signaling/src/common/time_profiling -I/home/ignisvulpis/mozilla/firefox/media/webrtc/signaling/src/peerconnection -I/home/ignisvulpis/mozilla/firefox/ipc/chromium/src -I/home/ignisvulpis/mozilla/firefox/ipc/glue -I/home/ignisvulpis/mozilla/firefox/dom/bluetooth/bluetooth1 -I../../dist/include -I/home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dist/include/nspr -I/home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dist/include/nss -fPIC -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/UnifiedBindings3.o.pp -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -Wno-uninitialized -DDEBUG -DTRACING -g -freorder-blocks -Os -fno-omit-frame-pointer /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings3.cpp
 9:22.24 make[5]: *** [UnifiedBindings3.o] Error 1
 9:22.24 make[5]: *** Waiting for unfinished jobs....

I tried to cleanup and get rid of the "interface" stuff but without it I get the error message above.
https://github.com/AxelNennker/firefox_credentials/tree/master/interfaces


-Axel
(Assignee)

Comment 3

4 years ago
> This webidl triggers the include of "nsIDOMCredential.h".

Why did you remove the [NoInterfaceObject] from Credential?

If something appears on the RHS of "implements", it should be [NoInterfaceObject].
(Assignee)

Comment 4

4 years ago
In any case, comment 2 has nothing to do with this bug....  Given the IDL there the include _is_ needed, since Credential has both an interface object and a prototype object but appears on the rhs of "implements".

Comment 5

4 years ago
(In reply to Not doing reviews right now from comment #3)
> > This webidl triggers the include of "nsIDOMCredential.h".
> 
> Why did you remove the [NoInterfaceObject] from Credential?
> 
> If something appears on the RHS of "implements", it should be
> [NoInterfaceObject].

Probably because I still could not get my head around [NoIntercaceObject].
My reading of http://www.w3.org/TR/WebIDL/#NoInterfaceObject is that the interface just does not appear on window;

Which text in the TR implies the rule?: 
"If something appears on the RHS of "implements", it should be [NoInterfaceObject]."

My guess is that the rule is related to supplemental interfaces:
http://www.w3.org/TR/WebIDL/#dfn-supplemental-interface
But in my reading this says when not to use NoInterfaceObject but not when is should be.

Comment 6

4 years ago
(In reply to Not doing reviews right now from comment #4)
> In any case, comment 2 has nothing to do with this bug....  Given the IDL
> there the include _is_ needed, since Credential has both an interface object
> and a prototype object but appears on the rhs of "implements".

Does this mean that I have to describe Credential twice? Once in WebIDL and then again in idl?
https://github.com/AxelNennker/firefox_credentials/blob/master/interfaces/nsIDOMCredential.idl
(or write nsIDOMCredential.h myself)

And then write 
class Credential : nsIDOMCredential
{
void GetId(nsString& aRetVal) { aRetVal =  id; }

Probably again my missing understanding of the subject... sorry.
(Assignee)

Comment 7

4 years ago
> Which text in the TR implies the rule?: 

None.  It's allowed per WebIDL spec, and there is one API in the web platform that does it.  It's just bad API design and people shouldn't add more APIs like that.  If you _do_ decide to add an API like that, you will need to define a corresponding nsIDOM* for use in instanceof checks.

Again, none of that has anything to do with this bug.

> Does this mean that I have to describe Credential twice?

If you insist on using interfaces on the rhs of implements that have an interface object, then yes.  The IDL version can be an empty interface; all codegen cares about is being able to QI to it.  

Again, the right solution is to not have interfaces with an interface object that are on the rhs of "implements".  That conflates mixins and classes in an undesirable way.

Comment 8

4 years ago
Even when Credential is a NoInterfaceObject the "#include nsIDOMCredential.h" is needed.

-----

ignisvulpis@namenlos:~/mozilla/firefox$ cat dom/webidl/Credentials.webidl 
/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* 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/.
 */

dictionary CredentialData {
  DOMString id;
  DOMString name;
  USVString iconURL;
};

[NoInterfaceObject]
interface Credential {
  readonly attribute DOMString id;
  readonly attribute DOMString type;
};

dictionary PasswordCredentialData : CredentialData {
  DOMString password;
};

[Constructor(optional PasswordCredentialData data), Exposed=Window]
interface PasswordCredential {
  Promise<Response> send(USVString url);
};
PasswordCredential implements Credential;

dictionary CredentialRequestOptions {
  sequence<USVString> federations;
};
  
dictionary CredentialRequest {
  CredentialRequestOptions options;
  boolean suppressUI = false;
  sequence<DOMString> types;
};

[JSImplementation="@mozilla.org/credentials/navigatorcredentials;1", NavigatorProperty="credentials", NoInterfaceObject]
interface CredentialsContainer {
  Promise<Credential?> get(optional CredentialRequest request);
  Promise<Credential> store(optional PasswordCredential credential);
  Promise<void> requireUserMediation();
};
ignisvulpis@namenlos:~/mozilla/firefox$ 

------

 6:05.03 In file included from /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings3.cpp:62:0:
 6:05.04 /home/ignisvulpis/mozilla/firefox/obj-x86_64-unknown-linux-gnu/dom/bindings/CredentialsBinding.cpp:15:30: fatal error: nsIDOMCredential.h: No such file or directory
 6:05.04  #include "nsIDOMCredential.h"
 6:05.04                               ^
 6:05.04 compilation terminated.

------

Why do I have to write any implementation of Credential anyway? 
Why would anyone have two different implementations for simple interfaces like Credential.
Maybe introduce [NoInterfaceObject, MozAutoGeneratedImplementation]

+++++++

Another question: Why does Mozilla have NavigatorProperty="credentials" instead of 
partial interface Navigator {
  readonly attribute CredentialContainer credentials;
};

and would'nt this be the way for the credential management spec to solve the extension point

partial interface CredentialContainer {
  Promise<Credential?> get(optional CredentialRequest request);
  Promise<void>requireUserMediation();
};
partial interface CredentialContainer {
    Promise<Credential> store(PasswordCredential credential);
}
partial interface CredentialContainer {
    Promise<Credential> store(FooCredential credential);
}
(Assignee)

Comment 9

4 years ago
> Even when Credential is a NoInterfaceObject the "#include nsIDOMCredential.h" is needed.

Yes, that, finally, is this bug.  You should have tried with the patch here.


The rest of what you wrote has nothing to do with this bug.  Feel free to mail me directly if you have codegen questions, but this is not the right place for them.  I'm going to respond to comment 8, but that's it.

> Why do I have to write any implementation of Credential anyway? 

Because it's being passed as an argument (as I've said before, the spec is buggy here), so the codegen needs to know what that maps to on the C++ side.  Note that you don't necessarily need an implementation per se; e.g. it could all be pure virtual with all implementation living in the subclasses.

> Why would anyone have two different implementations for simple interfaces like
> Credential.

I'm not sure what you're asking here.  Please feel free to clarify over e-mail.

> Maybe introduce [NoInterfaceObject, MozAutoGeneratedImplementation]

I have no idea what the generated implementation would do here and hence whether it would be commonly used.  We do have something like this for DOM events, since that _is_ commonly enough used that it made sense to hook up implementation autogeneration.  Again, this bug is not the right place for this discussion.

> Another question: Why does Mozilla have NavigatorProperty="credentials"

Which Mozilla?  There is no NavigatorProperty="credentials" anywhere in our source code.  Again, nothing to do with this bug, feel free to send me mail.

> and would'nt this be the way for the credential management spec to solve

Yes, that would work fine.  As would using a union type.
Comment on attachment 8597990 [details] [diff] [review]
Don't try to include stuff for a generated hasInstance hook if we have no interface object, since in that case we don't need the include

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

::: dom/bindings/Codegen.py
@@ +1774,5 @@
>          args = [Argument('JSContext*', 'cx'),
>                  Argument('JS::Handle<JSObject*>', 'obj'),
>                  Argument('JS::MutableHandle<JS::Value>', 'vp'),
>                  Argument('bool*', 'bp')]
> +        assert descriptor.interface.hasInterfaceObject()x

Drop that trailing 'x'?
Attachment #8597990 - Flags: review?(peterv) → review+
> Drop that trailing 'x'?

Er, yes, long since done locally since it totally fails to run that way.  ;)
https://hg.mozilla.org/mozilla-central/rev/13f7e6d0e909
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.