1545801 - Statically identify a set of XPIDL interfaces that can be marked builtinclass

Status: RESOLVED WORKSFORME

(Core :: XPCOM, task)

Description

[Andrew McCreight [:mccr8]]

It would be good to mark more XPIDL interfaces that are never used from JS as builtinclass. One way to do this would be to have the browser dynamically detect which interfaces are never implemented in JS, but this would be annoying because there are many interfaces that are only implemented in JS for use in a test or two, so you'd have to run every test suite and then combine the results.

I have a script in progress that instead takes a static approach. It scans every file in the tree looking for calls to ChromeUtils.generateQI, which gives us a set of interfaces that are implemented in JS. It also scans every XPIDL file in the tree, looking for interfaces that are not already marked builtinclass. With the combination of these two, we can generate a list of classes that might be markable as builtinclass.

One limitation is that a callback-like interface, that contains just a single function, or something like that, can be implemented without explicitly implementing a QI. There may be some other similar cases. But this at least gives us a starting set of things to look at.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: initial script results

There are a little over 700 interfaces here. Out of the first 4 or so interfaces I looked at, all but one were false positives, so I don't know how useful this is, but it is a start.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: builtin.py

Here's the analyzer script. It takes a couple of minutes to run, and requires some patches to XPIDL files because the parsing is quite fragile. I'll try to land those XPIDL cleanups in bugs blocking this one.

Comment 3

[Boris Zbarsky [:bzbarsky]]

False positives in what sense? Both IJSDebugger and IPeerConnection look like they could be builtinclass...

And IPeerConnectionObserver looks unused.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #0)

One limitation is that a callback-like interface, that contains just a single function, or something like that, can be implemented without explicitly implementing a QI. There may be some other similar cases. But this at least gives us a starting set of things to look at.

It's actually much worse than that. Any time a JS object is passed to a function that expects a specific interface, or returned from a function that's supposed to return a specific interface, it automatically implements that interface, whether its QI function says it does or not. That means basically any interface can be implemented by a JS object even if the interface name doesn't appear anywhere in our JS sources.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Kris Maglione [:kmag] from comment #4)

It's actually much worse than that. Any time a JS object is passed to a function that expects a specific interface, or returned from a function that's supposed to return a specific interface, it automatically implements that interface, whether its QI function says it does or not. That means basically any interface can be implemented by a JS object even if the interface name doesn't appear anywhere in our JS sources.

Ah, interesting.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

False positives in what sense? Both IJSDebugger and IPeerConnection look like they could be builtinclass...

One example is nsIDOMXULRadioGroupElement, which is passed as an argument to MozXULElement.implementCustomInterface(), which eventually passes it to generateQI().

Comment 6

[Andrew McCreight [:mccr8]]

Another example of a false positive is that I managed to find a class that was still manually implementing QI. Unfortunately I don't recall what it was. My script could probably detect that, come to think of it.

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #6)

Another example of a false positive is that I managed to find a class that was still manually implementing QI. Unfortunately I don't recall what it was. My script could probably detect that, come to think of it.

A search on SearchFox for "queryinterface :" finds a few.

Comment 8

[Andrew McCreight [:mccr8]]

The analysis did not find nsIPrefLocalizedString for some reason.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #8)

The analysis did not find nsIPrefLocalizedString for some reason.

Oh, I see. My forward interface declaration detection was not specific enough.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: possible builtinclass interfaces

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: builtin.py

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Print out a message if something is a builtinclass. NOT FOR LANDING.

This patch is useful for debugging failures when marking classes builtinclass. It logs a message containing the interface name when something checks if a class is builtin, and it is. I originally make it fatal, but it was causing crashes when it shouldn't have been. That might have just been another bug in that version of the ptach.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: possible builtinclass interfaces

This version of the output (applied to a tree with some local patches to make more things builtinclass) is split up by directory, which is useful if somebody wanted to focus on the dom/ directory or whatever.

Comment 14

[Andrew McCreight [:mccr8]]

Another interesting category is interfaces that are implicitly builtinclass. Some things you can declare on interfaces make it impossible to implement an interface in JS. For some of these, we throw an error. For others, they get a flag implicit_builtinclass set, which looks like builtinclass to any consumer of the XPT JSON. There are about 30 of those classes that don't have builtinclass set, at least when I checked in a build. We could consider making them explicitly declare builtinclass.

There may also be other conditions that prevent JS implementation we could check for. Nika has the comment "Why does nostdcall not imply builtinclass?" in xpidl.py

Comment 15

[Andrew McCreight [:mccr8]]

I think we can close this for now.