Steve Fink [:sfink] [:s:] Reporter
	Description • 8 years ago

I would like a "can be JS-implemented" __attribute__ of some sort for the GC rooting hazard analysis, so I know whether the current set of virtual implementations is complete or not. (Or more simply, I want to treat anything that can be imlemented in JS as GC'ing.)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 8 years ago

Steve, does this do what you want?

	Steve Fink [:sfink] [:s:] Reporter
	Comment 2 • 8 years ago

Comment on attachment 8848171 [details] [diff] [review]
Annotate xpidl methods/attributes that can be implemented in script as JS_HAZ_CAN_RUN_SCRIPT

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

This is perfect, thanks!

It looks like sixgill is currently not producing output for pure virtual method decls, so I'll need to look into that before I can say for sure that this will work for me.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 8 years ago

Comment on attachment 8848171 [details] [diff] [review]
Annotate xpidl methods/attributes that can be implemented in script as JS_HAZ_CAN_RUN_SCRIPT

Optimistically (in terms of this doing what Steve wants) requesting review.

	Nathan Froyd [:froydnj]
	Comment 4 • 8 years ago

Comment on attachment 8848171 [details] [diff] [review]
Annotate xpidl methods/attributes that can be implemented in script as JS_HAZ_CAN_RUN_SCRIPT

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

If my comment below is true, you can probably get away with a lot fewer changes to header.py, since you won't have to thread the parent interface all the way through.

::: xpcom/idl-parser/xpidl/header.py
@@ +140,5 @@
> +    # This can only happen if the interface is scriptable and not builtinclass and the
> +    # member is scriptable too.
> +    return (iface.attributes.scriptable and
> +            not iface.attributes.builtinclass and
> +            not member.noscript)

Doesn't this reduce down to member.isScriptable()--which works for both Attribute and Method?

http://dxr.mozilla.org/mozilla-central/source/xpcom/idl-parser/xpidl/xpidl.py#803
http://dxr.mozilla.org/mozilla-central/source/xpcom/idl-parser/xpidl/xpidl.py#887

(I suppose those methods should assert that things are builtinclass?  Though it looks like we'll happily create a non-scriptable, non-builtinclass interface...)

If isScriptable() doesn't do the right thing for this, we should probably fix something...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 8 years ago

> Doesn't this reduce down to member.isScriptable()--which works for both Attribute and Method?

Sadly, no.  isScriptable() does this:

        if not self.iface.attributes.scriptable:
            return False
        return not self.noscript

which comes down to, effectively:

  return iface.attributes.scriptable and not member.noscript

but completely ignores builtinclass.  That is, isScriptable() answers the "can this be called from script?" question, and hence doesn't care whether the interface is builtinclass or not, but I want to answer the "can calling this call into script?" question.

What I _could_ do is change my condition to:

  return member.isScriptable() and not member.iface.attributes.builtinclass

and stop threading the iface through, since it's reachable from the member.

	Nathan Froyd [:froydnj]
	Comment 7 • 8 years ago

Comment on attachment 8848243 [details] [diff] [review]
With that simplification

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

Oh, duh, sorry.  Got attributes a bit mixed up there.  This is OK.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 8 years ago

Steve, let me know whether this ends up doing what you need (in which case I'll land it) or whether something else is needed, ok?

	Steve Fink [:sfink] [:s:] Reporter
	Comment 9 • 6 years ago

I finally got back to working on some hazard stuff. Sixgill did not have support for this particular granularity of attributes (it could do class/structure/union attributes and toplevel/static function attributes, but that's it.) I have implemented support for virtual method attributes and it appears to work.

Given that there appears to be interest in correctly identifying everything that can run script (CC'ing Alex Gaynor), I believe this would be useful even if there are simpler ways of identifying which functions can GC -- we want to be able to distinguish the subset "can run script" of the "can GC" functions.

Can this still be landed?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 6 years ago

Wow, it's been a while.  Amazingly, this still applied to tip.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5207f7c92930019eda375e92d2be85ed2de43aea

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 6 years ago

The test was calling print_header() without calling resolve(), so objects didn't have the properties they expected to have.

	Pulsebot
	Comment 12 • 6 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3895fde69249
Annotate xpidl methods/attributes that can be implemented in script as JS_HAZ_CAN_RUN_SCRIPT. r=froydnj

	Steve Fink [:sfink] [:s:] Reporter
	Comment 13 • 6 years ago

Thank you! Now I just need to write the analysis code that uses it.

	Cristina Coroiu [:ccoroiu]
	Comment 14 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3895fde69249