Some XBL bindings no longer compile the implementation when they should

RESOLVED FIXED

Status

()

Core
XBL
--
major
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Karsten Düsterloh, Assigned: dveditz)

Tracking

({fixed-aviary1.0.5, fixed1.7.9, regression})

Trunk
fixed-aviary1.0.5, fixed1.7.9, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.9 +
blocking-aviary1.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: need landing)

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
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

13 years ago
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?
Summary: Can't "overload" XBL methods → XBL methods are no longer "virtual"
(Reporter)

Comment 2

13 years ago
(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.
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.
Flags: blocking1.8b3?

Comment 4

13 years ago
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

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?
(Reporter)

Comment 6

13 years ago
(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.
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.
Flags: blocking1.7.9?
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.0.5?
Summary: XBL methods are no longer "virtual" → Some XBL bindings no longer compile the implementation when they should
(Assignee)

Comment 8

13 years ago
What kind of XBL bindings, are they installed as chrome or are they remote?
(Assignee)

Comment 9

13 years ago
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.")
Flags: blocking1.8b3?
Flags: blocking1.8b3+
Flags: blocking1.7.9?
Flags: blocking1.7.9+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.5?
Flags: blocking-aviary1.0.5+
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.
(Reporter)

Comment 11

13 years ago
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... :(
Whiteboard: dupeme
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.
(Assignee)

Comment 13

13 years ago
Thanks for the detective work!
Assignee: general → dveditz
(Assignee)

Updated

13 years ago
Whiteboard: dupeme → patch in 221490
(Assignee)

Comment 14

13 years ago
Created attachment 187009 [details] [diff] [review]
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
Attachment #187009 - Flags: superreview?(dbaron)
Attachment #187009 - Flags: review?(benjamin)
Attachment #187009 - Flags: approval1.7.9?
Attachment #187009 - Flags: approval-aviary1.0.5?
(Assignee)

Updated

13 years ago
Whiteboard: patch in 221490 → need reviews

Comment 15

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

r=bsmedberg (branch only)
Attachment #187009 - Flags: review?(benjamin) → review+

Comment 16

13 years ago
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
Attachment #187009 - Flags: approval1.7.9?
Attachment #187009 - Flags: approval1.7.9+
Attachment #187009 - Flags: approval-aviary1.0.5?
Attachment #187009 - Flags: approval-aviary1.0.5+
Attachment #187009 - Flags: superreview?(dbaron) → superreview+

Updated

13 years ago
Whiteboard: need reviews → need landing

Comment 17

13 years ago
No longer blocking the trunk now that bug 221490 is fixed.
Flags: blocking1.8b3+
Flags: blocking-aviary1.1+
(Assignee)

Comment 18

13 years ago
This fix checked into 1.7 and aviary1.0.1 branches; bug 221490 fixes this on the
trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0.5, fixed1.7.9
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.