297603 - Some XBL bindings no longer compile the implementation when they should

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Karsten Düsterloh]

Mozilla trunk build 2005-06-13, Win2k

I have a custom XBL binding that extends one of Mozilla's mailWidgets
(chrome://messenger/content/mailWidgets.xml#mail-multi-emailHeaderField),
including the overloading of one of its methods (updateEmailAddressNode). 

This does not work on trunk anymore: 
when the function updateEmailAddressNode is called inside the base widget,
the base widget function is called, not my replacement anymore. 

Maybe this is just another fallout of the automatic XPCNativeWrapper stuff?

Anyway, XBL bindings that can't change the behaviour of the bindings they extend
anymore are pretty useless...

Comment 1

[Benjamin Smedberg]

This is an interesting problem: it's not clear to me at least that XBL methods
should be virtual, but if they are there should be a way to expicitly call the
base version (through .prototype.__parent__ perhaps?). In addition, the
interactions with interface declarations is interesting: should derived bindings
declare the same interfaces that base classes declare?

Comment 2

[Karsten Düsterloh]

(In reply to comment #1)
> This is an interesting problem:

*g*

> it's not clear to me at least that XBL methods should be virtual,

Well, up to now, at least, they were...

> but if they are there should be a way to expicitly call the
> base version (through .prototype.__parent__ perhaps?).

That's not the problem here, since I can't modify the base version.
There may be some workaround by using leet JS hacking, but I don't think that
this should be the way.

With the help of the German Mozilla newsgroup
de.comm.software.mozilla.nightly-builds, we were able to pin down the regression
to a time window of 2006-06-02:15 to 2005-06-02:19. The IMO most suspect
checkins there were for bug 292591 and maybe for bug 294795, but both are
security bugs I can't read.

I'm currently debugging that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Karsten, could you post a bonsai link for the range you think you found?  The
range you list in comment 2 doesn't include the fix for bug 294795 that I can see...

In any case, even if the new behavior is desirable (which I doubt) we should
understand _why_ it's happening to see whether something else could also have
gotten broken.

Comment 4

[Tobias Fischer]

Like Karsten wrote, we have tried some Builds and cut the timeframe down to
2005060215--working and 2005060219--broken. 

I have give something about 10 minutes plus for ending time at the Bonsai Query,
because I can't remember the Checkout-Time for the 2005060219 Build exactly:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-06-02+15%3A00%3A00&maxdate=2005-06-02+19%3A10%3A00&cvsroot=%2Fcvsroot

Comment 5

[Boris Zbarsky [:bzbarsky]]

That range in fact doesn't include bug 294795....

In any case, what bothers me is how this could happen at all.  There's only one
JSObject involved, right?  And the methods in question are just set on it via
JS_DefineUCProperty.  So as long as the more-derived binding is installed later,
it should automatically "override" (rather replace) the methods from an ancestor
binding.

Could it be that the child binding is somehow ending up with a PR_FALSE return
from AllowScripts()?  That is, do methods that are _not_ replacing ancestor
binding methods work?

Comment 6

[Karsten Düsterloh]

(In reply to comment #5)
> That range in fact doesn't include bug 294795....

Ah, okay, off by half an hour.

> Could it be that the child binding is somehow ending up with a PR_FALSE return
> from AllowScripts()?

AllowScripts() gets called *a lot*. Inspired by Bonsai, I already put a bunch of
printf's in, and at the time when a new element with the binding gets created,
it becomes |false| at the final return, because the (successful!) call to
CanExecuteScripts sets canExecute to |false|.

> That is, do methods that are _not_ replacing ancestor binding methods work?

No, they're missing also. The binding's <content> is there, though.

I'll try build a testcase.

Comment 7

[Boris Zbarsky [:bzbarsky]]

What's the filename of the file the binding is in?  Do you have JS disabled, by
chance?

Requesting blocking on the branches that bug 292591 landed on.

Comment 8

[Daniel Veditz [:dveditz]]

What kind of XBL bindings, are they installed as chrome or are they remote?

Comment 9

[Daniel Veditz [:dveditz]]

Absent a reduced testcase which Karsten is working on, this was initially seen
in Mnenhy 0.7.0.2's header display ("broken and shows just undefined for NGs etc.")

Comment 10

[Boris Zbarsky [:bzbarsky]]

Mnenhy has XBL in .xbl files, which don't get the system principal (unlike .xml
files).  Since JS is off in mail windows, I would fully expect said XBL to stop
working.  The solutions are to either put the XBL in .xml files or fix our
extension silliness for system principals in chrome somewhat.

Comment 11

[Karsten Düsterloh]

I dug down to the point where my .xbl got a principal different from
mSystemPrincipal, but didn't know how to interpret that - so I debugged myself
lost in some pretty irrelevant area for the last hours... :-/ 

Anyway:

(In reply to comment #10)
> Mnenhy has XBL in .xbl files, which don't get the system principal (unlike
> .xml files).  Since JS is off in mail windows, I would fully expect said XBL
> to stop working.

That's oh so very true. :-/

> The solutions are to either put the XBL in .xml files or fix our
> extension silliness for system principals in chrome somewhat.

I changed the binding to use an .xml file and now it's working again.
Thank you so much, bz! :)

Maybe we really should at least allow .xbl for chrome URIs?

So this bug here is very likely a dupe of this most probably already reported
problem, but I don't find a real fit (Bug 138850 and bug 269378 are related,
though)... Feel free to dupe.

Sorry folks for all the hassle - who would have thought that a file suffix would
stop working... :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

The file suffix should NOT have stopped working, so there was no hassle.

As a bare minimum, we should add "xbl" to the list of "system" extensions. 
Better would be to fix bug 221490.

Comment 13

[Daniel Veditz [:dveditz]]

Thanks for the detective work!

Comment 14

[Daniel Veditz [:dveditz]]

Attachment: branch fix for common .xbl extension only

For the branches bsmedberg thought we ought to be conservative and simply make
.xbl work--since that was the scope of the branch regression--rather than take
on the full change in bug 221490

Comment 15

[Benjamin Smedberg]

Comment on attachment 187009 [details] [diff] [review]
branch fix for common .xbl extension only

r=bsmedberg (branch only)

Comment 16

[Jay Patel [:jay]]

Comment on attachment 187009 [details] [diff] [review]
branch fix for common .xbl extension only

Let's get this checked in as soon as dbaron gives the sr=. a=jay

Comment 17

[Benjamin Smedberg]

No longer blocking the trunk now that bug 221490 is fixed.

Comment 18

[Daniel Veditz [:dveditz]]

This fix checked into 1.7 and aviary1.0.1 branches; bug 221490 fixes this on the
trunk.