127418 - [FIXr]Adding XBL binding with method to HTML BODY element breaks DOM

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Matthew Wilson]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.8) Gecko/20020204
BuildID:    2002020406

If I add an XBL binding to the BODY of an HTML document using
body.style.setProperty ('-moz-binding', ...), and if that XBL binding has a
<method> in its <implementation>, then many (all?) other DOM operations on nodes
in the HTML document fail with an error

Exception: Illegal operation on WrappedNative prototype object nsresult:
0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)

Reproducible: Always
Steps to Reproduce:
Load the test case (I will attach this shortly). It tries to attach the binding
to a BODY, H1, H2 and H3 element in turn.

Actual Results:  Operations on the BODY succeed.
The binding is successfully set on the H1 element, but afterwards, retrieving
the nodetype fails with the above error.

Expected Results:  Mozilla should successfully set bindings and report the node
type for each element in turn.

If the attempt to change the body is removed from the test case, then all
operations succeed.

Comment 1

[Matthew Wilson]

Attachment: Test case: XBL bindings

Comment 2

[Matthew Wilson]

Attachment: Test case: HTML file

Comment 3

[Matthew Wilson]

For the test case, you'll need to download both files into the same directory
and rename the bindings file to a .xml extension, then adjust the HTML file so
that the pathnames point to the XML file.

Just discovered that it looks as though the problem goes away if the HTML file
uses a relative URL to refer to the bindings file.

Comment 4

[Matthew Wilson]

Attachment: HTML Test case referring to XBL document via HTTP

It seems that referring to the XBL bindings document via an http:// URL also
works.	So http:// succeeds, relative URLs succeed, file:// fails, and
chrome:// fails.

Comment 5

[Brendan Eich [:brendan]]

Do we need to fix this for mozilla1.0/xbl1.0?

DOM and XPConnect gurus cc'd -- I hope something obvious leaps out at you guys
and there's an easy fix.

/be

Comment 6

[Matthew Wilson]

Looks like there's nothing special about the HTML BODY - the test fails in the
same way if you add a DIV and manipulate that instead.

Also you can remove the <content> from the XBL bindings without changing the
behaviour - it's the <implementation> with a <method> that causes the problem.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The testcase worksforme from both HTTP and local disk.  Is this still an issue?

Comment 8

[Matthew Wilson]

In Mozilla 1.5 and Firebird 0.7 I still see failure in the following case:

1. save attachments 71072 and 71073 to local disk
2. edit the HTML file to specify the location of the XBL file as an absolute
file:// URL
3. load the HTML file

In this case the last non-error alert I see is
[after] h1 is [object HTMLHeadingElement]

followed by
[Exception... "Illegal operation on WrappedNative prototype object" <etc.> ]

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Standalone testcase -- just load this file

This should even work (as in, demonstrate the bug) directly from bugzilla, I
hope... if not, just save it to your hard drive.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So this is a longstanding (ever since the original "make inheritance work" code
landed) bug in nsXBLBinding::DoInitJSClass (which wasn't called that at the
time).  The problematic code in that function looks like this:

1092   if ((!::JS_LookupProperty(cx, global, aClassName.get(), &val)) ||
1093       JSVAL_IS_PRIMITIVE(val)) {
          // initialize proto here, using the current proto of "obj" as the
          // parent proto.  This sets the aClassName property of the global to
          // the proto we create
1166   }
1167   else {
1168     proto = JSVAL_TO_OBJECT(val);
1169   }
1170 
1171   // Set the prototype of our object to be the new class.
1172   if (!::JS_SetPrototype(cx, obj, proto)) {
1173     return NS_ERROR_FAILURE;
1174   }

In the attached testcase, the same exact binding is attached to two different
types of objects -- an <h1> and a <div>.  The aClassName is a property of the
_binding_.  So the first time through this code, we will create a new class with
the <h1>'s proto as it's proto and set the proto of the <h1> to that new class.
 The second time through, we will find the aClassName property of the window
object set (as the testcase shows) and will just get the value of that property
and use it as the proto for the <div>.

So now the proto chains look like

h1 -> XBL proto -> PrototypeHeaderElement (or whatever)
div -> XBL proto -> PrototypeHeaderElement (!)

In other words, we've screwed up the proto chain for the div.  This bug follows
(since XPConnect gets all unhappy).

As things stand, you simply can't have two different node types with the same
binding attached to them in the same document and have things work.  Hasn't come
up as a problem in XUL, since in XUL there is only one node type...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

This fixes the bug on at least these testcases by making the new protoclass
name depend on the classname of obj.  The extra string munging seems to be the
only reasonable way to do that....  Luckily, we will still hit the cached proto
all the time for XUL (since there is only one XULElement).

Other options include not using the cached proto off the global at all (this
may cause noticeable problems with Ts/Txul) and declaring this a "feature"
instead of a bug... ;)

Anyone have any other ideas on how to fix this?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: On second thought, maybe this is better....

The first approach has one more drawback -- it breaks for cases when objects of
the same class happen to have different prototypes (for example <applet>
elements with different applets, since applets muck with the prototype chain). 
This patch enforces the invariant we _really_ care about, which is that the the
proto of the cached val is the same as the proto of the incoming object.

The drawback of this patch is that it will end up "trashing" (always missing
out cached proto) if the same binding is attached to nodes of different types
alternately, but I doubt that's a common scenario.

A second (possible) drawback is that this will create different classes with
identical classnames.  Is that an issue?

Comment 13

[David Hyatt]

This is actually a known issue with XBL and is closely related to the prototype forking problem (see 
the prototype forking bug).  

I think the class names should probably be unique, but perhaps just munged using integers, e.g., 
BindingClass1, BindingClass2, etc.  I dislike including the name of a prototype, since the chain can 
be dynamically mucked with later anyway (as is the case when a chain of bindings is installed).

If you really want to kick ass on this bug, a prototype forking system should be able to look up a 
cached class based on the desired prototype object for that class (which may or may not be the 
current immediate prototype of the bound element's js object).  This system should share cached 
classes for any objects or classes that share the same prototype.

This is all reasonably straightforward, but what makes the problem difficult is when the prototype 
chain gets mucked with dynamically at a later date, e.g., if/when we support the ability to add/
remove bindings using the DOM.  That problem could be spun off into a separate bug though.

Comment 14

[Matthew Wilson]

> This should even work (as in, demonstrate the bug) directly from bugzilla, I
> hope... if not, just save it to your hard drive.

Actually I get a crash either way (in Firebird 0.7/WinNT).

Thanks for looking at this by the way.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Actually I get a crash either way (in Firebird 0.7/WinNT).

Time to upgrade?  ;) A recent nightly does not crash, and I recall some sort of
crash with that setup in 1.5.

The forking bug hyatt refers to is bug 41428. hyatt, why would the desired
prototype for the binding ever not be the current prototype of the element's
JSObject?  Note that right now, the element's prototype is used as the binding's
prototype, so in fact my second patch covers what needs to happen as far as I
can tell...

Comment 16

[Brendan Eich [:brendan]]

bz: identical class names for differeng bindings means last class initialized
wins, and defines the last binding's prototype under the given name.  Still,
patch 2 looks better to me.  It makes progress, AFAICT -- does it make anything
worse for a hard case?

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

> and defines the last binding's prototype under the given name

Right.  But all that means is that the corresponding property of the global will
be set to whatever.  As long as no one is depending on those properties on the
global (except for this code, of course), that should be fine...

I've been thinking some more about patch 2, and as far as I can tell its only
drawback is that it will thrash the one-element cache.  Comment 13 paragraph 3
is talking about a way to have a more-than-one-element cache here.

As for what comment 13 paragraph 4, is saying...  Adding bindings should be safe
with the "patch 2" approach.  Removing bindings, I've not fully thought through
yet.  I'm not sure when the 

Munging as suggested in comment 13 paragraph 2 makes it nontrivial to look up
the cached class (how do you know what integer it got munged with?).

Anyway, I'd like to know hyatt's answer to comment 15 before I say too many more
foolish things.  :)

Comment 18

[David Hyatt]

What we need here (this will make prototype forking work too) is to mangle the class name 
somehow, possibly just with integers, e.g., classname1, classname2, etc.  Then for some JSObject 
with a current prototype class of C and for a binding with a classname of B, we'd need to have a 
hash from B*C -> Bn, creating the class Bn as needed.

This would avoid collisions, allow for caching still, and ensure that any unique prototypes end up 
forking their own copy of the class.

Comment 19

[Boris Zbarsky [:bzbarsky]]

So...  Is the pointer to a prototype a stable thing?  That is, given binding B
and an object with prototype C, can we just take the JSObject* that is C, cast
to int, and append to the B URI?  That would do exactly what we want as far as I
can tell.

I guess my current patch assumes that the prototype pointer is in fact a stable
thing, huh?  ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: So maybe like this

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137069 [details] [diff] [review]
So maybe like this

Since hyatt is clearly reading this bug... ;)

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 137069 [details] [diff] [review]
So maybe like this

If the JS GC becomes a copying GC, then the JSObject* will change.  No worries
for the foreseeable future, but something to comment.

If you didn't have a class name identifying the prototype object, then the
object might become garbage and be collected.  The class-named property of the
window object is a necessary strong reference.

Does this really implement forking in the way that bug 41428 wants it?

/be

Comment 23

[David Hyatt]

I'd like to sit down with Brendan in person so we can work out whether or not this implements 
prototype forking.

Comment 24

[Boris Zbarsky [:bzbarsky]]

I'd need a clear explanation of what bug 41428 is talking about to know whether
this does it...  I can certainly add a comment regarding garbage collection
here.  If that gets implemented, we may need some sort of other way to uniquely
identify prototypes.

Comment 25

[Brendan Eich [:brendan]]

Attachment: proposed trivial jsapi extension: JS_GetObjectId

bz, can you use this for a little future proofing?  If it fails, return
NS_ERROR_OUT_OF_MEMORY.  If it succeeds, use a |jsid objid| local whose address
you pass as the |idp| formal as the cookie to sprintf at the end of the
classname (using %lx format, I suppose).

/be

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch using Brendan's suggestion

I rolled his patch into this diff, since I was testing it and all...

This seems to work just dandy.	sr=me on the JS part, as if that's needed.. ;)

Comment 27

[Mike Shaver (:shaver emeritus)]

Comment on attachment 137171 [details] [diff] [review]
proposed trivial jsapi extension: JS_GetObjectId

Aye, cap'n.

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 137172 [details] [diff] [review]
Patch using Brendan's suggestion

Great work, bz.  If hyatt likes it, and it tests ok, I say we get it in for
1.6.  Small patch, low risk.  Why wait?

Nit: preexisting code overparenthesizes here:

+  if ((!::JS_LookupProperty(cx, global, className.get(), &val)) ||
+      JSVAL_IS_PRIMITIVE(val)) {  

Fix that if you care.

/be

Comment 29

[Boris Zbarsky [:bzbarsky]]

Taking, so I don't lose this....

Comment 30

[Asa Dotzler [:asa]]

It's getting pretty late in the game for 1.6. Is this going to have necessary
reviews and be of low enough risk that we should be evaluating it for inclusion
in 1.6?

Comment 31

[Boris Zbarsky [:bzbarsky]]

It's low-risk, yes.  As for reviews, ask hyatt, not me.  I've done what I could
to get review, and hyatt is clearly reading the bug....  Then again, given the
number of outstanding review requests he has, I doubt he'll ever actually review
it.  <shrug>

Comment 32

[Brendan Eich [:brendan]]

Hyatt told me on IRC that he'd get to this, but not by 1.6final ship date.  I
don't think we need to hold 1.6 for it, it was more of a safe fix to ship there
(IMHO).  So bz, how about punting to 1.7a?

/be

Comment 33

[Asa Dotzler [:asa]]

OK. not a 1.6 blocker.

Comment 34

[Boris Zbarsky [:bzbarsky]]

All good with me; I wouldn't have been the one checking this in for 1.6 anyway... ;)

Comment 35

[Brendan Eich [:brendan]]

I checked the jsapi.[ch] changes reviewed by shaver (attachment 137171 [details] [diff] [review]) into the
1.7a trunk.

/be

Comment 36

[Boris Zbarsky [:bzbarsky]]

hyatt says r=hyatt via email.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137172 [details] [diff] [review]
Patch using Brendan's suggestion

Marking the r=hyatt

Comment 38

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.7a.