608651 - Yelp javascript broken (maps/filtering) for 4+ months NS_ERROR_XPC_BAD_CONVERT_JS/NS_ERROR_XPC_BAD_OP_ON_WN_PROTO

Status: VERIFIED FIXED

(Tech Evangelism Graveyard :: English US, defect)

Description

[Ed Lee :Mardak]

On the latest builds in a new profile, the javascript errors prevent the map from loading and filtering on yelp search results:

http://www.yelp.com/search

Error: uncaught exception: [Exception... "Could not convert JavaScript argument"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: http://media2.px.yelpcdn.com/static/201007092876478317/js/www-pkg-en_US.min.js :: anonymous :: line 158"  data: no]

There's also a bunch of yelp.event errors:

Error: yelp.event is undefined
Source File: http://www.yelp.com/search
Line: 76

This started breaking yelp sometime in June (tracemonkey):

0601 ok
0701 bad wrapped
0801 bad wrapped
0901 bad convert
1001 bad convert

At some point the exception switched to the current XPC_BAD_CONVERT from WrappedNative:

Error: uncaught exception: [Exception... "Illegal operation on WrappedNative prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame :: http://media2.px.yelpcdn.com/static/201007092876478317/js/www-pkg-en_US.min.js :: anonymous :: line 158"  data: no]

http://input.mozilla.com/en-US/search/?product=firefox&q=yelp

Comment 1

[Ed Lee :Mardak]

The line that this is breaking on is trying to access:

window.HTMLElement.prototype.style

e = "style"
exception thrown at c[e]

var e, d, b, a = window.SVGElement, c = window.HTMLElement;
if (a && c) {
  a = a.prototype;
  c = c.prototype;
  for (e in c){
    if (!(e in a) && Object.isFunction(d = c[e])) {
      if ((b = d + "").substr(b.indexOf("{")).replace(/\s/g, "") != "{[nativecode]}") {
        a[e] = d;
      }
    }
  }
}

Comment 2

[Boris Zbarsky [:bzbarsky]]

Right, so the minimal testcase is:

  javascript:window.HTMLElement.prototype.style

In 3.6, that returns Undefined.

The switch to XPC_BAD_CONVERT happened when we went from using castNative to using castNativeFromWrapper; presumably as part of fast-unwrapping for nsIDOMCSSInlineStyle...  But either way, why are we landing in this getter at all?  In 3.6, we have:

  "style" in window.HTMLElement.prototype

testing false, but on trunk it tests true?  That's the real issue here, imo....

Comment 3

[Boris Zbarsky [:bzbarsky]]

Cameron, I have two questions:

1)  What does WebIDL specify here in terms of the proto chain going forward? 
    Should ElementCSSInlineStyle.prototype be on the proto chain of
    HTMLElement.prototype?
2)  If so, then what should the "style" property getter do when invoked on
    HTMLElement.prototype?

Comment 4

[Cameron McCormack (:heycam)]

(In reply to comment #3)
> 1)  What does WebIDL specify here in terms of the proto chain going forward? 
>     Should ElementCSSInlineStyle.prototype be on the proto chain of

DOM Level 2 Style isn't written with Web IDL, although CSSOM is -- CSSOM has this:

  http://dev.w3.org/csswg/cssom/#the-elementcssinlinestyle-interface

with no indication of where the interface should be implemented.  DOM 2 Style says:

  The expectation is that an instance of the ElementCSSInlineStyle interface
  can be obtained by using binding-specific casting methods on an instance of
  the Element interface when the element supports inline CSS style
  informations.

So if we assumed CSSOM had either

  Element implements ElementCSSInlineStyle;

or

  HTMLElement implements ElementCSSInlineStyle;

then the style property would go on the mixin prototype object -- which would be an object that is the direct prototype of HTMLElement instances.

If the interface had a [Supplemental] on it (even though [Supplemental] is not defined in the spec yet -- Ian and Anne have been using it) then it would mean that the prototype object for whatever is on the left of the "implements" would get the "style" property.

That is as currently specified (and assuming [Supplemental] gets specified as it is being currently used).  Usual caveats about various people not liking the way prototype chains are currently defined here.  (Hopefully some progress on this can be made after meeting with TC39 folks in a couple of weeks.)

>     HTMLElement.prototype?
> 2)  If so, then what should the "style" property getter do when invoked on
>     HTMLElement.prototype?

It should throw a TypeError, due to step 2 of the "attribute getter":

  http://dev.w3.org/2006/webapi/WebIDL/#dfn-attribute-getter

since HTMLElement.prototype is just a regular native object, and not an instance of HTMLElement itself.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ok, so if the [Supplemental] thing happens, then our behavior is correct modulo the exception not being a TypeError.... (and yelp just needs to be fixed)  Otherwise, it's wrong.  Great.

I guess for now we should keep the old behavior....

Comment 6

[Aakash Desai [:aakashd]]

Seeing this on Input as well: http://input.mozilla.com/en-US/sites/theme/17690

Comment 7

[Brendan Eich [:brendan]]

Old behavior looks a de-facto standard we helped to set, no? Better stick with it for now, assuming we can ever change it.

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

m-c range for ("style" in HTMLElement.prototype) becoming true:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=befec51e10ab&tochange=c7eaf9387b1f

(matches comment 0 range).

So this is a regression from bug 562008.  The problem is that HTMLElement.prototype is now the direct proto of some elements, and we flatten all properties on the direct proto at the moment in XPConnect.  So HTMLElement.prototype ends up with all the properties of NodeSelector, NSElement, EventTarget, ElementCSSInlineStyle, NSHTMLElement, etc on it.

Peter, is there a sane way to stick another proto object between the sectioning elements and HTMLElement.prototype so they behave like all other HTML elements in that sense?  We need to either do that, back out bug 562008, totally change how XPConnect proto chains work, or evangelize Yelp, imo.

Comment 9

[Brendan Eich [:brendan]]

Is it only Yelp that counts on this? If we differ from past Firefox releases and all the other browsers, I smell trouble. Yet this bug is unassigned.

Johnny, who can take this one?

/be

Comment 10

[Johnny Stenback (:jst)]

I suspect what we need to do here is give our "HTMLElement" element classes a different class in JS, which in turn inherits from HTMLElement. That way HTMLElement.prototype would match (more or less) what it was in 3.6, yet we could have HTMLElement elements as we do now. If need be we could fudge what toString returns on HTMLElements such that they return "[object HTMLElement]" even if the underlying class is something else (suggestions?).

Mounir, I'm giving this to you, but I and/or peterv/bz can help here if needed.

Comment 11

[Peter Van der Beken [:peterv]]

Note that HTML5 has a style attribute on the HTMLElement interface. If we follow that then 'style in HTMLElement.prototype' really should be true, no? If that's the case I think we should at least try to evangelize Yelp.

Can we try to figure out if it's only Yelp? I discussed this with jst, not sure if we can spider for this. We'd need to be able to distinguish the direct property lookups on the prototype from just a general property lookup?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Note that HTML5 has a style attribute on the HTMLElement interface.

That's the [Supplemental] version from above.  Is this final enough that we can depend on it not changing?  If so, then yes we should evangelize Yelp...

Comment 13

[Cameron McCormack (:heycam)]

After discussions with TC39 yesterday, Web IDL is likely going to change so that interfaces like ElementCSSInlineStyle will definitely have their properties go on to the interfaces they're defined to augment.  So if we had

  Element implements ElementCSSInlineStyle;

in the relevant spec (CSSOM I guess), then you would have `'style' in Element.prototype`.  And if instead it was

  HTMLElement implements ElemnetCSSInlineStyle;

then it would be `'style' in HTMLElement.prototype`.

Which of these would be the case is up to the spec that defines ElementCSSInlineStyle.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Given comment 11, that's actually irrelevant, unless HTML5 changes.  So all that matters is whether properties end up on prototypes at all (which is still a big if; we'd need to convince at least the webkit folks).  If they do, then Yelp's code is broken for "title" and the like, not just "style".

Comment 15

[Cameron McCormack (:heycam)]

There seemed to be broad agreement here at the TC39 meeting about having `A implements B;` meaning that B's properties go on to A.prototype.

Comment 16

[Mounir Lamouri (:mounir)]

Sayrer, did you mark this bug has blocking b8 for a specific reason? It sounds like it would be better to wait for feedbacks on spec side before doing anything here.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Cameron, it's not even a matter of what A implements B means.  It's a question of what the |attribute| keyword means.   HTMLElement definitely has properties on it in the IDL.  If those end up as getter/setter pairs on HTMLElement.prototype (which is not what any shipping browser does right now for the specific case of HTMLElement, note, though we do put getter/setter pairs on other DOM prototypes) then this bug is evang.  If |attribute| just leads to own properties on objects, then what the site is doing is fine...

So we need to be pretty sure that making |attribute| into getter/setter pairs will stick, before evangelizing.  Imo.

Comment 18

[Cameron McCormack (:heycam)]

OK.  Oliver said he was happy with attributes corresponding to accessor properties on prototypes.

Comment 19

[Brendan Eich [:brendan]]

Sounds like we need to evangelize hard, including telling them WebKit's DOM will change to break them and track the WebIDL spec.

/be

Comment 20

[Peter Van der Beken [:peterv]]

For now I'd be fine with either turning this completely into evang or changing these elements' classinfo back into HTMLUnknownElement (since according to bz that apparently wouldn't break the html5test). I'd rather do that than special-case these with a custom toString and .constructor.

Comment 21

[Cameron McCormack (:heycam)]

Chrome bug for putting attributes on prototypes: http://code.google.com/p/chromium/issues/detail?id=43394

Comment 22

[Cameron McCormack (:heycam)]

And WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=49739

Comment 23

[Johnny Stenback (:jst)]

Over to tech evangelism then.

Comment 24

[Brendan Eich [:brendan]]

Blizzard said he'd take this one.

/be

Comment 25

[Christopher Blizzard (:blizzard)]

Now have contacts, will follow up.

Comment 26

[LegNeato]

Clearing blocking flag.

Comment 27

[LegNeato]

Apparently I failed to clear the beta8 flag previously. NOW it's cleared

Comment 30

[Christopher Blizzard (:blizzard)]

No updates from Yelp so far.

Comment 31

[Christopher Blizzard (:blizzard)]

Yelp says that it might be a few days, but the fix is in the pipeline.

Comment 32

[Ed Lee :Mardak]

Works for me now!