Closed Bug 608651 Opened 13 years ago Closed 13 years ago

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

Categories

(Tech Evangelism Graveyard :: English US, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Mardak, Unassigned)

References

()

Details

(Keywords: regression, top500, Whiteboard: [Input])

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
blocking2.0: ? → beta8+
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;
      }
    }
  }
}
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....
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?
(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.
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....
Seeing this on Input as well: http://input.mozilla.com/en-US/sites/theme/17690
Whiteboard: [Input]
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
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.
Blocks: 562008
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
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
OS: Mac OS X → All
Hardware: x86 → All
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.
Assignee: nobody → mounir.lamouri
Keywords: regression
Version: unspecified → Trunk
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?
> 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...
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.
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".
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.
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.
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.
OK.  Oliver said he was happy with attributes corresponding to accessor properties on prototypes.
Sounds like we need to evangelize hard, including telling them WebKit's DOM will change to break them and track the WebIDL spec.

/be
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.
Keywords: top500
Chrome bug for putting attributes on prototypes: http://code.google.com/p/chromium/issues/detail?id=43394
Over to tech evangelism then.
Assignee: mounir.lamouri → english-us
Component: DOM → English US
Product: Core → Tech Evangelism
QA Contact: general → english-us
Version: Trunk → unspecified
Blizzard said he'd take this one.

/be
Assignee: english-us → blizzard
Now have contacts, will follow up.
Status: NEW → ASSIGNED
Assignee: blizzard → nobody
Component: English US → General
Product: Tech Evangelism → Firefox
QA Contact: english-us → general
Clearing blocking flag.
Assignee: nobody → english-us
Component: General → English US
Product: Firefox → Tech Evangelism
QA Contact: general → english-us
Assignee: english-us → nobody
Component: English US → General
Product: Tech Evangelism → Firefox
QA Contact: english-us → general
blocking2.0: beta8+ → ---
Assignee: nobody → english-us
Component: General → English US
Product: Firefox → Tech Evangelism
QA Contact: general → english-us
Apparently I failed to clear the beta8 flag previously. NOW it's cleared
No updates from Yelp so far.
Yelp says that it might be a few days, but the fix is in the pipeline.
Works for me now!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Product: Tech Evangelism → Tech Evangelism Graveyard
You need to log in before you can comment on or make changes to this bug.