It was recently brought to our attention that a change to the context menu handling code has broken some extensions that built on the context menu. Davide Ficano found one of his extensions was broken when we replaced a property on the gContextMenu object, a helper object used to manage the context menu. The change was made in bug 449522 to accommodate new video element support.
I saw Davide’s blog post, but it sat in my many open tabs. Thankfully, Mike Kaply got my attention this morning on IRC. We filed bug 497098 and got a patch up and reviewed. Hopefully, this minimal fix will make it into the next Firefox 3.5 release candidate. SeaMonkey has a similar bug open.
This change has been in Firefox 3.5 (nee 3.1) since beta 2. We got a fix ready to go in around two days after the initial blog post mention. What could we have done to have seen this breakage months ago, when it initially landed?
UPDATE: The patch landed on trunk and the 3.5 branch. Thanks to everyone involved.
Looks like you attached the wrong patch to the bug – surely for something which is completely unused by Firefox, you wrote a test to keep it from being broken without any way for anyone to notice, didn’t you?
First of all somebody needs to “educate” the mozilla core developers to really think about add-on compatibility. Such a breakage wouldn’t have happened if developers indeed thought twice before modifying/removing existing behavior.
Always try to be backwards-compatible where it isn’t too much of a burden, like it is the case here. And consider that effectively all browser/toolkit code is likely used by extensions in some way, so be careful when changing method/property signatures or return values.
The case here isn’t actually a bad one. It would have been far worse if some method was changed in an incompatible way but some extension re-implemented it still the old way, so that not only the extension would break but the parts of browser (or other host app) as well.
I could furthermore imagine some (semi-)automated tools to help here. AMO already indexes much of the extension source code for the “view source” feature.
Hook that up with some mxr like system (js-hydra?) to make it easy for developers to search for example for uses of gContextMenu.imageURL prior to removing such a property.
And require to document changes that aren’t backwards-compatible, no matter how minor they are.
Right now MDC just documents major new features and major changes, but everything considered “minor” isn’t documented at all.
I remember FX3 removing the help-viewer in favor of sumo after it was beta5. This was completely undocumented. But there were in fact extensions that made use of it and relied on it. In fact there is still documentation on MDC (and IIRC code in toolkit) for prefpane.helpURI which is completely useless since helpviewer is gone. https://developer.mozilla.org/en/XUL/prefpane#a-helpURI
Hopefully the new SR policy of “anything that touches an API” will help?
Of course, it’s also going to mean anything in /browser/content/ needs SR, so… I wouldn’t be too surprised if it comes under discussion shortly after it gets applied.
What would _actually_ solve this: Putting XPIDL on everything (tabbrowser, nsContextMenu, other UI elements). People have been trained to actually think about compatibility when touching interfaces, and separating the interface from the implementation will mean it’s easier to see changes (see Mossop’s API tool).
… Or I’m on crack again. In fact, that’s more likely.
Hi Mark,
I’ve not considered to file a bug mainly because the imageURL property was removed on FF3.5b2 and I noticed it *only* in FF3.5b4, too late to ask a revision because it was declared “no more changes for add-on compatibility” after b1 AFAIK
Only a personal note, I was unhappy to read (not here) “So no extension author has taken care of it until now”, well I love to care of my extensions and their users I’ve created forums, blogs, svn repos, and wikis always updated but as 99% of extensions authors I can dedicate only my free time to extensions so if I miss a beta please don’t tell me “I don’t take care on my work”, period.
BTW I’m working to a new set of automated tests to run on every beta, the problem is how to easily interact with high ‘dynamic’ widgets like menus.
thank for you patch
> First of all somebody needs to “educate” the mozilla core developers to really
> think about add-on compatibility. Such a breakage wouldn’t have happened if
> developers indeed thought twice before modifying/removing existing behavior.
Education isn’t the problem here. Firefox reviewers are already very careful to consider addon compatibility. As you can see in the comments for bug 449522 (particularly comment 26 and comment 30), addon compatibility was certainly considered – we just dropped the ball on implementing one part of it (we did keep the saveImage/sendImage methods, just not the imageURL property).
I can understand that these kind of bugs can be frustrating for addon developers, but I don’t think it’s fair to suggest that they’re caused by ignorance. Sometimes we just make mistakes!
Phil – Tests would be nice for this, no doubt. Shawn will punish me for my testless patch. But in this case it probably would not have stopped us from making the breaking change. We would have just “fixed” the tests too.
davide – It’s never too late to let us know we broke something. Even if we can’t get the fix in the current release, we at least know about the problem and can get a fix ready for release ASAP. Thank you for blogging about the problem!
Nils – I completely agree with Gavin on this one. We knew it was a potential problem and we thought we had add-on compatibility handled. We missed a piece, that’s all.
Just to be clear, I didn’t talk about this particular bug.
Here it indeed seems that there was the intension to provide backward-compatibility but the even though .imageURL was discussed it was (accidentally) omitted in the patch. An honest mistake, no arguing here.
After re-reading my comment it indeed creates the impression that I tried to blame the author and reviewer of this particular patch for not considering add-ons. That’s not what I intended, and I’m sorry if that upset people.