477166 - Have an automated way to check for changed interfaces without iid changes

Status: RESOLVED WONTFIX

(Developer Services :: Mercurial: hg.mozilla.org, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

We should have an automated way, running as part of our release process, to identify changed interfaces that have not had their iid rev-ed.  That would prevent us from shipping such changes.

Not sure what the right bucket for this bug is....

Comment 1

[Benjamin Smedberg]

Should block 1.9.2 on doing this manually, but I don't think we necessarily need to do it automatically as part of the release.

Comment 2

[[not reading bugmail]]

Maybe you could try some kind of changelog design (I saw this in SAP for every user change, its old vs new values, but its principle is the same) which could hold values: 

'null' or 'old value'/'new value' or 'null' having 4 states which could be compared automatically: ie..

Null/New  - brand new items
Old/Null  - killed off, dead wood
Old/New   - updated, rev'd
Old/Old   - no change

an example would be /1 or 1/ or 1/2 or 1/1

Comment 3

[Dave Townsend [:mossop]]

Attachment: WIP patch

This is a work in progress that adds a tool xpt_diff to the tree. When run and given two xpt files it compares them looking for any interfaces that didn't change IID but have changed in some way. It should be making the following checks:

Interface flags (scriptable etc.) didn't change.
Interface parent didn't change (using parent's name only, not IID).

Method flags didn't change.
Method return type didn't change.
Method arguments didn't change (changing an arg to/from optional is ignored).
Methods weren't added or removed.

Constant type and value didn't change.
Constants weren't removed.

This still needs some tests before I think it should be properly reviewed and right now the output is pretty confusing, but for now it is providing some information that indicated problems between 1.9.1 and 1.9.2 so that might be enough for at least the blockingness of this bug.

Comment 4

[Dave Townsend [:mossop]]

Attachment: potential problems

Here is the current output. Some of these I know are false positives (in that the change is there but it doesn't actually affect the ABI so wouldn't need an IID rev) but some are real problems that I'll either file bugs on or just put a patch together to rev a bunch of IIDs

Comment 5

[Dave Townsend [:mossop]]

Filed bug 526635 with patches for the initial issues

Comment 6

[Dave Townsend [:mossop]]

Also bug 526651 for current differences between 1.9.2 and trunk

Comment 7

[palo misik]

>Interface parent didn't change (using parent's name only, not IID).
I think this is not proof enough that interface is changed.

Imagine a situation of inheritance 

A
^
|
B
^
|
C

C is inherited from B and B is inherited from A.

If you change interface B you change(add/remove method) interface C too. If you change interface A you change B and C. 

As proof it can happen is interface nsIAbDirectory in Thunderbird (just compare Tb3b4 and Tb3rc1 build1).

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> >Interface parent didn't change (using parent's name only, not IID).
> I think this is not proof enough that interface is changed.

Correct, it isn't. But apparently traditionally we haven't revved the subclass IID when the superclass IID changes. If we want to start doing that then fine but for now I just wanted to have an automated way to check that we had done the normal things we do.

Comment 9

[Johnathan Nightingale [:johnath]]

Talked with Mossop - he says that the portions of this work that were needed for 1.9.2 (identifying & fixing changed interfaces) have already been completed even though the tool itself isn't completed. So I don't want to close the bug (it's not done!) but I don't want to clear the blocking flag either (since Benjamin felt that identifying those breakages did block release.)

I'll therefore take the rather unusual step of marking this status1.9.2:final-fixed but leaving it open. I'm sure someone will kick me if they dislike this approach.

Comment 10

[palo misik]

Attachment: Added ability to detect inheritance issues.

Hi Dave,
I've extended your patch to have an ability to detect inheritance issues. 
It can detect issues I've described in Comment 7.

Comment 11

[palo misik]

I didn't expected that improvement i did will find issues in gecko.

I compared 1.8.1.23 with 1.9.2
here are serious problem that have to be resolved before 1.9.2 will be shipped

nsIDOMXULControlElement needs new iid due to changes in parent interface nsIDOMXULElement
 nsIDOMXULTextBoxElement needs new iid due to changes in parent interface nsIDOMXULElement
  nsIDOMXULMenuListElement needs new iid due to changes in parent interface nsIDOMXULElement
  nsIDOMXULMultiSelectControlElement needs new iid due to changes in parent interface nsIDOMXULElement
 nsIDOMXULLabeledControlElement needs new iid due to changes in parent interface nsIDOMXULElement
  nsIDOMXULCheckboxElement needs new iid due to changes in parent interface nsIDOMXULElement
  nsIDOMXULButtonElement needs new iid due to changes in parent interface nsIDOMXULElement
nsIDOMXULTreeElement needs new iid due to changes in parent interface nsIDOMXULElement
nsIDOMXULSelectControlItemElement needs new iid due to changes in parent interface nsIDOMXULElement
nsIDOMXULPopupElement needs new iid due to changes in parent interface nsIDOMXULElement
nsIDOMXULDescriptionElement needs new iid due to changes in parent interface nsIDOMXULElement
 nsIDOMXULLabelElement needs new iid due to changes in parent interface nsIDOMXULElement
nsIDOMXULImageElement needs new iid due to changes in parent interface nsIDOMXULElement
nsIStatusBarBiffManager needs new iid due to changes in parent interface nsIFolderListener
nsIExtendedExpatSink needs new iid due to changes in parent interface nsIExpatSink
nsIMsgCustomColumnHandler needs new iid due to changes in parent interface nsITreeView
nsINativeTreeView needs new iid due to changes in parent interface nsITreeView
nsIASN1Tree needs new iid due to changes in parent interface nsITreeView
nsIAuthPromptWrapper needs new iid due to changes in parent interface nsIAuthPrompt
nsIToolkitChromeRegistry needs new iid due to changes in parent interface nsIXULChromeRegistry
nsITransfer needs new iid due to changes in parent interface nsIWebProgressListener2
nsIDOMXULMenuListElement needs new iid due to changes in parent interface nsIDOMXULSelectControlElement
nsIDOMXULMultiSelectControlElement needs new iid due to changes in parent interface nsIDOMXULSelectControlElement
nsPIDNSService needs new iid due to changes in parent interface nsIDNSService
 nsIToolkitChromeRegistry needs new iid due to changes in parent interface nsIChromeRegistry
nsIAutoCompleteBaseResult needs new iid due to changes in parent interface nsIAutoCompleteResult
 nsIAutoCompleteMdbResult needs new iid due to changes in parent interface nsIAutoCompleteResult
  nsIAutoCompleteMdbResult2 needs new iid due to changes in parent interface nsIAutoCompleteResult

Comment 12

[Dave Townsend [:mossop]]

Like I said that isn't something we have traditionally enforced so there are bound to be issues.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So...  Just to be clear, I believe the time for IID changes was "before beta 1".  this is an important tool to have, but I don't believe the changes from comment 11 should block 1.9.2 (or happen, period, possibly).

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 410339 [details] [diff] [review]
WIP patch

This code is actually not quite right and I have a version that fixes some issues locally.

Comment 15

[palo misik]

I checked all issues reported in Comment 11 and all are true issues.

Comment 16

[Dave Townsend [:mossop]]

Attachment: WIP patch 2

Mostly there now, this adds some option parsing to adjust the behaviour of the tool. It can optionally display all changes even if the IID was correctly changed and will by default ignore changes to certain flags that shouldn't affect the IID but that can be turned off too. Also adds the check that the parent interface's IID hasn't changed either.

Made the output a lot nicer.

In an ideal world we can run this as part of the test suite using a known good browser.xpt from a previous version and comparing it against the newly built browser.xpt we can turn the tree orange for any checkin that changes an interface without changing its IID

Comment 17

[Scott Johnson (:jwir3)]

Attachment: checkiid.py

So, this is a mistake that I, for one, keep making, and both akeybl and I think this should be automated on the server-side. My intent was to simply write a python hook that disallows a push if an IDL file was changed without a corresponding IID change. The script I just added does this, BUT it doesn't take into account if there was a functional change (e.g. a new member or function added) or a non-functional change (e.g. a comment was reworded). 

I think I can make this more robust to include that, but before I do that, I'd like to know if this is the direction we want to proceed, or whether Mossop's patch is a better candidate...

Comment 18

[(no longer active)]

We can always use a=comment for those cases, right?

Comment 19

[Scott Johnson (:jwir3)]

Attachment: b477166

Better version of the same script, in patch form. (I wasn't sure where to place it in the tree, or if it should even go in the tree, so I just put it in tools/ for now).

Comment 20

[Benjamin Smedberg]

I think that we have two options of where to implement this:

1) at push time (to inbound/central/whatever).
2) At branch time for mozilla-aurora and mozilla-beta: compare the current tree against the previous release and see if any IID changes are missing.

I don't know whether this script would be good enough for a #1 style check. It theoretically has false-negatives (because there can be multiple interfaces in an IDL file) and certainly has some false positives, where we specifically are trying not to change the IID while still making some change in a file such as marking a method [noscript] etc. So if were going to make this a #1-style HG hook, we'd have to allow for a ba=foo style override.

We can of course parse the full IDL files, although we'd have to explicitly ignore #includes and such and stick to parsing only the current file.

The advantage of something like #2 (comparing XPT files from a release) is that you can check the whole inheritance chain as described in comment 7.

Perhaps we should do both.

Comment 21

[Scott Johnson (:jwir3)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #20)
> I think that we have two options of where to implement this:
> 
> 1) at push time (to inbound/central/whatever).
I personally favor this one, because I think it's the case (at least in my experience when this has happened) that I simply forget to rev the IID. Thus, if I tried to push and failed, then I could simply rev the IID and re-push. In other words, if we do it at this time, it seems like less overhead to simply fix the problem.

> 2) At branch time for mozilla-aurora and mozilla-beta: compare the current
> tree against the previous release and see if any IID changes are missing.
I think this is good, too, but it would require (slightly) more overhead to fix the problem if it's detected - instead of just revving the IID and re-pushing, the owner has to be found, a new bug probably has to be created, and a backout patch needs to be done (in order to prevent the change from affecting the merge). Thus, it seems more logical to not even have it in the commit history if there's a bug like this.

> I don't know whether this script would be good enough for a #1 style check.
> It theoretically has false-negatives (because there can be multiple
> interfaces in an IDL file) and certainly has some false positives, where we
> specifically are trying not to change the IID while still making some change
> in a file such as marking a method [noscript] etc. So if were going to make
> this a #1-style HG hook, we'd have to allow for a ba=foo style override.
It's a pretty early stage script, and you're right, it does have some issues. I wasn't aware that an IDL file could contain multiple interfaces (or, more accurately, I did know this, but I didn't think about it when developing the script).

It's perfectly reasonable that we have a ba= override for the first version of the script, just so that we _CAN_ override it, even if we think it's "good enough" for the #1 style hook. In reality, it's probably better to have _something_ that helps prevent this in most cases, rather than _nothing_, as is the situation right now. This is something that's super easy to let slip through as both a developer and reviewer, and it would be nice to just have a good server-side double-check.
 
> 
> We can of course parse the full IDL files, although we'd have to explicitly
> ignore #includes and such and stick to parsing only the current file.
Yeah, that was an idea I thought about, too. I'm not sure whether this is going to be too non-performant to do as a hook before checkins. My thinking is that we could check just IDL files that have been changed in a given patch, but check the whole file. My intuition tells me that it probably wouldn't be too bad of a situation.

It would be nice if we could add another letter to the tbpl builds indicating either green (during build all interface changes have been associated with a UUID change) or red (there is at least one interface change that doesn't have an IID change). My thinking is that we could do this with just a python script, similar to the one above that I posted, that simply runs server-side as a prerequisite during build.

> Perhaps we should do both.
FWIW, I think this is the best plan.

Comment 22

[Scott Johnson (:jwir3)]

If I understand the problem correctly, I think there are four cases where changes in the idl files within the repository could have consequences:

Case 1: New IDL File Added
 - We don't need to worry about this, since new interfaces will come with IID changes automatically.

Case 2: IDL File x is removed
 - We don't need to worry about this case, either, since the interfaces within file x are no longer available.

Case 3: IDL File x is moved/renamed to y
 - We need to verify that all interfaces within file y have new IIDs. Is this correct? Would a file rename/move affect binary compatibility?

Case 4: IDL File x is changed in place
 - We need to verify that any changes to an interface within x has a corresponding IID change.

Am I correct with how we should handle these cases, and also: are these the only cases? I've been focusing on Case 4, but I realized that perhaps it might be better to take a more general approach.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Would a file rename/move affect binary compatibility?

On its own, no.  If the file was also changed in the process, it might.

Comment 24

[Dave Townsend [:mossop]]

(In reply to Scott Johnson (:jwir3) from comment #17)
> I think I can make this more robust to include that, but before I do that,
> I'd like to know if this is the direction we want to proceed, or whether
> Mossop's patch is a better candidate...

Not sure I'm the one to answer this but I got flagged...

My patch is written against the C++ idl stuff. Last I heard that wasn't in use much so I don't know if it is up to date or not. My patch also checks compiled xpts against each other when it seems what you really want is to check source idls against each other. This is probably hard to do properly since most idls #include other idls so you'd need things like the include path from the build system to make it work with the normal parsers (they'll bail if they can't find the #include I think). I bet the python idl parsing stuff can be made to do this sanely though.

That doesn't really answer your question but it's what I've got.

Comment 25

[Scott Johnson (:jwir3)]

Attachment: b477166 (v2) (WIP)

Another version of the previous script. I fundamentally changed the way I was handling things to address the cases I described above. It's gone through preliminary testing (by me), but I'm going to go through individual, more targeted cases shortly. 

I know there's still debug code commented out in the patch, so I'm not sending it up for review just yet. I just want an idea of if I'm on the right track.

I don't know for certain if this is stable enough to be put in as an hg hook on the server-side yet, but it would probably be reliable enough to be run manually during merges. I don't know exactly what we would need to do to test it enough to make it a pre-push hook. Also, it doesn't currently have any override capability (e.g. ba=?, if something goes awry, and someone needs to force-push).

Comment 26

[Scott Johnson (:jwir3)]

Comment on attachment 709919 [details] [diff] [review]
b477166 (v2) (WIP)

Switching the feedback request over to ted, as he is the one who reviewed bug 813809.

Comment 27

[Scott Johnson (:jwir3)]

Comment on attachment 709919 [details] [diff] [review]
b477166 (v2) (WIP)

Found a few minor issues with the script (mostly false positives), which I'm working to correct. I'll post it for review following that. It was more complicated to write than I originally anticipated. :|

Also, just to let the powers that be know, I've found some valid interface changes without IID changes in the FF19 uplift (currently on the release channel) with the help of this script. I'll be posting bugs for these soon.

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Scott Johnson (:jwir3) from comment #27)
> Comment on attachment 709919 [details] [diff] [review]
> b477166 (v2) (WIP)
> 
> Found a few minor issues with the script (mostly false positives), which I'm
> working to correct. I'll post it for review following that. It was more
> complicated to write than I originally anticipated. :|
> 
> Also, just to let the powers that be know, I've found some valid interface
> changes without IID changes in the FF19 uplift (currently on the release
> channel) with the help of this script. I'll be posting bugs for these soon.

Hey Scott, just checking on progress of this hook . Would this be ready in time before the next merge happens (Apr 1) to verify on when FX21(Aurora) moves to Beta ?

Comment 29

[Scott Johnson (:jwir3)]

(In reply to bhavana bajaj [:bajaj] from comment #28)
> Hey Scott, just checking on progress of this hook . Would this be ready in
> time before the next merge happens (Apr 1) to verify on when FX21(Aurora)
> moves to Beta ?

Possibly. I'm not 100% sure, but I would say it's pretty likely (depending on how long review takes... this is going to be a pretty large and complex script, much moreso than I originally thought). I'm working on some multicol bugs right now that are taking precedence, but I've been working on it every free chance I get.

Comment 30

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #29)
> (In reply to bhavana bajaj [:bajaj] from comment #28)
> > Hey Scott, just checking on progress of this hook . Would this be ready in
> > time before the next merge happens (Apr 1) to verify on when FX21(Aurora)
> > moves to Beta ?
> 
> Possibly. I'm not 100% sure, but I would say it's pretty likely (depending
> on how long review takes... this is going to be a pretty large and complex
> script, much moreso than I originally thought). I'm working on some multicol
> bugs right now that are taking precedence, but I've been working on it every
> free chance I get.

By the way, there's probably going to be some number of changes that might slip through as 'false positives' - that is, they will be flagged by the script as requiring an IID change, but don't actually require one. I would suggest that, at least for the first few merges that this script is in place, we don't use it as an hg hook, but rather to check out work on a more 'manual' basis. This means, we download all the changesets that are being uplifted (or, in the case of mozilla-release, changes from mozilla-beta) and run the script on this diff file. 

If you want to do this (which is what I've been doing for a bit now to check my own work), you can run the script as-is right now without it being landed. I can give you instructions on how to do this if you'd like. That way, the script will get a bit of "alpha" testing. ;)

Comment 31

[Scott Johnson (:jwir3)]

Since this is sort of a standalone script for the time being, I'm going to use this bug as a method of tracking the status toward making this script a pre-commit hook. I set up a github repository to track the progress of this goal. It is available here:

https://github.com/jwir3/checkiid

Once that repo reaches the 'pre-commit hook' milestone, I'll re-post a patch to this bug for review. I anticipate it will be one or two release cycles before I do this, so we can get a feel for what issues still need to be resolved with this script.

Comment 32

[Andrew McCreight [:mccr8]]

Changing UUID is not required any more.