414063 - annotate xpidl-generated c++ headers with attributes for scriptable and deprecated methods

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Mandelin [:dmandelin]]

Please add an attribute something like __attribute__(scriptable) to class and method declarations that are scriptable. 

It appears that it is pretty much necessary for static analysis safety--without it, there is no reliable way to find out which methods can be called from JS.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Patch that creates NS_SCRIPTABLE on methods

This is a patch I've been working on over the past few days. It annotates all methods and attributes without [noscript] on [scriptable] interfaces with NS_SCRIPTABLE. In addition, it also defines [deprecated] for all interfaces, attributes, and methods using NS_DEPRECATED. Both of these are defined in nscore.h.

To use NS_SCRIPTABLE, the flag DEHYDRA_GCC is needed to get the attribute defined.

Comment 2

[David Bradley]

Comment on attachment 300776 [details] [diff] [review]
Patch that creates NS_SCRIPTABLE on methods

Looks fine, my only concern is that the patch really goes beyond the scope of this bug since it's adding the deprecated attribute as well as the logic for scriptable marking. Someone should probably update the summary to reflect this so others can find this bug in future when dealing with the deprecated functionality. Ideally it probably should be a separate bug so it could be considered on it's own. But I'm ok with it being one patch, one bug. I think it will be a useful tool to help move people to proper API's.

Comment 4

[timeless]

Comment on attachment 300776 [details] [diff] [review]
Patch that creates NS_SCRIPTABLE on methods

/js/src/xpconnect/idl/xpcjsid.idl * line 60 -- [notxpcom] const_nsID_ptr getID();

> Joshua it annotates with scriptable

FAIL.

Comment 5

[David Bradley]

Comment on attachment 300776 [details] [diff] [review]
Patch that creates NS_SCRIPTABLE on methods

Yes, you need to add the notxpcom check as well. Ideally we probably should add a function to this code base named something like IsMethodScriptable or some such since there's really no simple test.
One check is done here, I'm sure there are others: http://lxr.mozilla.org/seamonkey/source/extensions/java/xpcom/tools/xpidl/xpidl_util.c#551

So I'd see if you can create a common function and then use it. Then hopefully others won't make this same easy mistake in the future.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Try #2

This patch:
* Checks for [notxpcom] and emits NS_SCRIPTABLE on those instances
* Adds @deprecated in the doc comment for xpidl_java emitting.
* Has a separate function for testing method and attribute scriptability.
* Does everything else the old patch did.
* Is an hg-style patch.

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: REAL patch #2

Please ignore my previous attachment. I accidentally left some debug stuff in.

Comment 8

[timeless]

Comment on attachment 301169 [details] [diff] [review]
REAL patch #2

comment 6 is wrong and doesn't match your patch, so ignoring that...

i can't remember how oom safe xpidl is

+    replaced = (char *)malloc(strlen(buffer)+20);

if this were standard gecko code, i'd ask that you:
1) ensure the arg to malloc doesn't overflow
2) check for oom

+     * see if it's [scriptable] and duly not it.

as mentioned on irc (by someone else), this should be "note"

+    /* The interface is scriptable, so therefore the method must be. */

this isn't right. i think you ran away last time before i gave you the example for why it's wrong.

try:
nsIAccessibleRetrieval.getAccessibleInShell

in today's world, that's not-scriptable

please be warned: just because it isn't scriptable today doesn't mean i might not eventually fix it, but if i do, i promise to update this code :).

Comment 9

[David Mandelin [:dmandelin]]

(In reply to comment #8)
> (From update of attachment 301169 [details] [diff] [review])
> comment 6 is wrong and doesn't match your patch, so ignoring that...
> 
> i can't remember how oom safe xpidl is
> 
> +    replaced = (char *)malloc(strlen(buffer)+20);
> 
> if this were standard gecko code, i'd ask that you:
> 2) check for oom

According to discussions last week, we're not going to do OOM checks any more--they don't really work. Instead there will be a centralized handler that ensures that calls to allocation functions never return null.

> +    /* The interface is scriptable, so therefore the method must be. */
> 
> this isn't right. i think you ran away last time before i gave you the example
> for why it's wrong.
> 
> try:
> nsIAccessibleRetrieval.getAccessibleInShell
> 
> in today's world, that's not-scriptable

For my own edification (as a user of this feature, I should understand how it works), why is this? I looked at the IDL and it looked like every other method of nsIAccessibleRetrieval.

Comment 10

[Joshua Cranmer [:jcranmer]]

Attachment: Patch #3

This patch drops the NS_SCRIPTABLE stuff per a discussion in IRC about when exactly stuff is scriptable in the mozilla tree that concluded that xpconnect hackery was better than xpidl hackery.

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: Yet Another Patch

Patches tend to work better if you update CVS before posting an updated patch...

Comment 12

[Benjamin Smedberg]

Joshua, could you document the IRC conversation that led you to drop the NS_SCRIPTABLE stuff? timeless' odd comments notwithstanding, we still need that attribute for static analysis work.

Comment 13

[David Mandelin [:dmandelin]]

Paraphrasing, timeless said that the correct rule is that a method is scriptable if the interface is 'scriptable', the method is not 'noscript' (a.k.a. hidden), and none of the argument types of the method lack IIDs. (Example: nsIDocument doesn't have an IID, apparently in order to prevent instances being used as parameters from scripts.) timeless said that it would be much easier to apply this rule on xpt_dump output than in the IDL compiler. I think it's because you need to cross-reference results of compiling all the IDL files to see what doesn't have an IID.

I have a Python script that finds the scriptable methods according to that rule in Elsa format. I'll attach it.

Comment 14

[David Mandelin [:dmandelin]]

Attachment: scriptable method extractor

To use, first xpt_link all the IDL, then xpt_dump it (non-verbose), then run it through this script. I think you also need a file that has just the list of unresolved interface names. I prepared it manually as:

OMException
LSException
RangeException
XPathException
nsAString
nsICmdLineService
nsIContent
nsIDOMCSSStyleRuleCollection
nsIDocument
nsIFileSpec
nsIJVMPlugin
nsILayoutHistoryState
nsIMdbEnv
nsIMdbRow
nsIMdbTable
nsIObjectFrame
nsISAXEntityResolver
nsIScriptElement
nsIScriptGlobalObject
nsISecureEnv
nsIServiceManagerObsolete
nsIUnicodeDecoder
nsIUnicodeEncoder
nsIWidget
nsIWordBreaker

Comment 15

[Benjamin Smedberg]

We don't need to be that complex. Let's just stick with the easy output we can get from the xpidl compiler. Sure there are methods which are marked scriptable but aren't actually scriptable in practice, but those aren't IMO worth distinguishing for any of our static analysis work.

Comment 16

[David Mandelin [:dmandelin]]

(In reply to comment #15)
> We don't need to be that complex. 

You are probably right. The naive approach, corresponding to the initial spec for the attributes, should find 7520 methods, while filtering by no-IID gets it down only to 7472.

Comment 17

[Joshua Cranmer [:jcranmer]]

Attachment: Patch with NS_SCRIPTABLE again

A new patch with NS_SCRIPTABLE, as re-requested by dmandelin/bsmedberg. Slightly different from the second patch in that the second one has an unused variable in it.

Comment 18

[David Bradley]

Comment on attachment 303799 [details] [diff] [review]
Patch with NS_SCRIPTABLE again

The comment list usage in add_deprecated seems out of sorts. Shouldn't you be using g_slist_append to add the string. Also shouldn't that string be allocated instead of just assigning the constant? You did that in the subsequent code when replacing the value. And you probably shouldn't be accessing the "data" member directly but using g_slist_nth_data and other family of API methods to do the manipulation.

In add_deprecated the strrchr call can return null. You should test for  and handle that condition.

Also in add_deprecated it should declare \n* @deprecated\n*/ as a constant and then you can use the "sizeof" that instead of the 20 in the malloc. That way if someone happens to change that message we won't be over running the buffer.

Typo should be note:
+     * see if it's [scriptable] and duly not it.

In  verify_attribute_declaration you removed the only use of scriptable_interface so the definition and assignment should just be removed.

Also in verify_method_declaration you can remove the declaration of scriptable_interface as it's no longer needed.

Comment 19

[Joshua Cranmer [:jcranmer]]

Attachment: Patch #?

This is patch #?, where `?' stands for some positive real number.

Addresses dbradley's comments, I hope.

Comment 20

[David Bradley]

Comment on attachment 306648 [details] [diff] [review]
Patch #?

r=dbradley

With one minor addition.

Change:
if (buffer == NULL || buffer[-1] == '*') {

to:
if (buffer == NULL || replace == buffer || buffer[-1] == '*') {

Just makes sure by some chance if the only / is the first character in the buffer we aren't copying before the buffer.

Comment 21

[Joshua Cranmer [:jcranmer]]

Attachment: patch to check in

Adding the patch to be checked in...

Comment 22

[Joshua Cranmer [:jcranmer]]

(I apologize for the bugspam...)

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Tests, please!

Comment 24

[Benjamin Smedberg]

We should land this in mozilla-central (not CVS) and then it's possible to hook up some kind of test in xpcom/tests/stack-checking

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It's not possible to test via "make check", a test IDL file, and searching the output?

Comment 26

[Benjamin Smedberg]

Searching for what? The random presence of NS_SCRIPTABLE anywhere, or actually checking that it's in the right place (and not in the wrong place)?

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Gold master?

Comment 28

[Damon Sicore (:damons)]

Holding off on approval until we can get a solid reason why we don't have tests.  Side note:  It REALLY helps when people ask for tests in bugs!  Thanks, Shaver.

Comment 29

[David Mandelin [:dmandelin]]

The most important test is simply that Mozilla builds using this version of the XPIDL compiler.

The other kind of test is whether NS_SCRIPTABLE is in the right place. I guess it would be sufficient have test cases where the input is a small IDL file, the test runs the IDL compiler on them, and the expected output is that the .h file compiles and grep finds the correct line with NS_SCRIPTABLE.

Comment 30

[Damon Sicore (:damons)]

Comment on attachment 307825 [details] [diff] [review]
patch to check in

Please re-request approval once test(s) is/are included.

Comment 31

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, now with tests!

This patch adds a simple test to see if stuff that is scriptable is marked NS_SCRIPTABLE and stuff that is not is not.

Sorry about the complicated command in the second test, I just couldn't write a simpler command that got it to work...

Comment 32

[David Bradley]

Comment on attachment 308547 [details] [diff] [review]
Patch, now with tests!

r=dbradley

Comment 33

[Damon Sicore (:damons)]

Comment on attachment 308547 [details] [diff] [review]
Patch, now with tests!

Very cool.  Thanks for adding tests.  

a1.9+=damons

Comment 34

[Reed Loden [:reed]]

Checking in xpcom/base/nscore.h;
/cvsroot/mozilla/xpcom/base/nscore.h,v  <--  nscore.h
new revision: 1.104; previous revision: 1.103
done
Checking in xpcom/tests/Makefile.in;
/cvsroot/mozilla/xpcom/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.105; previous revision: 1.104
done
RCS file: /cvsroot/mozilla/xpcom/tests/TestScriptable.idl,v
done
Checking in xpcom/tests/TestScriptable.idl;
/cvsroot/mozilla/xpcom/tests/TestScriptable.idl,v  <--  TestScriptable.idl
initial revision: 1.1
done
Checking in xpcom/typelib/xpidl/xpidl.h;
/cvsroot/mozilla/xpcom/typelib/xpidl/xpidl.h,v  <--  xpidl.h
new revision: 1.50; previous revision: 1.49
done
Checking in xpcom/typelib/xpidl/xpidl_header.c;
/cvsroot/mozilla/xpcom/typelib/xpidl/xpidl_header.c,v  <--  xpidl_header.c
new revision: 1.102; previous revision: 1.101
done
Checking in xpcom/typelib/xpidl/xpidl_java.c;
/cvsroot/mozilla/xpcom/typelib/xpidl/xpidl_java.c,v  <--  xpidl_java.c
new revision: 1.21; previous revision: 1.20
done
Checking in xpcom/typelib/xpidl/xpidl_util.c;
/cvsroot/mozilla/xpcom/typelib/xpidl/xpidl_util.c,v  <--  xpidl_util.c
new revision: 1.28; previous revision: 1.27
done